Re: [PATCH] pkeys: Introduce PKEY_ALLOC_SIGNALINHERIT and change signal semantics

[Florian Weimer <fweimer@redhat.com>]

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?

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