All of lore.kernel.org
 help / color / mirror / Atom feed
From: Florian Weimer <fweimer@redhat.com>
To: Ram Pai <linuxram@us.ibm.com>
Cc: Dave Hansen <dave.hansen@intel.com>,
	linux-mm <linux-mm@kvack.org>,
	x86@kernel.org, linux-arch <linux-arch@vger.kernel.org>,
	linux-x86_64@vger.kernel.org,
	Linux API <linux-api@vger.kernel.org>
Subject: Re: pkeys: Support setting access rights for signal handlers
Date: Sat, 16 Dec 2017 16:25:14 +0100	[thread overview]
Message-ID: <2eba29f4-804d-b211-1293-52a567739cad@redhat.com> (raw)
In-Reply-To: <20171216150910.GA5461@ram.oc3035372033.ibm.com>

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.

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.

Thanks,
Florian

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

WARNING: multiple messages have this Message-ID (diff)
From: Florian Weimer <fweimer@redhat.com>
To: Ram Pai <linuxram@us.ibm.com>
Cc: Dave Hansen <dave.hansen@intel.com>,
	linux-mm <linux-mm@kvack.org>,
	x86@kernel.org, linux-arch <linux-arch@vger.kernel.org>,
	linux-x86_64@vger.kernel.org,
	Linux API <linux-api@vger.kernel.org>
Subject: Re: pkeys: Support setting access rights for signal handlers
Date: Sat, 16 Dec 2017 16:25:14 +0100	[thread overview]
Message-ID: <2eba29f4-804d-b211-1293-52a567739cad@redhat.com> (raw)
Message-ID: <20171216152514.g1OyYqfUz_BQh90Tkrr8dX4zK2baTizr2VIpI596PvE@z> (raw)
In-Reply-To: <20171216150910.GA5461@ram.oc3035372033.ibm.com>

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.

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.

Thanks,
Florian

  reply	other threads:[~2017-12-16 15:25 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-09 21:16 pkeys: Support setting access rights for signal handlers Florian Weimer
     [not found] ` <5fee976a-42d4-d469-7058-b78ad8897219-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2017-12-10  0:17   ` Dave Hansen
2017-12-10  0:17     ` Dave Hansen
2017-12-10  0:17     ` Dave Hansen
2017-12-10  6:42     ` Florian Weimer
2017-12-10  6:42       ` Florian Weimer
2017-12-11 16:13       ` Dave Hansen
2017-12-11 16:13         ` Dave Hansen
2017-12-12 23:13         ` Ram Pai
2017-12-12 23:13           ` Ram Pai
2017-12-13  2:14           ` Florian Weimer
2017-12-13  2:14             ` Florian Weimer
2017-12-13 11:35             ` Ram Pai
2017-12-13 11:35               ` Ram Pai
     [not found]               ` <20171213113544.GG5460-LOE2q6NSToAxGrZ80giIafUQ3DHhIser@public.gmane.org>
2017-12-13 15:08                 ` Florian Weimer
2017-12-13 15:08                   ` Florian Weimer
2017-12-13 15:08                   ` Florian Weimer
2017-12-13 15:22                   ` Dave Hansen
2017-12-13 15:22                     ` Dave Hansen
2017-12-13 15:40                     ` Florian Weimer
2017-12-13 15:40                       ` Florian Weimer
2017-12-14  0:17                       ` Ram Pai
2017-12-14  0:17                         ` Ram Pai
2017-12-14 11:21                         ` Florian Weimer
2017-12-16 15:09                           ` Ram Pai
2017-12-16 15:09                             ` Ram Pai
2017-12-16 15:25                             ` Florian Weimer [this message]
2017-12-16 15:25                               ` Florian Weimer
     [not found]                               ` <2eba29f4-804d-b211-1293-52a567739cad-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2017-12-16 17:20                                 ` Ram Pai
2017-12-16 17:20                                   ` Ram Pai
2017-12-16 17:20                                   ` Ram Pai
2017-12-18 11:00                                   ` Florian Weimer
2017-12-18 11:00                                     ` Florian Weimer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2eba29f4-804d-b211-1293-52a567739cad@redhat.com \
    --to=fweimer@redhat.com \
    --cc=dave.hansen@intel.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-x86_64@vger.kernel.org \
    --cc=linuxram@us.ibm.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.