Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1438570 > unrolled thread

[PATCH 6/9] x86, pkeys: add pkey set/get syscalls

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Dave Hansen <dave@sr71.net> - 2016-07-07 14:50 +0200
    Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Mel Gorman <mgorman@techsingularity.net> - 2016-07-07 16:50 +0200
      Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Dave Hansen <dave@sr71.net> - 2016-07-07 19:40 +0200
        Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Ingo Molnar <mingo@kernel.org> - 2016-07-08 09:20 +0200
          Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Dave Hansen <dave@sr71.net> - 2016-07-08 18:40 +0200
            Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Ingo Molnar <mingo@kernel.org> - 2016-07-09 10:40 +0200
              Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Andy Lutomirski <luto@amacapital.net> - 2016-07-11 06:30 +0200
                Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Ingo Molnar <mingo@kernel.org> - 2016-07-11 09:40 +0200
                  Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Dave Hansen <dave@sr71.net> - 2016-07-11 16:30 +0200
                    Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Ingo Molnar <mingo@kernel.org> - 2016-07-12 09:20 +0200
                      Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Dave Hansen <dave@sr71.net> - 2016-07-12 17:40 +0200
                  Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Andy Lutomirski <luto@amacapital.net> - 2016-07-11 17:00 +0200
                Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Dave Hansen <dave@sr71.net> - 2016-07-11 16:40 +0200
                  Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Andy Lutomirski <luto@amacapital.net> - 2016-07-11 16:50 +0200
                    Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Dave Hansen <dave.hansen@intel.com> - 2016-07-11 17:50 +0200
                      Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Andy Lutomirski <luto@amacapital.net> - 2016-07-12 18:40 +0200
                        Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Dave Hansen <dave.hansen@intel.com> - 2016-07-12 19:20 +0200
                          Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Andy Lutomirski <luto@amacapital.net> - 2016-07-13 01:00 +0200
                        Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Ingo Molnar <mingo@kernel.org> - 2016-07-13 10:00 +0200
                          Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Andy Lutomirski <luto@amacapital.net> - 2016-07-13 20:50 +0200
                            Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Ingo Molnar <mingo@kernel.org> - 2016-07-14 10:10 +0200
                              Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Andy Lutomirski <luto@amacapital.net> - 2016-07-18 06:50 +0200
                                Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Ingo Molnar <mingo@kernel.org> - 2016-07-18 12:00 +0200
              Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Dave Hansen <dave@sr71.net> - 2016-07-18 20:10 +0200
              Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Dave Hansen <dave.hansen@intel.com> - 2016-07-18 22:20 +0200
          Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Dave Hansen <dave@sr71.net> - 2016-07-08 21:30 +0200
        Re: [PATCH 6/9] x86, pkeys: add pkey set/get syscalls Mel Gorman <mgorman@techsingularity.net> - 2016-07-08 12:30 +0200

Page 1 of 2  [1] 2  Next page →

#1438570 — [PATCH 6/9] x86, pkeys: add pkey set/get syscalls

From: Dave Hansen <dave.hansen@linux.intel.com>

This establishes two more system calls for protection key management:

	unsigned long pkey_get(int pkey);
	int pkey_set(int pkey, unsigned long access_rights);

The return value from pkey_get() and the 'access_rights' passed
to pkey_set() are the same format: a bitmask containing
PKEY_DENY_WRITE and/or PKEY_DENY_ACCESS, or nothing set at all.

These can replace userspace's direct use of the new rdpkru/wrpkru
instructions.

With current hardware, the kernel can not enforce that it has
control over a given key.  But, this at least allows the kernel
to indicate to userspace that userspace does not control a given
protection key.  This makes it more likely that situations like
using a pkey after sys_pkey_free() can be detected.

The kernel does _not_ enforce that this interface must be used for
changes to PKRU, whether or not a key has been "allocated".

This syscall interface could also theoretically be replaced with a
pair of vsyscalls.  The vsyscalls would just call WRPKRU/RDPKRU
directly in situations where they are drop-in equivalents for
what the kernel would be doing.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: linux-api@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Cc: linux-mm@kvack.org
Cc: x86@kernel.org
Cc: torvalds@linux-foundation.org
Cc: akpm@linux-foundation.org
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: mgorman@techsingularity.net
Cc: hughd@google.com
Cc: viro@zeniv.linux.org.uk
---

 b/arch/x86/entry/syscalls/syscall_32.tbl |    2 +
 b/arch/x86/entry/syscalls/syscall_64.tbl |    2 +
 b/arch/x86/include/asm/pkeys.h           |    4 +-
 b/arch/x86/kernel/fpu/xstate.c           |   55 +++++++++++++++++++++++++++++--
 b/include/linux/pkeys.h                  |    8 ++++
 b/mm/mprotect.c                          |   41 +++++++++++++++++++++++
 6 files changed, 109 insertions(+), 3 deletions(-)

diff -puN arch/x86/entry/syscalls/syscall_32.tbl~pkeys-118-syscalls-set-get arch/x86/entry/syscalls/syscall_32.tbl
--- a/arch/x86/entry/syscalls/syscall_32.tbl~pkeys-118-syscalls-set-get	2016-07-07 05:47:02.197865624 -0700
+++ b/arch/x86/entry/syscalls/syscall_32.tbl	2016-07-07 05:47:02.209866169 -0700
@@ -389,3 +389,5 @@
 380	i386	pkey_mprotect		sys_pkey_mprotect
 381	i386	pkey_alloc		sys_pkey_alloc
 382	i386	pkey_free		sys_pkey_free
+383	i386	pkey_get		sys_pkey_get
+384	i386	pkey_set		sys_pkey_set
diff -puN arch/x86/entry/syscalls/syscall_64.tbl~pkeys-118-syscalls-set-get arch/x86/entry/syscalls/syscall_64.tbl
--- a/arch/x86/entry/syscalls/syscall_64.tbl~pkeys-118-syscalls-set-get	2016-07-07 05:47:02.199865715 -0700
+++ b/arch/x86/entry/syscalls/syscall_64.tbl	2016-07-07 05:47:02.211866259 -0700
@@ -338,6 +338,8 @@
 329	common	pkey_mprotect		sys_pkey_mprotect
 330	common	pkey_alloc		sys_pkey_alloc
 331	common	pkey_free		sys_pkey_free
+332	common	pkey_get		sys_pkey_get
+333	common	pkey_set		sys_pkey_set
 
 #
 # x32-specific system call numbers start at 512 to avoid cache impact
diff -puN arch/x86/include/asm/pkeys.h~pkeys-118-syscalls-set-get arch/x86/include/asm/pkeys.h
--- a/arch/x86/include/asm/pkeys.h~pkeys-118-syscalls-set-get	2016-07-07 05:47:02.201865805 -0700
+++ b/arch/x86/include/asm/pkeys.h	2016-07-07 05:47:02.211866259 -0700
@@ -56,7 +56,7 @@ static inline bool validate_pkey(int pke
 }
 
 static inline
