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.
