On Wed, May 02, 2018 at 05:12:50PM +0200, Florian Weimer wrote: > On 05/02/2018 04:30 PM, Dave Hansen wrote: > >On 05/02/2018 06:26 AM, Florian Weimer wrote: > >>pkeys support for IBM POWER intends to inherited the access rights of > >>the current thread in signal handlers. The advantage is that this > >>preserves access to memory regions associated with non-default keys, > >>enabling additional usage scenarios for memory protection keys which > >>currently do not work on x86 due to the unconditional reset to the > >>(configurable) default key in signal handlers. > > > >What's the usage scenario that does not work? > > Here's what I want to do: > > Nick Clifton wrote a binutils patch which puts the .got.plt section > on separate pages. We allocate a protection key for it, assign it > to all such sections in the process image, and change the access > rights of the main thread to disallow writes via that key during > process startup. In _dl_fixup, we enable write access to the GOT, > update the GOT entry, and then disable it again. > > This way, we have a pretty safe form of lazy binding, without having > to resort to BIND_NOW. > > With the current kernel behavior on x86, we cannot do that because > signal handlers revert to the default (deny) access rights, so the > GOT turns inaccessible. > > >>Consequently, this commit updates the x86 implementation to preserve > >>the PKRU register value of the interrupted context in signal handlers. > >>If a key is allocated successfully with the PKEY_ALLOC_SIGNALINHERIT > >>flag, the application can assume this signal inheritance behavior. > > > >I think this is a pretty gross misuse of the API. Adding an argument to > >pkey_alloc() is something that folks would assume would impact the key > >being *allocated*, not pkeys behavior across the process as a whole. > > From the application point of view, only the allocated key is > affected—it has specific semantics that were undefined before and > varied between x86 and POWER. > > >>This change does not affect the init_pkru optimization because if the > >>thread's PKRU register is zero due to the init_pkru setting, it will > >>remain zero in the signal handler through inheritance from the > >>interrupted context. > > > >I think you are right, but it's rather convoluted. It does: > > > >1. Running with PKRU in the init state > >2. Kernel saves off init-state-PKRU XSAVE signal buffer > >3. Enter signal, kernel XRSTOR (may) set the init state again > >4. fpu__clear() does __write_pkru(), takes it out of the init state > >5. Signal handler runs, exits > >6. fpu__restore_sig() XRSTOR's the state from #2, taking PKRU back to > > the init state > > Isn't that just the cost of not hard-coding the XSAVE area layout? > > >But, about the patch in general: > > > >I'm not a big fan of doing this in such a PKRU-specific way. It would > >be nice to have this available for all XSAVE states. It would also keep > >you from so unnecessarily frobbing with WRPKRU in fpu__clear(). You > >could just clear the PKRU bit in the Requested Feature BitMap (RFBM) > >passed to XRSTOR. That would be much straightforward and able to be > >more easily extended to more states. > > I don't see where I could plug this into the current kernel sources. > Would you please provide some pointers? > > >PKRU is now preserved on signal entry, but not signal exit. Was that > >intentional? That seems like odd behavior, and also differs from the > >POWER implementation as I understand it. > > Ram, would you please comment? on POWER the pkey behavior will remain the same at entry or at exit from the signal handler. For eg: if a key is read-disabled on entry into the signal handler, and gets read-enabled in the signal handler, than it will continue to be read-enabled on return from the signal handler. In other words, changes to key permissions persist across signal boundaries. > > I think it is a bug not restore the access rights to the former > value in the interrupted context. In userspace, we have exactly > this problem with errno, and it can lead to subtle bugs. > > Thanks, > Florian -- Ram Pai
