Message-Id: <20220903001412.17015-1-yangff1@gmail.com>
Date:   Fri,  2 Sep 2022 19:14:12 -0500
From:   Fangfei Yang <yangff1@...il.com>
To:     dave.hansen@...el.com
Cc:     dave.hansen@...ux.intel.com, keescook@...omium.org,
        linux-kernel@...r.kernel.org, luto@...nel.org,
        sroettger@...gle.com, x86@...nel.org
Subject: Re: PKU usage improvements for threads


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.

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.

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.

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.

>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.
This is also more accurate than checking the PKEY of the page when registering the signal stack (if we restricted the PKRU when 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);
```
These plus the testing of the current runtime environment (MPK) are truly disgusting. It's just structually ugly.

Powered by blists - more mailing lists