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@xxxxxxx>