-bool mm_pkey_is_allocated(struct mm_struct *mm, unsigned long pkey)
+bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
 {
 	if (!validate_pkey(pkey))
 		return true;
@@ -107,4 +107,6 @@ extern int arch_set_user_pkey_access(str
 extern int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 		unsigned long init_val);
 
+extern unsigned long arch_get_user_pkey_access(struct task_struct *tsk,
+		int pkey);
 #endif /*_ASM_X86_PKEYS_H */
diff -puN arch/x86/kernel/fpu/xstate.c~pkeys-118-syscalls-set-get arch/x86/kernel/fpu/xstate.c
--- a/arch/x86/kernel/fpu/xstate.c~pkeys-118-syscalls-set-get	2016-07-07 05:47:02.203865896 -0700
+++ b/arch/x86/kernel/fpu/xstate.c	2016-07-07 05:47:02.211866259 -0700
@@ -708,7 +708,7 @@ void fpu__resume_cpu(void)
  *
  * Note: does not work for compacted buffers.
  */
-void *__raw_xsave_addr(struct xregs_state *xsave, int xstate_feature_mask)
+static void *__raw_xsave_addr(struct xregs_state *xsave, int xstate_feature_mask)
 {
 	int feature_nr = fls64(xstate_feature_mask) - 1;
 
@@ -882,6 +882,7 @@ out:
 
 #define NR_VALID_PKRU_BITS (CONFIG_NR_PROTECTION_KEYS * 2)
 #define PKRU_VALID_MASK (NR_VALID_PKRU_BITS - 1)
+#define PKRU_INIT_STATE	0
 
 /*
  * This will go out and modify the XSAVE buffer so that PKRU is
@@ -900,6 +901,9 @@ int __arch_set_user_pkey_access(struct t
 	int pkey_shift = (pkey * PKRU_BITS_PER_PKEY);
 	u32 new_pkru_bits = 0;
 
+	/* Only support manipulating current task for now */
+	if (tsk != current)
+		return -EINVAL;
 	/*
 	 * This check implies XSAVE support.  OSPKE only gets
 	 * set if we enable XSAVE and we enable PKU in XCR0.
@@ -925,7 +929,7 @@ int __arch_set_user_pkey_access(struct t
 	 * state.
 	 */
 	if (!old_pkru_state)
-		new_pkru_state.pkru = 0;
+		new_pkru_state.pkru = PKRU_INIT_STATE;
 	else
 		new_pkru_state.pkru = old_pkru_state->pkru;
 
@@ -963,4 +967,51 @@ int arch_set_user_pkey_access(struct tas
 		return -EINVAL;
 	return __arch_set_user_pkey_access(tsk, pkey, init_val);
 }
+
+/*
+ * Figures out what the rights are currently for 'pkey'.
+ * Converts from PKRU's format to the user-visible PKEY_DISABLE_*
+ * format.
+ */
+unsigned long arch_get_user_pkey_access(struct task_struct *tsk, int pkey)
+{
+	struct fpu *fpu = &current->thread.fpu;
+	u32 pkru_reg;
+	int ret = 0;
+
+	/* Only support manipulating current task for now */
+	if (tsk != current)
+		return -1;
+	if (!cpu_feature_enabled(X86_FEATURE_OSPKE))
+		return -1;
+	/*
+	 * The contents of PKRU itself are invalid.  Consult the
+	 * task's XSAVE buffer for PKRU contents.  This is much
+	 * more expensive than reading PKRU directly, but should
+	 * be rare or impossible with eagerfpu mode.
+	 */
+	if (!fpu->fpregs_active) {
+		struct xregs_state *xsave = &fpu->state.xsave;
+		struct pkru_state *pkru_state =
+			get_xsave_addr(xsave, XFEATURE_MASK_PKRU);
+		/*
+		 * PKRU is in its init state and not present in
+		 * the buffer in a saved form.
+		 */
+		if (!pkru_state)
+			return PKRU_INIT_STATE;
+
+		return pkru_state->pkru;
+	}
+	/*
+	 * Consult the user register directly.
+	 */
+	pkru_reg = read_pkru();
+	if (!__pkru_allows_read(pkru_reg, pkey))
+		ret |= PKEY_DISABLE_ACCESS;
+	if (!__pkru_allows_write(pkru_reg, pkey))
+		ret |= PKEY_DISABLE_WRITE;
+
+	return ret;
+}
 #endif /* CONFIG_ARCH_HAS_PKEYS */
diff -puN include/linux/pkeys.h~pkeys-118-syscalls-set-get include/linux/pkeys.h
--- a/include/linux/pkeys.h~pkeys-118-syscalls-set-get	2016-07-07 05:47:02.204865942 -0700
+++ b/include/linux/pkeys.h	2016-07-07 05:47:02.212866305 -0700
@@ -44,6 +44,14 @@ static inline int mm_pkey_free(struct mm
 static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 			unsigned long init_val)
 {
+	return -EINVAL;
+}
+
+static inline
+unsigned long arch_get_user_pkey_access(struct task_struct *tsk, int pkey)
+{
+	if (pkey)
+		return -1;
 	return 0;
 }
 
diff -puN mm/mprotect.c~pkeys-118-syscalls-set-get mm/mprotect.c
--- a/mm/mprotect.c~pkeys-118-syscalls-set-get	2016-07-07 05:47:02.206866032 -0700
+++ b/mm/mprotect.c	2016-07-07 05:47:02.212866305 -0700
@@ -537,3 +537,44 @@ SYSCALL_DEFINE1(pkey_free, int, pkey)
 	 */
 	return ret;
 }
+
+SYSCALL_DEFINE2(pkey_get, int, pkey, unsigned long, flags)
+{
+	unsigned long ret = 0;
+
+	if (flags)
+		return -EINVAL;
+
+	down_write(&current->mm->mmap_sem);
+	if (!mm_pkey_is_allocated(current->mm, pkey))
+		ret = -EBADF;
+	up_write(&current->mm->mmap_sem);
+
+	if (ret)
+		return ret;
+
+	ret = arch_get_user_pkey_access(current, pkey);
+
+	return ret;
+}
+
+SYSCALL_DEFINE3(pkey_set, int, pkey, unsigned long, access_rights,
+		unsigned long, flags)
+{
+	unsigned long ret = 0;
+
+	if (flags)
+		return -EINVAL;
+
+	down_write(&current->mm->mmap_sem);
+	if (!mm_pkey_is_allocated(current->mm, pkey))
+		ret = -EBADF;
+	up_write(&current->mm->mmap_sem);
+
+	if (ret)
+		return ret;
+
+	ret = arch_set_user_pkey_access(current, pkey, access_rights);
+
+	return ret;
+}
_

[toc] | [next] | [standalone]

#1438659

On Thu, Jul 07, 2016 at 05:47:28AM -0700, Dave Hansen wrote:
> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> This establishes two more system calls for protection key management:
> 
> 	unsigned long pkey_get(int pkey);
> 	int pkey_set(int pkey, unsigned long access_rights);
> 
> The return value from pkey_get() and the 'access_rights' passed
> to pkey_set() are the same format: a bitmask containing
> PKEY_DENY_WRITE and/or PKEY_DENY_ACCESS, or nothing set at all.
> 
> These can replace userspace's direct use of the new rdpkru/wrpkru
> instructions.
> 
> With current hardware, the kernel can not enforce that it has
> control over a given key.  But, this at least allows the kernel
> to indicate to userspace that userspace does not control a given
> protection key.  This makes it more likely that situations like
> using a pkey after sys_pkey_free() can be detected.
> 
> The kernel does _not_ enforce that this interface must be used for
> changes to PKRU, whether or not a key has been "allocated".
> 
> This syscall interface could also theoretically be replaced with a
> pair of vsyscalls.  The vsyscalls would just call WRPKRU/RDPKRU
> directly in situations where they are drop-in equivalents for
> what the kernel would be doing.
> 

This one feels like something that can or should be implemented in
glibc.

There is no real enforcement of the values yet looking them up or
setting them takes mmap_sem for write. Applications that frequently get
called will get hammed into the ground with serialisation on mmap_sem
not to mention the cost of the syscall entry/exit.

RIght now, I'm seeing a lot of cost and not much benefit with this
specific patch.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1438772

On 07/07/2016 07:45 AM, Mel Gorman wrote:
> On Thu, Jul 07, 2016 at 05:47:28AM -0700, Dave Hansen wrote:
>> > 
>> > From: Dave Hansen <dave.hansen@linux.intel.com>
>> > 
>> > This establishes two more system calls for protection key management:
>> > 
>> > 	unsigned long pkey_get(int pkey);
>> > 	int pkey_set(int pkey, unsigned long access_rights);
>> > 
>> > The return value from pkey_get() and the 'access_rights' passed
>> > to pkey_set() are the same format: a bitmask containing
>> > PKEY_DENY_WRITE and/or PKEY_DENY_ACCESS, or nothing set at all.
>> > 
>> > These can replace userspace's direct use of the new rdpkru/wrpkru
>> > instructions.
...
> This one feels like something that can or should be implemented in
> glibc.

I generally agree, except that glibc doesn't have any visibility into
whether a pkey is currently valid or not.

> There is no real enforcement of the values yet looking them up or
> setting them takes mmap_sem for write.

There are checks for mm_pkey_is_allocated().  That's the main thing
these syscalls add on top of the raw instructions.

> Applications that frequently get
> called will get hammed into the ground with serialisation on mmap_sem
> not to mention the cost of the syscall entry/exit.

I think we can do both of them without mmap_sem, as long as we resign
ourselves to this just being fundamentally racy (which it is already, I
think).  But, is it worth performance-tuning things that we don't expect
performance-sensitive apps to be using in the first place?  They'll just
use the RDPKRU/WRPKRU instructions directly.

Ingo, do you still feel strongly that these syscalls (pkey_set/get())
should be included?  Of the 5, they're definitely the two with the
weakest justification.

[toc] | [prev] | [next] | [standalone]

#1439145

* Dave Hansen <dave@sr71.net> wrote:

> > Applications that frequently get called will get hammed into the ground with 
> > serialisation on mmap_sem not to mention the cost of the syscall entry/exit.
>
> I think we can do both of them without mmap_sem, as long as we resign ourselves 
> to this just being fundamentally racy (which it is already, I think).  But, is 
> it worth performance-tuning things that we don't expect performance-sensitive 
> apps to be using in the first place?  They'll just use the RDPKRU/WRPKRU 
> instructions directly.
>
> Ingo, do you still feel strongly that these syscalls (pkey_set/get()) should be 
> included?  Of the 5, they're definitely the two with the weakest justification.

Firstly, I'd like to thank Mel for the review, having this kind of high level 
interface discussion was exactly what I was hoping for before we merged any ABI 
patches.

So my hope was that we'd also grow some debugging features: such as a periodic 
watchdog timer clearing all non-allocated pkeys of a task and re-setting them to 
their (kernel-)known values and thus forcing user-space to coordinate key 
allocation/freeing.

While allocation/freeing of keys is very likely a slow path in any reasonable 
workload, _setting_ the values of pkeys could easily be a fast path. The whole 
point of pkeys is to allow both thread local and dynamic mapping and unmapping of 
memory ranges, without having to touch any page table attributes in the hot path.

Now another, more fundamental question is that pkeys are per-CPU (per thread) on 
the hardware side - so why do we even care about the mmap_sem in the syscalls in 
the first place? If we want any serialization wouldn't a pair of 
get_cpu()/put_cpu() preemption control calls be enough? Those would also be much 
cheaper.

The idea behind my suggestion to manage all pkey details via a kernel interface 
and 'shadow' the pkeys state in the kernel was that if we don't even offer a 
complete system call interface then user-space is _forced_ into using the raw 
instructions and into implementing a (potentially crappy) uncoordinated API or not 
implementing any API at all but randomly using fixed pkey indices.

My hope was to avoid that, especially since currently _all_ memory mapping details 
on x86 are controlled via system calls. If we don't offer that kind of facility 
then we force user-space into using the raw instructions and will likely end up 
with a poor user-space interface.

So the question is, what is user-space going to do? Do any glibc patches exist? 
How are the user-space library side APIs going to look like?

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1439653

On 07/08/2016 12:18 AM, Ingo Molnar wrote:
> So the question is, what is user-space going to do? Do any glibc
> patches exist? How are the user-space library side APIs going to look
> like?

My goal at the moment is to get folks enabled to the point that they can
start modifying apps to use pkeys without having to patch their kernels.
 I don't have confidence that we can design good high-level userspace
interfaces without seeing some real apps try to use the low-level ones
and seeing how they struggle.

I had some glibc code to do the pkey alloc/free operations, but those
aren't necessary if we're doing it in the kernel.  Other than getting
the syscall wrappers in place, I don't have any immediate plans to do
anything in glibc.

Was there something you were expecting to see?

[toc] | [prev] | [next] | [standalone]

#1439915

* Dave Hansen <dave@sr71.net> wrote:

> On 07/08/2016 12:18 AM, Ingo Molnar wrote:
>
> > So the question is, what is user-space going to do? Do any glibc patches 
> > exist? How are the user-space library side APIs going to look like?
> 
> My goal at the moment is to get folks enabled to the point that they can start 
> modifying apps to use pkeys without having to patch their kernels.
>  I don't have confidence that we can design good high-level userspace interfaces 
> without seeing some real apps try to use the low-level ones and seeing how they 
> struggle.
> 
> I had some glibc code to do the pkey alloc/free operations, but those aren't 
> necessary if we're doing it in the kernel.  Other than getting the syscall 
> wrappers in place, I don't have any immediate plans to do anything in glibc.
> 
> Was there something you were expecting to see?

Yeah, so (as you probably guessed!) I'm starting to have second thoughts about the 
complexity of the alloc/free/set/get interface I suggested, and Mel's review 
certainly strengthened that feeling.

I have two worries:

1)

