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

[Ram Pai <linuxram@us.ibm.com>]

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
> affecteda??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