Messages in this thread | | | Date | Mon, 05 Sep 2022 21:34:30 -0700 | From | "Andy Lutomirski" <> | Subject | Re: 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.
| |