A technical worry I have is that the 'pkey allocation interface' does not seem to 
be taking the per thread property of pkeys into account - while that property 
would be useful for apps. That is a limitation that seems unjustified.

The reason for this is that we are storing the key allocation bitmap in struct_mm, 
in mm->context.pkey_allocation_map - while we should be storing it in task_struct 
or thread_info.

We could solve this by moving the allocation bitmap to the task struct, but:

2)

My main worry is that it appears at this stage that we are still pretty far away 
from completely shadowing the hardware pkey state in the kernel - and without that 
we cannot really force user-space to use the 'proper' APIs. They can just use the 
raw instructions, condition them on a CPUID and be done with it: everything can be 
organized in user-space.

Furthermore, implementing it in a high performance fashion would be pretty complex 
- at minimum we'd have to register a per thread read-write user-space data area 
where the kernel could store pkeys management data so that vsyscalls can access it 
... None of that facility exists today.

And without vsyscall optimizations user-space might legitimately use its own 
implementation for performance reasons and we'd end up with twice the complexity 
and a largely unused piece of kernel infrastructure ...

So how about the following minimalistic approach instead, to get the ball rolling 
without making ABI decisions we might regret:

 - There are 16 pkey indices on x86 currently. We already use index 15 for the 
   true PROT_EXEC implementation. Let's set aside another pkey index for the 
   kernel's potential future use (index 14), and clear it explicitly in the 
   FPU context on every context switch if CONFIG_X86_DEBUG_FPU is enabled to make 
   sure it remains unallocated.

 - Expose just the new mprotect_pkey() system call to install a pkey index into 
   the page tables - but we let user-space organize its key allocations.

 - Give user-space an idea about limits:

     "ALL THESE WORLDS ARE YOURS—EXCEPT EUROPA ATTEMPT NO LANDING THERE"

   Ooops, wrong one. Lets try this instead:

     Expose the current maximum user-space usable pkeys index in some
     programmatically accessible fashion. Maybe mprotect_pkey() could reject a 
     permanently allocated kernel pkey index via a distinctive error code?

   I.e. this pattern:

     ret = pkey_mprotect(NULL, PAGE_SIZE, real_prot, pkey);

   ... would validate the pkey and we'd return -EOPNOTSUPP for pkey that is not 
   available? This would allow maximum future flexibility as it would not define 
   kernel allocated pkeys as a 'range'.

 - ... and otherwise leave the remaining 14 pkey indices for user-space to manage.

If in the future user-space pkeys usage grows to such a level that kernel 
arbitration becomes desirable then we can still implement the get/set/alloc/free 
system calls as well: the first use of those system calls would switch on the 
kernel's pkey management facilities and from that point on user-space is supposed 
to use the published system calls only. Applications using pkey instructions 
directly would still work just fine: they'd never use the new system calls.

I.e. we can actually keep a bigger ABI flexibility by introducing the simplest 
possible ABI at this stage. Maybe user-space usage of this hardware feature will 
never grow beyond that simple ABI - in which case we've saved quite a bit of 
ongoing maintenance complexity...

And yes, I realize that we've come a full round since the very first version of 
this patch set, but I think the extra hoops were still worth it, because the 
true-PROT_EXEC feature came out of it which is very useful IMHO. But my more 
complex pkey management syscall ideas don't seem to be all that useful anymore.

So what do you think about this direction? This would simplify the patch set quite 
a bit and would touch very little MM code beyond the mprotect_pkey() bits.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1440241

On Jul 9, 2016 1:37 AM, "Ingo Molnar" <mingo@kernel.org> wrote:
>
>
> * Dave Hansen <dave@sr71.net> wrote:
>
> > On 07/08/2016 12:18 AM, Ingo Molnar wrote:
> >
> > > So the question is, what is user-space going to do? Do any glibc patches
> > > exist? How are the user-space library side APIs going to look like?
> >
> > My goal at the moment is to get folks enabled to the point that they can start
> > modifying apps to use pkeys without having to patch their kernels.
> >  I don't have confidence that we can design good high-level userspace interfaces
> > without seeing some real apps try to use the low-level ones and seeing how they
> > struggle.
> >
> > I had some glibc code to do the pkey alloc/free operations, but those aren't
> > necessary if we're doing it in the kernel.  Other than getting the syscall
> > wrappers in place, I don't have any immediate plans to do anything in glibc.
> >
> > Was there something you were expecting to see?
>
> Yeah, so (as you probably guessed!) I'm starting to have second thoughts about the
> complexity of the alloc/free/set/get interface I suggested, and Mel's review
> certainly strengthened that feeling.
>
> I have two worries:
>
> 1)
>
> A technical worry I have is that the 'pkey allocation interface' does not seem to
> be taking the per thread property of pkeys into account - while that property
> would be useful for apps. That is a limitation that seems unjustified.
>
> The reason for this is that we are storing the key allocation bitmap in struct_mm,
> in mm->context.pkey_allocation_map - while we should be storing it in task_struct
> or thread_info.

Huh?  Doesn't this have to be per mm?  Sure, PKRU is per thread, but
the page tables are shared.

> 2)
>
> My main worry is that it appears at this stage that we are still pretty far away
> from completely shadowing the hardware pkey state in the kernel - and without that
> we cannot really force user-space to use the 'proper' APIs. They can just use the
> raw instructions, condition them on a CPUID and be done with it: everything can be
> organized in user-space.
>

My vote would be to keep the allocation mechanism but get rid of pkey_set.

Also, I think the debug poisoning feature is overcomplicated.  Let's
just forbid mprotect_key with an unallocated key.

There are still two issues that I think we need to address, though:

1. Signal delivery shouldn't unconditionally clear PKRU.  That's what
the current patches do, and it's unsafe.  I'd rather set PKRU to the
maximally locked down state on signal delivery (except for the
PROT_EXEC key), although that might cause its own set of problems.

2. When thread A allocates a pkey, how does it lock down thread B?

