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

[Andy Lutomirski <luto@kernel.org>]

On Wed, May 2, 2018 at 10:17 AM Florian Weimer <fweimer@redhat.com> wrote:

> On 05/02/2018 07:09 PM, Andy Lutomirski wrote:
> >> 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.

> > Dave is right: the current behavior was my request, and I still think
it’s correct.  The whole point is that, even if something nasty happens
like a SIGALRM handler hitting in the middle of _dl_fixup, the SIGALRM
handler is preventing from accidentally writing to the protected memory.
When SIGALRM returns, PKRU should get restored
> >
> > Another way of looking at this is that the kernel would like to
approximate what the ISA behavior*should*  have been: the whole sequence
“modify PKRU; access memory; restore PKRU” should be as atomic as possible.
> >
> > Florian, what is the actual problematic sequence of events?

> See above.  The signal handler will crash if it calls any non-local
> function through the GOT because with the default access rights, it's
> not readable in the signal handler.

> Any use of memory protection keys for basic infrastructure will run into
> this problem, so I think the current kernel behavior is not very useful.
>    It's also x86-specific.

>   From a security perspective, the atomic behavior is not very useful
> because you generally want to modify PKRU *before* computing the details
> of the memory access, so that you don't have a general “poke anywhere
> with this access right” primitive in the text segment.  (I called this
> the “suffix problem” in another context.)


Ugh, right.  It's been long enough that I forgot about the underlying
issue.  A big part of the problem here is that pkey_alloc() should set the
initial value of the key across all threads, but it *can't*.  There is
literally no way to do it in a multithreaded program that uses RDPKRU and
WRPKRU.

But I think the right fix, at least for your use case, is to have a per-mm
init_pkru variable that starts as "deny all".  We'd add a new pkey_alloc()
flag like PKEY_ALLOC_UPDATE_INITIAL_STATE that causes the specified mode to
update init_pkru.  New threads and delivered signals would get the
init_pkru state instead of the hardcoded default.