public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Fangfei Yang <yangff1@gmail.com>
To: dave.hansen@intel.com
Cc: dave.hansen@linux.intel.com, keescook@chromium.org,
	linux-kernel@vger.kernel.org, luto@kernel.org,
	sroettger@google.com, x86@kernel.org
Subject: Re: PKU usage improvements for threads
Date: Fri,  2 Sep 2022 19:14:12 -0500	[thread overview]
Message-ID: <20220903001412.17015-1-yangff1@gmail.com> (raw)
In-Reply-To: <b4f0dca5-1d15-67f7-4600-9a0a91e9d0bd@intel.com>


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.

  parent reply	other threads:[~2022-09-03  0:14 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-22 20:40 PKU usage improvements for threads Kees Cook
2022-08-22 21:11 ` Dave Hansen
2022-08-23 11:08   ` Stephen Röttger
2022-08-23 18:12     ` Dave Hansen
2022-08-23 18:24       ` Andy Lutomirski
2022-08-24  8:51         ` Stephen Röttger
2022-08-24 16:28           ` Dave Hansen
2022-08-24 16:45           ` Andy Lutomirski
2022-08-25 12:30             ` Stephen Röttger
2022-08-25 14:36               ` Dave Hansen
2022-09-02 17:18                 ` Andy Lutomirski
2022-09-03  0:16         ` Fangfei Yang
2022-09-03  0:14       ` Fangfei Yang [this message]
2022-09-06  4:34         ` Andy Lutomirski
2022-09-06  5:58           ` Fangfei Yang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220903001412.17015-1-yangff1@gmail.com \
    --to=yangff1@gmail.com \
    --cc=dave.hansen@intel.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=sroettger@google.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox