Re: [PATCH RFC RFT v2 2/5] fork: Add shadow stack support to clone3()

Edgecombe, Rick P
2023-11-16T00:52:15Z

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.