On Wed, 2023-11-15 at 18:43 +0000, Mark Brown wrote:
end marker token (0) needs it i guess.
x86 doesn't currently have end markers. Actually, that's a point - should we add a flag for specifying the use of end markers here? There's code in my map_shadow_stack() implementation for arm64 which does that.
Hmm, I guess there isn't a way to pass a flag for the initial exec stack? So probably it should just mirror that behavior. Unless you think a lot of people would like to skip the default behavior.
And of course we don't have a marker on x86 (TODO with alt shadow stacks). We could still check for size < 8 if we want it to be a universal thing.
otherwise 0 size would be fine: the child may not execute a call instruction at all.
It seems like a special case. Where should the SSP be for the new thread?
Well, a size of specifically zero will result in a fallback to implicit allocation/sizing of the stack as things stand so this is specifically the case where a size has been specified but is smaller than a single entry.
I think for CLONE_VM we should not require a non-zero size. Speaking of CLONE_VM we should probably be clear on what the expected behavior is for situations when a new shadow stack is not usually allocated. !CLONE_VM || CLONE_VFORK will use the existing shadow stack. Should we require shadow_stack_size be zero in this case, or just ignore it? I'd lean towards requiring it to be zero so userspace doesn't pass garbage in that we have to accommodate later. What we could possibly need to do around that though, I'm not sure. What do you think?
Yes, requiring it to be zero in that case makes sense I think.
i think the condition is "no specified separate stack for the child (stack==0 || stack==sp)".
CLONE_VFORK does not imply that the existing stack will be used (a stack for the child can be specified, i think both glibc and musl do this in posix_spawn).
That also works as a check I think, though it requires the arch to check for the stack==sp case - I hadn't been aware of the posix_spawn() usage, the above checks Rick suggested just follow the handling for implicit allocation we have currently.
I didn't realize it was passing its own stack either. I guess the reason is to avoid stack overflows. But none of the specific reasons listed in the comments seem to applicable to shadow stacks.
What is the case for stack=sp bit of the logic?
I need to look into this more. My first thought is, passing in a stack doesn't absolutely mean they want a new shadow stack allocated either. We are changing one heuristic, for another.
The other thought is, the new behavior in the !CLONE_VM case doesn't seem optimal. fork has ->stack==0. Then we would be allocating a stack in only the child's MM and changing the SSP there, and for what reason? So I don't think we should fully move away from taking hints from the CLONE flags.
Maybe an alternate could just be to lose the CLONE_VFORK specific stack sharing logic. CLONE_VM always gets a new shadow stack. I don't think it would disturb userspace functionally, but just involves more mapping/unmapping.