#2 could be addressed by using fully-locked-down as the initial state
post-exec() and copying the state on clone().  Dave, are there any
cases in practice where one thread would allocate a pkey and want
other threads to immediately have access to the memory with that key?

I find myself wondering whether we should stop using XSAVE for PKRU
sooner rather than later.  If we do anything like the above, we
completely lose the init optimization, and the code would be a good
deal simpler if we switched PKRU directly in switch_to and could
therefore treat it like a normal register everywhere else.

--Andy

[toc] | [prev] | [next] | [standalone]

#1440320

* Andy Lutomirski <luto@amacapital.net> wrote:

> On Jul 9, 2016 1:37 AM, "Ingo Molnar" <mingo@kernel.org> wrote:
> >
> >
> > * Dave Hansen <dave@sr71.net> wrote:
> >
> > > On 07/08/2016 12:18 AM, Ingo Molnar wrote:
> > >
> > > > So the question is, what is user-space going to do? Do any glibc patches
> > > > exist? How are the user-space library side APIs going to look like?
> > >
> > > My goal at the moment is to get folks enabled to the point that they can start
> > > modifying apps to use pkeys without having to patch their kernels.
> > >  I don't have confidence that we can design good high-level userspace interfaces
> > > without seeing some real apps try to use the low-level ones and seeing how they
> > > struggle.
> > >
> > > I had some glibc code to do the pkey alloc/free operations, but those aren't
> > > necessary if we're doing it in the kernel.  Other than getting the syscall
> > > wrappers in place, I don't have any immediate plans to do anything in glibc.
> > >
> > > Was there something you were expecting to see?
> >
> > Yeah, so (as you probably guessed!) I'm starting to have second thoughts about the
> > complexity of the alloc/free/set/get interface I suggested, and Mel's review
> > certainly strengthened that feeling.
> >
> > I have two worries:
> >
> > 1)
> >
> > A technical worry I have is that the 'pkey allocation interface' does not seem to
> > be taking the per thread property of pkeys into account - while that property
> > would be useful for apps. That is a limitation that seems unjustified.
> >
> > The reason for this is that we are storing the key allocation bitmap in struct_mm,
> > in mm->context.pkey_allocation_map - while we should be storing it in task_struct
> > or thread_info.
> 
> Huh?  Doesn't this have to be per mm?  Sure, PKRU is per thread, but
> the page tables are shared.

But the keys are not shared, and they carry meaningful per thread information.

mprotect_pkey()'s effects are per MM, but the system calls related to managing the 
keys (alloc/free/get/set) are fundamentally per CPU.

Here's an example of how this could matter to applications:

 - 'writer thread' gets a RW- key into index 1 to a specific data area
 - a pool of 'reader threads' may get the same pkey index 1 R-- to read the data 
   area.

Same page tables, same index, two protections and two purposes.

With a global, per MM allocation of keys we'd have to use two indices: index 1 and 2.

Depending on how scarce the index space turns out to be making the key indices per 
thread is probably the right model.

> There are still two issues that I think we need to address, though:
> 
> 1. Signal delivery shouldn't unconditionally clear PKRU.  That's what
> the current patches do, and it's unsafe.  I'd rather set PKRU to the
> maximally locked down state on signal delivery (except for the
> PROT_EXEC key), although that might cause its own set of problems.

Right now the historic pattern for signal handlers is that they safely and 
transparently stack on top of existing FPU related resources and do a save/restore 
of them. In that sense saving+clearing+restoring the pkeys state would be the 
correct approach that follows that pattern. There are two extra considerations:

- If we think of pkeys as a temporary register that can be used to access/unaccess 
  normally unaccessible memory regions then this makes sense, in fact it's more 
  secure: signal handlers cannot accidentally stomp on an encryption key or on a
  database area, unless they intentionally gain access to them.

- If we think of pkeys as permanent memory mappings that enhance existing MM
  permissions then it would be correct to let them leak into signal handler state. 
  The globl true-PROT_EXEC key would fall into this category.

So I agree, mostly: the correct approach is to save+clear+restore the first 14 
pkey indices, and to leave alone the two 'global' indices.

> 2. When thread A allocates a pkey, how does it lock down thread B?

So see above, I think the temporary key space should be per thread, so there would 
be no inter thread interactions: each thread is responsible for its own key 
management (via per thread management data in the library that implments it).

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1440652

On 07/11/2016 12:35 AM, Ingo Molnar wrote:
> * Andy Lutomirski <luto@amacapital.net> wrote:
> mprotect_pkey()'s effects are per MM, but the system calls related to managing the 
> keys (alloc/free/get/set) are fundamentally per CPU.
> 
> Here's an example of how this could matter to applications:
> 
>  - 'writer thread' gets a RW- key into index 1 to a specific data area
>  - a pool of 'reader threads' may get the same pkey index 1 R-- to read the data 
>    area.
> 
> Same page tables, same index, two protections and two purposes.
> 
> With a global, per MM allocation of keys we'd have to use two indices: index 1 and 2.

I'm not sure how this would work.  A piece of data mapped at only one
virtual address can have only one key associated with it.  For a data
area, you would need to indicate between threads which key they needed
in order to access the data.  Both threads need to agree on the virtual
address *and* the key used for access.

Remember, PKRU is just a *bitmap*.  The only place keys are stored is in
the page tables.

Here's how this ends up looking in practice when we have an initializer,
a reader and a writer:

	/* allocator: */
	pkey = pkey_alloc();
	data = mmap(PAGE_SIZE, PROT_NONE, ...);
	pkey_mprotect(data, PROT_WRITE|PROT_READ, pkey);
	metadata[data].pkey = pkey;

	/* reader */
	pkey_set(metadata[data].pkey, PKEY_DENY_WRITE);
	readerfoo = *data;
	pkey_set(metadata[data].pkey, PKEY_DENY_WRITE|ACCESS);

	/* writer */
	pkey_set(metadata[data].pkey, 0); /* 0 == deny nothing */
	*data = bar;
	pkey_set(metadata[data].pkey, PKEY_DENY_WRITE|ACCESS);


I'm also not sure what the indexes are that you're referring to.

> Depending on how scarce the index space turns out to be making the key indices per 
> thread is probably the right model.

Yeah, I'm totally confused about what you mean by indexes.

