Re: PKU usage improvements for threads

["Andy Lutomirski" <luto@kernel.org>]

On Fri, Sep 2, 2022, at 5:14 PM, Fangfei Yang wrote:
> I guess the question here is whether the code to call sigaltstack and 
> signal handler is considered part of the security code (sigreturn 
> obviously has to be, since the kernel has to restore the PKRU based on 
> the saved fpu).
> I think to a large extent this is necessary, at least for the signal 
> handler to be able to access the relevant registers at the time of the 
> interrupt, which may contain data that the handler should not have 
> access to. Even specifying a PKRU at the time of signal registration 
> would make the system functionally sound and safe since the relevant 
> calls must be protected.
>
> It's just that the design here should be such as to minimize the ways 
> in which the interface can be abused (e.g., accidental override access) 
> as well as to simplify the difficulty of writing secure code. It might 
> be reasonable, then, to save the PKRU when the `sigaltstack` is called.
>
> The main purpose is to simplify the design of the handler entry point 
> without adding new system calls, while not accidentally gaining 
> privileges that do not belong to the current PKRU because of the system 
> call, whether immediately, or later in signal delivery.

I think you might be so much more familiar with the system you’re working on than anyone else that you’re not explaining some basics and we’re all lost.

How is PKRU a “privilege” and what do you mean my “immediately”?  I can’t follow this.

>
> This is because this part of the design can be largely made easier if 
> additional source checking and PKRU switching by the handler at the 
> entry point can be avoided.

Why would the entry point check a source?  Or change PKRU?  What would its PKRU logic be and why?

As I see it, the handler can (awkwardly, perhaps) manage PKRU just fine for all purposes except kernel access to the signal stack.

>
> As `WRPKRU` can be abused, if the handler uses this instruction, 
> additional SP as well as PKRU checks must be performed to prevent 
> malicious programs from forging signals, and the check must get 
> multiplex among all threads. However, for the kernel, it takes very 
> little code to avoid these checks by giving the handler the PKRU it 
> wants.

Can you elaborate?  Of course WRPKRU can be abused to fully bypass PKRU protection.

>
> If only one PKEY is specified, then it is likely that `WRPKRU` is still 
> needed, since the TCB itself may occupy multiple PKEYs, or, the handler 
> need to access the memory of other PKEYs (e.g., complex multi-domain 
> signal designs).
>
> And, logically, it makes sense for a signal context (sigaltstack) to 
> have the same PKRU when it is registered, and when it is used in the 
> future. Thus, a special flag in `ss_flags & SS_SAVEPKRU` to ask the 
> kernel to save the current PKRU would be sufficient.

This isn’t logical at all to me. It makes some sense as an API simplification to avoid a new syscall, and it makes sense in a bizarre (to me) world in which user code can control access to PKRU but not to sigaltstack(), but why do we live in that world?

>
> From the security side, if the current PKRU does not have access to the 
> signal stack, then a future signal occurring when the kernel uses this 
> PKRU to write will also result in an segfault, thus avoiding unwanted 
> access through sigaltstack.

Do you mean in current kernels?

> This is also more accurate than checking the PKEY of the page when 
> registering the signal stack (if we restricted the PKRU when 

What do you mean “accurate”?


> registering the sigaltstack). Consider a possible error: a page is 
> accidentally unmaped after being registered as a signal stack, and then 
> another page that should not have been accessed by this PKRU is mapped 
> to the same location, thus causing an override during signal delivery.
>
>> I also bet we could do this with minimal new ABI.  There's already a
>> ->ss_flags field.  We could assign a flag to mean that stack_t doesn't
>> end at '->ss_size' and that there's a pkey value *after* ss_size.  I do
>> think having a single pkey that is made accessible before signal entry
>> is a more flexible ABI than taking an explicit PKRU value.
>
> Agreed, the most flexible way should be allow setting the PKRU to any 
> subset of the current PKRU. So we can check `(~new_pkru) & current_pkru 
> == 0` when calling sigaltstack. 
>
> However, no matter how it is done, one of the more disgusting thing is 
> that code like this appears in the program that handles the signal.
> ```
> old_pkru = read_pkru();
> write_pkru(stack_pkru);
> do_xsave(); 
> *(fpu_saved + pkru_offset()) = old_pkru; // this may be an argument of 
> fpu function call
> ```
> And when restoring, you also need
> ```
> old_pkru = *(fpu_saved + pkru_offset());
> *(fpu_saved + pkru_offset()) = stack_pkru;
> do_xstor();
> write_pkru(old_pkru);

Sorry, what code does XSAVE here?

> ```
> These plus the testing of the current runtime environment (MPK) are 
> truly disgusting. It's just structually ugly.