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

Groups > linux.kernel > #1440241 > unrolled thread

Re: [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

  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

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

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] | [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]

#1443153

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

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

So I think the way this could be done is by allocating it itself. The vDSO vma 
itself is 'external' to glibc as well to begin with - this would be a small 
extension to that concept.

Thanks,

	Ingo

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

#1445204

On Thu, Jul 14, 2016 at 1:07 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Andy Lutomirski <luto@amacapital.net> wrote:
>
>> 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?
>
> So I think the way this could be done is by allocating it itself. The vDSO vma
> itself is 'external' to glibc as well to begin with - this would be a small
> extension to that concept.

But how does the vdso code find it?  FS and GS are both spoken for by
existing userspace.

--Andy

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

#1445344

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

> On Thu, Jul 14, 2016 at 1:07 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Andy Lutomirski <luto@amacapital.net> wrote:
> >
> >> 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?
> >
> > So I think the way this could be done is by allocating it itself. The vDSO vma
> > itself is 'external' to glibc as well to begin with - this would be a small
> > extension to that concept.
> 
> But how does the vdso code find it?  FS and GS are both spoken for by existing 
> userspace.

Minimally relinking itself on a per mm basis?

Thanks,

	Ingo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web