>> There are still two issues that I think we need to address, though:
>>
>> 1. Signal delivery shouldn't unconditionally clear PKRU.  That's what
>> the current patches do, and it's unsafe.  I'd rather set PKRU to the
>> maximally locked down state on signal delivery (except for the
>> PROT_EXEC key), although that might cause its own set of problems.
> 
> Right now the historic pattern for signal handlers is that they safely and 
> transparently stack on top of existing FPU related resources and do a save/restore 
> of them. In that sense saving+clearing+restoring the pkeys state would be the 
> correct approach that follows that pattern. There are two extra considerations:
> 
> - If we think of pkeys as a temporary register that can be used to access/unaccess 
>   normally unaccessible memory regions then this makes sense, in fact it's more 
>   secure: signal handlers cannot accidentally stomp on an encryption key or on a
>   database area, unless they intentionally gain access to them.
> 
> - If we think of pkeys as permanent memory mappings that enhance existing MM
>   permissions then it would be correct to let them leak into signal handler state. 
>   The globl true-PROT_EXEC key would fall into this category.
> 
> So I agree, mostly: the correct approach is to save+clear+restore the first 14 
> pkey indices, and to leave alone the two 'global' indices.

The current scheme is the most permissive, but it has an important
property: it's the most _flexible_.  You can implement almost any scheme
you want in userspace on top of it.  The first userspace instruction of
the handler could easily be WRKRU to fully lock down access in whatever
scheme a program wants.

[toc] | [prev] | [next] | [standalone]

#1441104

* Dave Hansen <dave@sr71.net> wrote:

> On 07/11/2016 12:35 AM, Ingo Molnar wrote:
> > * Andy Lutomirski <luto@amacapital.net> wrote:
> > mprotect_pkey()'s effects are per MM, but the system calls related to managing the 
> > keys (alloc/free/get/set) are fundamentally per CPU.
> > 
> > Here's an example of how this could matter to applications:
> > 
> >  - 'writer thread' gets a RW- key into index 1 to a specific data area
> >  - a pool of 'reader threads' may get the same pkey index 1 R-- to read the data 
> >    area.
> > 
> > Same page tables, same index, two protections and two purposes.
> > 
> > With a global, per MM allocation of keys we'd have to use two indices: index 1 and 2.
> 
> I'm not sure how this would work.  A piece of data mapped at only one virtual 
> address can have only one key associated with it.

Yeah, indeed, got myself confused there - but the actual protection bits are per 
CPU (per task).

> Remember, PKRU is just a *bitmap*.  The only place keys are stored is in the 
> page tables.

A pkey is an index *and* a protection mask. So by representing it as a bitmask we 
lose per thread information. This is what I meant by 'incomplete shadowing' - for 
example the debug code couldn't work: if we cleared a pkey in a task we wouldn't 
know what to restore it to with the current data structures, right?

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1441512

On 07/12/2016 12:13 AM, Ingo Molnar wrote:
>> > Remember, PKRU is just a *bitmap*.  The only place keys are stored is in the 
>> > page tables.
> A pkey is an index *and* a protection mask. So by representing it as a bitmask we 
> lose per thread information. This is what I meant by 'incomplete shadowing' - for 
> example the debug code couldn't work: if we cleared a pkey in a task we wouldn't 
> know what to restore it to with the current data structures, right?

Right.  I actually have some code to do the shadowing that I wrote to
explore how to do different PKRU values in signal handlers.  The code
only shadowed the keys that were currently allocated, and used the
(mm-wide) allocation map to figure that out.  It did not have a separate
per-thread concept of which parts of PKRU need to be shadowed.

It essentially populated the shadow value on all pkru_set() calls.

[toc] | [prev] | [next] | [standalone]

#1440694

On Mon, Jul 11, 2016 at 12:35 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Andy Lutomirski <luto@amacapital.net> wrote:
>
>> On Jul 9, 2016 1:37 AM, "Ingo Molnar" <mingo@kernel.org> wrote:
>> >
>> >
>> > * Dave Hansen <dave@sr71.net> wrote:
>> >
>> > > On 07/08/2016 12:18 AM, Ingo Molnar wrote:
>> > >
>> > > > So the question is, what is user-space going to do? Do any glibc patches
>> > > > exist? How are the user-space library side APIs going to look like?
>> > >
>> > > My goal at the moment is to get folks enabled to the point that they can start
>> > > modifying apps to use pkeys without having to patch their kernels.
>> > >  I don't have confidence that we can design good high-level userspace interfaces
>> > > without seeing some real apps try to use the low-level ones and seeing how they
>> > > struggle.
>> > >
>> > > I had some glibc code to do the pkey alloc/free operations, but those aren't
>> > > necessary if we're doing it in the kernel.  Other than getting the syscall
>> > > wrappers in place, I don't have any immediate plans to do anything in glibc.
>> > >
>> > > Was there something you were expecting to see?
>> >
>> > Yeah, so (as you probably guessed!) I'm starting to have second thoughts about the
>> > complexity of the alloc/free/set/get interface I suggested, and Mel's review
>> > certainly strengthened that feeling.
>> >
>> > I have two worries:
>> >
>> > 1)
>> >
>> > A technical worry I have is that the 'pkey allocation interface' does not seem to
>> > be taking the per thread property of pkeys into account - while that property
>> > would be useful for apps. That is a limitation that seems unjustified.
>> >
>> > The reason for this is that we are storing the key allocation bitmap in struct_mm,
>> > in mm->context.pkey_allocation_map - while we should be storing it in task_struct
>> > or thread_info.
>>
>> Huh?  Doesn't this have to be per mm?  Sure, PKRU is per thread, but
>> the page tables are shared.
>
> But the keys are not shared, and they carry meaningful per thread information.
>
> mprotect_pkey()'s effects are per MM, but the system calls related to managing the
> keys (alloc/free/get/set) are fundamentally per CPU.
>
> Here's an example of how this could matter to applications:
>
>  - 'writer thread' gets a RW- key into index 1 to a specific data area
>  - a pool of 'reader threads' may get the same pkey index 1 R-- to read the data
>    area.

Sure, but this means you allocate index 1 once and then use it in both
threads.  If you allocate separately in each thread, nothing
guarantees you'll get the same index both times, and if you don't then
the code doesn't work.

>
>> There are still two issues that I think we need to address, though:
>>
>> 1. Signal delivery shouldn't unconditionally clear PKRU.  That's what
>> the current patches do, and it's unsafe.  I'd rather set PKRU to the
>> maximally locked down state on signal delivery (except for the
>> PROT_EXEC key), although that might cause its own set of problems.
>
> Right now the historic pattern for signal handlers is that they safely and
> transparently stack on top of existing FPU related resources and do a save/restore
> of them. In that sense saving+clearing+restoring the pkeys state would be the
> correct approach that follows that pattern. There are two extra considerations:
>
> - If we think of pkeys as a temporary register that can be used to access/unaccess
>   normally unaccessible memory regions then this makes sense, in fact it's more
>   secure: signal handlers cannot accidentally stomp on an encryption key or on a
>   database area, unless they intentionally gain access to them.
>

That how I think I would think of them, but for this to be fully safe,
we'd want to lock them down in signal handlers by default, which is
what I'm suggesting.

--Andy

[toc] | [prev] | [next] | [standalone]

#1440664

