lkml.org 
[lkml]   [2022]   [Sep]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: PKU usage improvements for threads


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.

\
 
 \ /
  Last update: 2022-09-06 06:39    [W:1.439 / U:0.244 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site