Re: pkeys: Support setting access rights for signal handlers

[Ram Pai <linuxram-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

On Sat, Dec 16, 2017 at 04:25:14PM +0100, Florian Weimer wrote:
> On 12/16/2017 04:09 PM, Ram Pai wrote:
> 
> >>It still restores the PKRU register value upon
> >>regular exit from the signal handler, which I think is something we
> >>should keep.
> >
> >On x86, the pkru value is restored, on return from the signal handler,
> >to the value before the signal handler was called. right?
> >
> >In other words, if 'x' was the value when signal handler was called, it
> >will be 'x' when return from the signal handler.
> >
> >If correct, than it is consistent with the behavior on POWER.
> 
> That's good to know.  I tended to implement the same semantics on x86.
> 
> >>I think we still should add a flag, so that applications can easily
> >>determine if a kernel has this patch.  Setting up a signal handler,
> >>sending the signal, and thus checking for inheritance is a bit
> >>involved, and we'd have to do this in the dynamic linker before we
> >>can use pkeys to harden lazy binding.  The flag could just be a
> >>no-op, apart from the lack of an EINVAL failure if it is specified.
> >
> >Sorry. I am little confused.  What should I implement on POWER?
> >PKEY_ALLOC_SETSIGNAL semantics?
> 
> No, we would add a flag, with a different name, and this patch only:
> 
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index ec39f73..021f1d4 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -523,14 +523,17 @@ static int do_mprotect_pkey(unsigned long
> start, size_t l
>         return do_mprotect_pkey(start, len, prot, pkey);
>  }
> 
> +#define PKEY_ALLOC_FLAGS ((unsigned long) (PKEY_ALLOC_SETSIGNAL))
> +
>  SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
>  {
>         int pkey;
>         int ret;
> 
> -       /* No flags supported yet. */
> -       if (flags)
> +       /* check for unsupported flags */
> +       if (flags & ~PKEY_ALLOC_FLAGS)
>                 return -EINVAL;
> +
>         /* check for unsupported init values */
>         if (init_val & ~PKEY_ACCESS_MASK)
>                 return -EINVAL;
> 
> 
> This way, an application can specify the flag during key allocation,
> and knows that if the allocation succeeds, the kernel implements
> access rights inheritance in signal handlers.  I think we need this
> so that applications which are incompatible with the earlier x86
> implementation of memory protection keys do not use them.
> 
> With my second patch (not the first one implementing
> PKEY_ALLOC_SETSIGNAL), no further changes to architecture=specific
> code are needed, except for the definition of the flag in the header
> files.

Ok. Sounds like I do not have much to do. My patches in its current form
will continue to work and provide the semantics you envision.


> 
> I'm open to a different way towards conveying this information to
> userspace.  I don't want to probe for the behavior by sending a
> signal because that is quite involved and would also be visible in
> debuggers, confusing programmers.

I am fine with your proposal.
RP