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