On 07/10/2016 09:25 PM, Andy Lutomirski wrote:
> 2. When thread A allocates a pkey, how does it lock down thread B?
> 
> #2 could be addressed by using fully-locked-down as the initial state
> post-exec() and copying the state on clone().  Dave, are there any
> cases in practice where one thread would allocate a pkey and want
> other threads to immediately have access to the memory with that key?

The only one I can think of is a model where pkeys are used more in a
"denial" mode rather than an "allow" mode.

For instance, perhaps you don't want to modify your app to use pkeys,
except for a small routine where you handle untrusted user data.  You
would, in that routine, deny access to a bunch of keys, but otherwise
allow access to all so you didn't have to change any other parts of the app.

Should we instead just recommend to userspace that they lock down access
to keys by default in all threads as a best practice?

[toc] | [prev] | [next] | [standalone]

#1440672

On Mon, Jul 11, 2016 at 7:34 AM, Dave Hansen <dave@sr71.net> wrote:
> On 07/10/2016 09:25 PM, Andy Lutomirski wrote:
>> 2. When thread A allocates a pkey, how does it lock down thread B?
>>
>> #2 could be addressed by using fully-locked-down as the initial state
>> post-exec() and copying the state on clone().  Dave, are there any
>> cases in practice where one thread would allocate a pkey and want
>> other threads to immediately have access to the memory with that key?
>
> The only one I can think of is a model where pkeys are used more in a
> "denial" mode rather than an "allow" mode.
>
> For instance, perhaps you don't want to modify your app to use pkeys,
> except for a small routine where you handle untrusted user data.  You
> would, in that routine, deny access to a bunch of keys, but otherwise
> allow access to all so you didn't have to change any other parts of the app.
>
> Should we instead just recommend to userspace that they lock down access
> to keys by default in all threads as a best practice?

Is that really better than doing it in-kernel?  My concern is that
we'll find library code that creates a thread, and that code could run
before the pkey-aware part of the program even starts running.  So how
is user code supposed lock down all of its threads?

seccomp has TSYNC for this, but I don't think that PKRU allows
something like that.

[toc] | [prev] | [next] | [standalone]

#1440725

On 07/11/2016 07:45 AM, Andy Lutomirski wrote:
> On Mon, Jul 11, 2016 at 7:34 AM, Dave Hansen <dave@sr71.net> wrote:
>> Should we instead just recommend to userspace that they lock down access
>> to keys by default in all threads as a best practice?
> 
> Is that really better than doing it in-kernel?  My concern is that
> we'll find library code that creates a thread, and that code could run
> before the pkey-aware part of the program even starts running. 

Yeah, so let's assume we have some pkey-unaware thread.  The upside of a
scheme where the kernel preemptively (and transparently to the thread)
locks down PKRU is that the thread can't go corrupting any non-zero-pkey
structures that came from other threads.

But, the downside is that the thread can not access any non-zero-pkey
structures without taking some kind of action with PKRU.  That obviously
won't happen since the thread is pkeys-unaware to begin with.  Would
that break these libraries unless everything using pkeys knows to only
share pkey=0 data with those threads?

> So how is user code supposed lock down all of its threads?
> 
> seccomp has TSYNC for this, but I don't think that PKRU allows 
> something like that.

I'm not sure this is possible for PKRU.  Think of a simple PKRU
manipulation in userspace:

	pkru = rdpkru();
	pkru |= PKEY_DENY_ACCESS<<key*2;
	wrpkru(pkru);

If we push a PKRU value into a thread between the rdpkru() and wrpkru(),
we'll lose the content of that "push".  I'm not sure there's any way to
guarantee this with a user-controlled register.

[toc] | [prev] | [next] | [standalone]

#1441547

On Jul 11, 2016 8:48 AM, "Dave Hansen" <dave.hansen@intel.com> wrote:
>
> On 07/11/2016 07:45 AM, Andy Lutomirski wrote:
> > On Mon, Jul 11, 2016 at 7:34 AM, Dave Hansen <dave@sr71.net> wrote:
> >> Should we instead just recommend to userspace that they lock down access
> >> to keys by default in all threads as a best practice?
> >
> > Is that really better than doing it in-kernel?  My concern is that
> > we'll find library code that creates a thread, and that code could run
> > before the pkey-aware part of the program even starts running.
>
> Yeah, so let's assume we have some pkey-unaware thread.  The upside of a
> scheme where the kernel preemptively (and transparently to the thread)
> locks down PKRU is that the thread can't go corrupting any non-zero-pkey
> structures that came from other threads.
>
> But, the downside is that the thread can not access any non-zero-pkey
> structures without taking some kind of action with PKRU.  That obviously
> won't happen since the thread is pkeys-unaware to begin with.  Would
> that break these libraries unless everything using pkeys knows to only
> share pkey=0 data with those threads?
>

Yes, but at least for the cases I can think of, that's probably a good
thing.  OTOH, I can see cases where you want everyone to be able to
read but only specific code paths to be able to write.

I think it's more or less impossible to get sensible behavior passing
pkey != 0 data to legacy functions.  If you call:

void frob(struct foo *p);

If frob in turn passes p to a thread, what PKRU is it supposed to use?

> > So how is user code supposed lock down all of its threads?
> >
> > seccomp has TSYNC for this, but I don't think that PKRU allows
> > something like that.
>
> I'm not sure this is possible for PKRU.  Think of a simple PKRU
> manipulation in userspace:
>
>         pkru = rdpkru();
>         pkru |= PKEY_DENY_ACCESS<<key*2;
>         wrpkru(pkru);
>
> If we push a PKRU value into a thread between the rdpkru() and wrpkru(),
> we'll lose the content of that "push".  I'm not sure there's any way to
> guarantee this with a user-controlled register.

We could try to insist that user code uses some vsyscall helper that
tracks which bits are as-yet-unassigned.  That's quite messy, though.

We could also arbitrarily partition the key space into
initially-wide-open, initially-read-only, and initially-no-access and
let pkey_alloc say which kind it wants.

--Andy

[toc] | [prev] | [next] | [standalone]

#1441607

On 07/12/2016 09:32 AM, Andy Lutomirski wrote:
> I think it's more or less impossible to get sensible behavior passing
> pkey != 0 data to legacy functions.  If you call:
> 
> void frob(struct foo *p);
> 
> If frob in turn passes p to a thread, what PKRU is it supposed to use?

The thread inheritance of PKRU can be nice.  It actually gives things a
good chance of working if you can control PKRU before clone().  I'd
describe the semantics like this:

	PKRU values are inherited at the time of a clone() system
	call.  Threads unaware of protection keys may work on
	protection-key-protected data as long as PKRU is set up in
	advance of the clone() and never needs to be changed inside the
	thread.

	If a thread is created before PKRU is set appropriately, the
	thread may not be able to act on protection-key-protected data.

