On Mon, Oct 23, 2023 at 04:32:22PM +0000, Edgecombe, Rick P wrote: > On Mon, 2023-10-23 at 14:20 +0100, Mark Brown wrote: > +Some security folks I *think* I captured everyone for future versions but I might've missed some, it's a long Cc list. > > Add parameters to clone3() specifying the address and size of a > > shadow > > stack for the newly created process, we validate that the range > > specified > > is accessible to userspace but do not validate that it has been > > mapped > > appropriately for use as a shadow stack (normally via > > map_shadow_stack()). > > If the shadow stack is specified in this way then the caller is > > responsible > > for freeing the memory as with the main stack. If no shadow stack is > > specified then the existing implicit allocation and freeing behaviour > > is > > maintained. > This will give userspace new powers, very close to a "set SSP" ability. > They could start a new thread on an active shadow stack, corrupt it, > etc. That's true. > One way to avoid this would be to have shstk_alloc_thread_stack() > consume a token on the shadow stack passed in the clone args. But it's > tricky because there is not a CMPXCHG, on x86 at least, that works with > shadow stack accesses. So the kernel would probably have to GUP the > page and do a normal CMPXCHG off of the direct map. > That said, it's already possible to get two threads on the same shadow > stack by unmapping one and mapping another shadow stack in the same > place, while the target thread is not doing a call/ret. I don't know if > there is anything we could do about that without serious compatibility > restrictions. But this patch would make it a bit more trivial. > I might lean towards the token solution, even if it becomes more heavy > weight to use clone3 in this way. It depends on whether the above is > worth defending. Right. We're already adding the cost of the extra map_shadow_stack() so it doesn't seem that out of scope. We could also allow clone3() to be used for allocation, potentially removing the ability to specify the address entirely and only specifying the size. I did consider that option but it felt awkward in the API, though equally the whole shadow stack allocation thing is a bit that way. That would avoid concerns about placing and validating tokens entirely but gives less control to userspace. This also doesn't do anything to stop anyone trying to allocate sub page shadow stacks if they're trying to save memory with all the lack of overrun protection that implies, though that seems to me to be much more of a deliberate decision that people might make, a token would prevent that too unless write access to the shadow stack is enabled. > > + /* > > + * This doesn't validate that the addresses are > > mapped > > + * VM_SHADOW_STACK, just that they're mapped at all. > > + */ > It just checks the range, right? Yes, same check as for the normal stack.
Attachment:
signature.asc
Description: PGP signature
