Re: [PATCH RFT v8 4/9] fork: Add shadow stack support to clone3()

[Catalin Marinas <catalin.marinas@arm.com>]

On Tue, Aug 13, 2024 at 07:58:26PM +0100, Mark Brown wrote:
> On Tue, Aug 13, 2024 at 05:25:47PM +0100, Catalin Marinas wrote:
> > However, the x86 would be slightly inconsistent here between clone() and
> > clone3(). I guess it depends how you look at it. The classic clone()
> > syscall, if one doesn't pass CLONE_VM but does set new stack, there's no
> > new shadow stack allocated which I'd expect since it's a new stack.
> > Well, I doubt anyone cares about this scenario. Are there real cases of
> > !CLONE_VM but with a new stack?
> 
> ISTR the concerns were around someone being clever with vfork() but I
> don't remember anything super concrete.  In terms of the inconsistency
> here that was actually another thing that came up - if userspace
> specifies a stack for clone3() it'll just get used even with CLONE_VFORK
> so it seemed to make sense to do the same thing for the shadow stack.
> This was part of the thinking when we were looking at it, if you can
> specify a regular stack you should be able to specify a shadow stack.

Yes, I agree. But by this logic, I was wondering why the current clone()
behaviour does not allocate a shadow stack when a new stack is
requested with CLONE_VFORK. That's rather theoretical though and we may
not want to change the ABI.

> > > > I'm confused that we need to consume the token here. I could not find
> > > > the default shadow stack allocation doing this, only setting it via
> > > > create_rstor_token() (or I did not search enough). In the default case,
> 
> > > As discussed for a couple of previous versions if we don't have the
> > > token and userspace can specify any old shadow stack page as the shadow
> > > stack this allows clone3() to be used to overwrite the shadow stack of
> > > another thread, you can point to a shadow stack page which is currently
> 
> > IIUC, the kernel-allocated shadow stack will have the token always set
> > while the user-allocated one will be cleared. I was looking to
> 
> No, when the kernel allocates we don't bother with tokens at all.  We
> only look for and clear a token with the user specified shadow stack.

Ah, you are right, I misread the alloc_shstk() function. It takes a
set_res_tok parameter which is false for the normal allocation.

> > I guess I was rather questioning the current choices than the new
> > clone3() ABI. But even for the new clone3() ABI, does it make sense to
> > set up a shadow stack if the current stack isn't changed? We'll end up
> > with a lot of possible combinations that will never get tested but
> > potentially become obscure ABI. Limiting the options to the sane choices
> > only helps with validation and unsurprising changes later on.
> 
> OTOH if we add the restrictions it's more code (and more test code) to
> check, and thinking about if we've missed some important use case.  Not
> that it's a *huge* amount of code, like I say I'd not be too unhappy
> with adding a restriction on having a regular stack specified in order
> to specify a shadow stack.

I guess we just follow the normal stack behaviour for clone3(), at least
we'd be consistent with that.

Anyway, I understood this patch now and the ABI decisions. FWIW:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>