Otherwise, the semantics are simpler, but they basically give threads no
chance of ever working:

	Threads unaware of protection keys and which can not manage
	PKRU may not operate on data where a non-zero key has been
	passed to pkey_mprotect().

It isn't clear to me that one of these is substantially better than the
other.  It's fairly easy in either case for an app that cares to get the
behavior of the other.

But, one is clearly easier to implement in the kernel. :)

>>> So how is user code supposed lock down all of its threads?
>>>
>>> seccomp has TSYNC for this, but I don't think that PKRU allows
>>> something like that.
>>
>> I'm not sure this is possible for PKRU.  Think of a simple PKRU
>> manipulation in userspace:
>>
>>         pkru = rdpkru();
>>         pkru |= PKEY_DENY_ACCESS<<key*2;
>>         wrpkru(pkru);
>>
>> If we push a PKRU value into a thread between the rdpkru() and wrpkru(),
>> we'll lose the content of that "push".  I'm not sure there's any way to
>> guarantee this with a user-controlled register.
> 
> We could try to insist that user code uses some vsyscall helper that
> tracks which bits are as-yet-unassigned.  That's quite messy, though.

Yeah, doable, but not without some new data going out to userspace, plus
the vsyscall code itself.

> We could also arbitrarily partition the key space into
> initially-wide-open, initially-read-only, and initially-no-access and
> let pkey_alloc say which kind it wants.

The point is still that wrpkru destroyed the 'push' operation.  You
always end up with a PKRU that (at least temporarily) ignored the 'push'.

[toc] | [prev] | [next] | [standalone]

#1441861

On Tue, Jul 12, 2016 at 10:12 AM, Dave Hansen <dave.hansen@intel.com> wrote:
> On 07/12/2016 09:32 AM, Andy Lutomirski wrote:
>> I think it's more or less impossible to get sensible behavior passing
>> pkey != 0 data to legacy functions.  If you call:
>>
>> void frob(struct foo *p);
>>
>> If frob in turn passes p to a thread, what PKRU is it supposed to use?
>
> The thread inheritance of PKRU can be nice.  It actually gives things a
> good chance of working if you can control PKRU before clone().  I'd
> describe the semantics like this:
>
>         PKRU values are inherited at the time of a clone() system
>         call.  Threads unaware of protection keys may work on
>         protection-key-protected data as long as PKRU is set up in
>         advance of the clone() and never needs to be changed inside the
>         thread.
>
>         If a thread is created before PKRU is set appropriately, the
>         thread may not be able to act on protection-key-protected data.

Given the apparent need for seccomp's TSYNC, I'm a bit nervous that
this will be restrictive to a problematic degree.

>
> Otherwise, the semantics are simpler, but they basically give threads no
> chance of ever working:
>
>         Threads unaware of protection keys and which can not manage
>         PKRU may not operate on data where a non-zero key has been
>         passed to pkey_mprotect().
>
> It isn't clear to me that one of these is substantially better than the
> other.  It's fairly easy in either case for an app that cares to get the
> behavior of the other.
>
> But, one is clearly easier to implement in the kernel. :)
>
>>>> So how is user code supposed lock down all of its threads?
>>>>
>>>> seccomp has TSYNC for this, but I don't think that PKRU allows
>>>> something like that.
>>>
>>> I'm not sure this is possible for PKRU.  Think of a simple PKRU
>>> manipulation in userspace:
>>>
>>>         pkru = rdpkru();
>>>         pkru |= PKEY_DENY_ACCESS<<key*2;
>>>         wrpkru(pkru);
>>>
>>> If we push a PKRU value into a thread between the rdpkru() and wrpkru(),
>>> we'll lose the content of that "push".  I'm not sure there's any way to
>>> guarantee this with a user-controlled register.
>>
>> We could try to insist that user code uses some vsyscall helper that
>> tracks which bits are as-yet-unassigned.  That's quite messy, though.
>
> Yeah, doable, but not without some new data going out to userspace, plus
> the vsyscall code itself.
>
>> We could also arbitrarily partition the key space into
>> initially-wide-open, initially-read-only, and initially-no-access and
>> let pkey_alloc say which kind it wants.
>
> The point is still that wrpkru destroyed the 'push' operation.  You
> always end up with a PKRU that (at least temporarily) ignored the 'push'.
>

Not with my partitioning proposal.  We'd never asynchronously modify
another thread's state -- we'd start start with a mask that gives us a
good chance of having the initial state always be useful.  To be
completely precise, the initial state would be something like:

0 = all access, 1 (PROT_EXEC) = deny read and write, 2-11: deny read
and write, 12-21: deny write, 22-31: all access

Then pkru_alloc would take a parameter giving the requested initial
state, and it would only work if a key with that initial state is
available.

If we went with the vdso approach, the API could look like:

pkru_state_t prev = pkru_push(mask, value);

...

pkru_pop(prev); // or pkru_pop(mask, prev)?

This doesn't fundamentally require the vdso, except that implementing
bitwise operations on PKRU can't be done atomically with RDPKRU /
WRPKRU.  Grr.  This also falls apart pretty badly when sigreturn
happens, so I don't think I like this approach.

--Andy

[toc] | [prev] | [next] | [standalone]

#1442131

* Andy Lutomirski <luto@amacapital.net> wrote:

> > If we push a PKRU value into a thread between the rdpkru() and wrpkru(), we'll 
> > lose the content of that "push".  I'm not sure there's any way to guarantee 
> > this with a user-controlled register.
> 
> We could try to insist that user code uses some vsyscall helper that tracks 
> which bits are as-yet-unassigned.  That's quite messy, though.

Actually, if we turned the vDSO into something more like a minimal user-space 
library with the ability to run at process startup as well to prepare stuff then 
it's painful to get right only *once*, and there will be tons of other areas where 
a proper per thread data storage on the user-space side would be immensely useful!

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1442765

On Wed, Jul 13, 2016 at 12:56 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Andy Lutomirski <luto@amacapital.net> wrote:
>
>> > If we push a PKRU value into a thread between the rdpkru() and wrpkru(), we'll
>> > lose the content of that "push".  I'm not sure there's any way to guarantee
>> > this with a user-controlled register.
>>
>> We could try to insist that user code uses some vsyscall helper that tracks
>> which bits are as-yet-unassigned.  That's quite messy, though.
>
> Actually, if we turned the vDSO into something more like a minimal user-space
> library with the ability to run at process startup as well to prepare stuff then
> it's painful to get right only *once*, and there will be tons of other areas where
> a proper per thread data storage on the user-space side would be immensely useful!

Doing this could be tricky: how exactly is the vDSO supposed to find
per-thread data without breaking existing glibc?

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web