[0/6] Memory Mapping (VMA) protection using PKU - set 1

  From: Jeff Xu <jeffxu@google.com>

This is the first set of Memory mapping (VMA) protection patches using PKU.

* * * 

Background:

As discussed previously in the kernel mailing list [1], V8 CFI [2] uses 
PKU to protect memory, and Stephen Röttger proposes to extend the PKU to 
memory mapping [3].

We're using PKU for in-process isolation to enforce control-flow integrity
for a JIT compiler. In our threat model, an attacker exploits a 
vulnerability and has arbitrary read/write access to the whole process
space concurrently to other threads being executed. This attacker can
manipulate some arguments to syscalls from some threads.

Under such a powerful attack, we want to create a “safe/isolated”
thread environment. We assign dedicated PKUs to this thread, 
and use those PKUs to protect the threads’ runtime environment.
The thread has exclusive access to its run-time memory. This
includes modifying the protection of the memory mapping, or
munmap the memory mapping after use. And the other threads
won’t be able to access the memory or modify the memory mapping
(VMA) belonging to the thread.

* * * 

Proposed changes:

This patch introduces a new flag, PKEY_ENFORCE_API, to the pkey_alloc()
function. When a PKEY is created with this flag, it is enforced that any
thread that wants to make changes to the memory mapping (such as mprotect)
of the memory must have write access to the PKEY. PKEYs created without
this flag will continue to work as they do now, for backwards 
compatibility.

Only PKEY created from user space can have the new flag set, the PKEY
allocated by the kernel internally will not have it. In other words,
ARCH_DEFAULT_PKEY(0) and execute_only_pkey won’t have this flag set,
and continue work as today.

This flag is checked only at syscall entry, such as mprotect/munmap in
this set of patches. It will not apply to other call paths. In other
words, if the kernel want to change attributes of VMA for some reasons,
the kernel is free to do that and not affected by this new flag.

This set of patch covers mprotect/munmap, I plan to work on other 
syscalls after this. 

* * * 

Testing:

I have tested this patch on a Linux kernel 5.15, 6,1, and 6.4-rc1,
new selftest is added in: pkey_enforce_api.c 

* * * 

Discussion:

We believe that this patch provides a valuable security feature. 
It allows us to create “safe/isolated” thread environments that are 
protected from attackers with arbitrary read/write access to 
the process space.

We believe that the interface change and the patch don't 
introduce backwards compatibility risk.

We would like to disucss this patch in Linux kernel community
for feedback and support. 

* * * 

Reference:

[1]https://lore.kernel.org/all/202208221331.71C50A6F@keescook/
[2]https://docs.google.com/document/d/1O2jwK4dxI3nRcOJuPYkonhTkNQfbmwdvxQMyXgeaRHo/edit?usp=sharing
[3]https://docs.google.com/document/d/1qqVoVfRiF2nRylL3yjZyCQvzQaej1HRPh3f5wj1AS9I/edit


Best Regards,
-Jeff Xu

Jeff Xu (6):
  PKEY: Introduce PKEY_ENFORCE_API flag
  PKEY: Add arch_check_pkey_enforce_api()
  PKEY: Apply PKEY_ENFORCE_API to mprotect
  PKEY:selftest pkey_enforce_api for mprotect
  KEY: Apply PKEY_ENFORCE_API to munmap
  PKEY:selftest pkey_enforce_api for munmap

 arch/powerpc/include/asm/pkeys.h              |   19 +-
 arch/x86/include/asm/mmu.h                    |    7 +
 arch/x86/include/asm/pkeys.h                  |   92 +-
 arch/x86/mm/pkeys.c                           |    2 +-
 include/linux/mm.h                            |    2 +-
 include/linux/pkeys.h                         |   18 +-
 include/uapi/linux/mman.h                     |    5 +
 mm/mmap.c                                     |   34 +-
 mm/mprotect.c                                 |   31 +-
 mm/mremap.c                                   |    6 +-
 tools/testing/selftests/mm/Makefile           |    1 +
 tools/testing/selftests/mm/pkey_enforce_api.c | 1312 +++++++++++++++++
 12 files changed, 1507 insertions(+), 22 deletions(-)
 create mode 100644 tools/testing/selftests/mm/pkey_enforce_api.c


base-commit: ba0ad6ed89fd5dada3b7b65ef2b08e95d449d4ab
  

On 5/15/23 06:05, jeffxu@chromium.org wrote:
> We're using PKU for in-process isolation to enforce control-flow integrity
> for a JIT compiler. In our threat model, an attacker exploits a 
> vulnerability and has arbitrary read/write access to the whole process
> space concurrently to other threads being executed. This attacker can
> manipulate some arguments to syscalls from some threads.

This all sounds like it hinges on the contents of PKRU in the attacker
thread.

Could you talk a bit about how the attacker is prevented from running
WRPKRU, XRSTOR or compelling the kernel to write to PKRU like at sigreturn?
  

On Mon, May 15, 2023 at 4:28 PM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 5/15/23 06:05, jeffxu@chromium.org wrote:
> > We're using PKU for in-process isolation to enforce control-flow integrity
> > for a JIT compiler. In our threat model, an attacker exploits a
> > vulnerability and has arbitrary read/write access to the whole process
> > space concurrently to other threads being executed. This attacker can
> > manipulate some arguments to syscalls from some threads.
>
> This all sounds like it hinges on the contents of PKRU in the attacker
> thread.
>
> Could you talk a bit about how the attacker is prevented from running
> WRPKRU, XRSTOR or compelling the kernel to write to PKRU like at sigreturn?

(resending without html)

Since we're using the feature for control-flow integrity, we assume
the control-flow is still intact at this point. I.e. the attacker
thread can't run arbitrary instructions.
* For JIT code, we're going to scan it for wrpkru instructions before
writing it to executable memory
* For regular code, we only use wrpkru around short critical sections
to temporarily enable write access

Sigreturn is a separate problem that we hope to solve by adding pkey
support to sigaltstack
  

On Mon, May 15, 2023 at 01:05:46PM +0000, jeffxu@chromium.org wrote:
> This patch introduces a new flag, PKEY_ENFORCE_API, to the pkey_alloc()
> function. When a PKEY is created with this flag, it is enforced that any
> thread that wants to make changes to the memory mapping (such as mprotect)
> of the memory must have write access to the PKEY. PKEYs created without
> this flag will continue to work as they do now, for backwards 
> compatibility.
> 
> Only PKEY created from user space can have the new flag set, the PKEY
> allocated by the kernel internally will not have it. In other words,
> ARCH_DEFAULT_PKEY(0) and execute_only_pkey won’t have this flag set,
> and continue work as today.

Cool! Yeah, this looks like it could become quite useful. I assume
V8 folks are on board with this API, etc?

> This set of patch covers mprotect/munmap, I plan to work on other 
> syscalls after this. 

Which ones are on your list currently?
  

On Tue, May 16, 2023 at 1:08 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, May 15, 2023 at 01:05:46PM +0000, jeffxu@chromium.org wrote:
> > This patch introduces a new flag, PKEY_ENFORCE_API, to the pkey_alloc()
> > function. When a PKEY is created with this flag, it is enforced that any
> > thread that wants to make changes to the memory mapping (such as mprotect)
> > of the memory must have write access to the PKEY. PKEYs created without
> > this flag will continue to work as they do now, for backwards
> > compatibility.
> >
> > Only PKEY created from user space can have the new flag set, the PKEY
> > allocated by the kernel internally will not have it. In other words,
> > ARCH_DEFAULT_PKEY(0) and execute_only_pkey won’t have this flag set,
> > and continue work as today.
>
> Cool! Yeah, this looks like it could become quite useful. I assume
> V8 folks are on board with this API, etc?
>
> > This set of patch covers mprotect/munmap, I plan to work on other
> > syscalls after this.
>
> Which ones are on your list currently?
>
mprotect/mprotect_pkey/munmap
mmap/mremap
madvice,brk,sbrk

Thanks!
-Jeff Xu

> --
> Kees Cook
  

On 5/16/23 15:17, Jeff Xu wrote:
>>> This set of patch covers mprotect/munmap, I plan to work on other
>>> syscalls after this.
>> Which ones are on your list currently?
>>
> mprotect/mprotect_pkey/munmap
> mmap/mremap
> madvice,brk,sbrk

What about pkey_free()?

Without that, someone can presumably free the pkey and then reallocate
it without PKEY_ENFORCE_API.
  

On 5/16/23 00:06, Stephen Röttger wrote:
> On Mon, May 15, 2023 at 4:28 PM Dave Hansen <dave.hansen@intel.com> wrote:
>>
>> On 5/15/23 06:05, jeffxu@chromium.org wrote:
>>> We're using PKU for in-process isolation to enforce control-flow integrity
>>> for a JIT compiler. In our threat model, an attacker exploits a
>>> vulnerability and has arbitrary read/write access to the whole process
>>> space concurrently to other threads being executed. This attacker can
>>> manipulate some arguments to syscalls from some threads.
>>
>> This all sounds like it hinges on the contents of PKRU in the attacker
>> thread.
>>
>> Could you talk a bit about how the attacker is prevented from running
>> WRPKRU, XRSTOR or compelling the kernel to write to PKRU like at sigreturn?
> 
> (resending without html)
> 
> Since we're using the feature for control-flow integrity, we assume
> the control-flow is still intact at this point. I.e. the attacker
> thread can't run arbitrary instructions.

Can't run arbitrary instructions, but can make (pretty) arbitrary syscalls?

> * For JIT code, we're going to scan it for wrpkru instructions before
> writing it to executable memory

... and XRSTOR, right?

> * For regular code, we only use wrpkru around short critical sections
> to temporarily enable write access
> 
> Sigreturn is a separate problem that we hope to solve by adding pkey
> support to sigaltstack

What kind of support were you planning to add?

I was thinking that an attacker with arbitrary write access would wait
until PKRU was on the userspace stack and *JUST* before the kernel
sigreturn code restores it to write a malicious value.  It could
presumably do this with some asynchronous mechanism so that even if
there was only one attacker thread, it could change its own value.

Also, the kernel side respect for PKRU is ... well ... rather weak.
It's a best effort and if we *happen* to be in a kernel context where
PKRU is relevant, we can try to respect PKRU.  But there are a whole
bunch of things like get_user_pages_remote() that just plain don't have
PKRU available and can't respect it at all.

I think io_uring also greatly expanded how common "remote" access to
process memory is.

So, overall, I'm thrilled to see another potential user for pkeys.  It
sounds like there's an actual user lined up here, which would be
wonderful.  But, I also want to make sure we don't go to the trouble to
build something that doesn't actually present meaningful, durable
obstacles to an attacker.

I also haven't more than glanced at the code.
  

On Tue, May 16, 2023 at 3:30 PM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 5/16/23 15:17, Jeff Xu wrote:
> >>> This set of patch covers mprotect/munmap, I plan to work on other
> >>> syscalls after this.
> >> Which ones are on your list currently?
> >>
> > mprotect/mprotect_pkey/munmap
> > mmap/mremap
> > madvice,brk,sbrk
>
> What about pkey_free()?
>
> Without that, someone can presumably free the pkey and then reallocate
> it without PKEY_ENFORCE_API.
>
Great catch. I will add it to the list.
Thanks!
-Jeff Xu

>
  

On Tue, May 16, 2023 at 10:08 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, May 15, 2023 at 01:05:46PM +0000, jeffxu@chromium.org wrote:
> > This patch introduces a new flag, PKEY_ENFORCE_API, to the pkey_alloc()
> > function. When a PKEY is created with this flag, it is enforced that any
> > thread that wants to make changes to the memory mapping (such as mprotect)
> > of the memory must have write access to the PKEY. PKEYs created without
> > this flag will continue to work as they do now, for backwards
> > compatibility.
> >
> > Only PKEY created from user space can have the new flag set, the PKEY
> > allocated by the kernel internally will not have it. In other words,
> > ARCH_DEFAULT_PKEY(0) and execute_only_pkey won’t have this flag set,
> > and continue work as today.
>
> Cool! Yeah, this looks like it could become quite useful. I assume
> V8 folks are on board with this API, etc?

Yes! (I'm from the v8 team driving the implementation on v8 side)

> > This set of patch covers mprotect/munmap, I plan to work on other
> > syscalls after this.
>
> Which ones are on your list currently?
>
> --
> Kees Cook
  

On Wed, May 17, 2023 at 12:41 AM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 5/16/23 00:06, Stephen Röttger wrote:
> > On Mon, May 15, 2023 at 4:28 PM Dave Hansen <dave.hansen@intel.com> wrote:
> >>
> >> On 5/15/23 06:05, jeffxu@chromium.org wrote:
> >>> We're using PKU for in-process isolation to enforce control-flow integrity
> >>> for a JIT compiler. In our threat model, an attacker exploits a
> >>> vulnerability and has arbitrary read/write access to the whole process
> >>> space concurrently to other threads being executed. This attacker can
> >>> manipulate some arguments to syscalls from some threads.
> >>
> >> This all sounds like it hinges on the contents of PKRU in the attacker
> >> thread.
> >>
> >> Could you talk a bit about how the attacker is prevented from running
> >> WRPKRU, XRSTOR or compelling the kernel to write to PKRU like at sigreturn?
> >
> > (resending without html)
> >
> > Since we're using the feature for control-flow integrity, we assume
> > the control-flow is still intact at this point. I.e. the attacker
> > thread can't run arbitrary instructions.
>
> Can't run arbitrary instructions, but can make (pretty) arbitrary syscalls?

The threat model is that the attacker has arbitrary read/write, while other
threads run in parallel. So whenever a regular thread performs a syscall and
takes a syscall argument from memory, we assume that argument can be attacker
controlled.
Unfortunately, the line is a bit blurry which syscalls / syscall arguments we
need to assume to be attacker controlled. We're trying to approach this by
roughly categorizing syscalls+args:
* how commonly used is the syscall
* do we expect the argument to be taken from writable memory
* can we restrict the syscall+args with seccomp
* how difficult is it to restrict the syscall in userspace vs kernel
* does the syscall affect our protections (e.g. change control-flow or pkey)

Using munmap as an example:
* it's a very common syscall (nearly every seccomp filter will allow munmap)
* the addr argument will come from memory
* unmapping pkey-tagged pages breaks our assumptions
* it's hard to restrict in userspace since we'd need to keep track of all
  address ranges that are unsafe to unmap and hook the syscall to perform the
  validation on every call in the codebase.
* it's easy to validate in kernel with this patch

For most other syscalls, they either don't affect the control-flow, are easy to
avoid and block with seccomp or we can add validation in userspace (e.g. only
install signal handlers at program startup).

> > * For JIT code, we're going to scan it for wrpkru instructions before
> > writing it to executable memory
>
> ... and XRSTOR, right?

Right. We’ll just have a list of allowed instructions that the JIT compiler can
emit.

>
> > * For regular code, we only use wrpkru around short critical sections
> > to temporarily enable write access
> >
> > Sigreturn is a separate problem that we hope to solve by adding pkey
> > support to sigaltstack
>
> What kind of support were you planning to add?

We’d like to allow registering pkey-tagged memory as a sigaltstack. This would
allow the signal handler to run isolated from other threads. Right now, the
main reason this doesn’t work is that the kernel would need to change the pkru
state before storing the register state on the stack.

> I was thinking that an attacker with arbitrary write access would wait
> until PKRU was on the userspace stack and *JUST* before the kernel
> sigreturn code restores it to write a malicious value.  It could
> presumably do this with some asynchronous mechanism so that even if
> there was only one attacker thread, it could change its own value.

I’m not sure I follow the details, can you give an example of an asynchronous
mechanism to do this? E.g. would this be the kernel writing to the memory in a
syscall for example?

> Also, the kernel side respect for PKRU is ... well ... rather weak.
> It's a best effort and if we *happen* to be in a kernel context where
> PKRU is relevant, we can try to respect PKRU.  But there are a whole
> bunch of things like get_user_pages_remote() that just plain don't have
> PKRU available and can't respect it at all.
>
> I think io_uring also greatly expanded how common "remote" access to
> process memory is.
>
> So, overall, I'm thrilled to see another potential user for pkeys.  It
> sounds like there's an actual user lined up here, which would be
> wonderful.  But, I also want to make sure we don't go to the trouble to
> build something that doesn't actually present meaningful, durable
> obstacles to an attacker.
>
> I also haven't more than glanced at the code.
  

On 5/17/23 03:51, Stephen Röttger wrote:
> On Wed, May 17, 2023 at 12:41 AM Dave Hansen <dave.hansen@intel.com> wrote:
>> Can't run arbitrary instructions, but can make (pretty) arbitrary syscalls?
> 
> The threat model is that the attacker has arbitrary read/write, while other
> threads run in parallel. So whenever a regular thread performs a syscall and
> takes a syscall argument from memory, we assume that argument can be attacker
> controlled.
> Unfortunately, the line is a bit blurry which syscalls / syscall arguments we
> need to assume to be attacker controlled. 

Ahh, OK.  So, it's not that the *attacker* can make arbitrary syscalls.
It's that the attacker might leverage its arbitrary write to trick a
victim thread into turning what would otherwise be a good syscall into a
bad one with attacker-controlled content.

I guess that makes the readv/writev-style of things a bad idea in this
environment.

>>> Sigreturn is a separate problem that we hope to solve by adding pkey
>>> support to sigaltstack
>>
>> What kind of support were you planning to add?
> 
> We’d like to allow registering pkey-tagged memory as a sigaltstack. This would
> allow the signal handler to run isolated from other threads. Right now, the
> main reason this doesn’t work is that the kernel would need to change the pkru
> state before storing the register state on the stack.
> 
>> I was thinking that an attacker with arbitrary write access would wait
>> until PKRU was on the userspace stack and *JUST* before the kernel
>> sigreturn code restores it to write a malicious value.  It could
>> presumably do this with some asynchronous mechanism so that even if
>> there was only one attacker thread, it could change its own value.
> 
> I’m not sure I follow the details, can you give an example of an asynchronous
> mechanism to do this? E.g. would this be the kernel writing to the memory in a
> syscall for example?

I was thinking of all of the IORING_OP_*'s that can write to memory or
aio(7).
  

On Wed, May 17, 2023 at 8:07 AM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 5/17/23 03:51, Stephen Röttger wrote:
> > On Wed, May 17, 2023 at 12:41 AM Dave Hansen <dave.hansen@intel.com> wrote:
> >> Can't run arbitrary instructions, but can make (pretty) arbitrary syscalls?
> >
> > The threat model is that the attacker has arbitrary read/write, while other
> > threads run in parallel. So whenever a regular thread performs a syscall and
> > takes a syscall argument from memory, we assume that argument can be attacker
> > controlled.
> > Unfortunately, the line is a bit blurry which syscalls / syscall arguments we
> > need to assume to be attacker controlled.
>
> Ahh, OK.  So, it's not that the *attacker* can make arbitrary syscalls.
> It's that the attacker might leverage its arbitrary write to trick a
> victim thread into turning what would otherwise be a good syscall into a
> bad one with attacker-controlled content.
>
> I guess that makes the readv/writev-style of things a bad idea in this
> environment.
>
> >>> Sigreturn is a separate problem that we hope to solve by adding pkey
> >>> support to sigaltstack
> >>
> >> What kind of support were you planning to add?
> >
> > We’d like to allow registering pkey-tagged memory as a sigaltstack. This would
> > allow the signal handler to run isolated from other threads. Right now, the
> > main reason this doesn’t work is that the kernel would need to change the pkru
> > state before storing the register state on the stack.
> >
> >> I was thinking that an attacker with arbitrary write access would wait
> >> until PKRU was on the userspace stack and *JUST* before the kernel
> >> sigreturn code restores it to write a malicious value.  It could
> >> presumably do this with some asynchronous mechanism so that even if
> >> there was only one attacker thread, it could change its own value.
> >
> > I’m not sure I follow the details, can you give an example of an asynchronous
> > mechanism to do this? E.g. would this be the kernel writing to the memory in a
> > syscall for example?
>
> I was thinking of all of the IORING_OP_*'s that can write to memory or
> aio(7).

IORING is challenging from security perspectives, for now, it is
disabled in ChromeOS.
Though I'm not sure how aio is related ?

Thanks
-Jeff
  

On 5/17/23 08:21, Jeff Xu wrote:
>>> I’m not sure I follow the details, can you give an example of an asynchronous
>>> mechanism to do this? E.g. would this be the kernel writing to the memory in a
>>> syscall for example?
>> I was thinking of all of the IORING_OP_*'s that can write to memory or
>> aio(7).
> IORING is challenging from security perspectives, for now, it is 
> disabled in ChromeOS. Though I'm not sure how aio is related ?

Let's say you're the attacking thread and you're the *only* attacking
thread.  You have three things at your disposal:

 1. A benign thread doing aio_read()
 2. An arbitrary write primitive
 3. You can send signals to yourself
 4. You can calculate where your signal stack will be

You calculate the address of PKRU on the future signal stack.  You then
leverage the otherwise benign aio_write() to write a 0 to that PKRU
location.  Then, send a signal to yourself.  The attacker's PKRU value
will be written to the stack.  If you can time it right, the AIO will
complete while the signal handler is in progress and PKRU is on the
stack.  On sigreturn, the kernel restores the aio_read()-placed,
attacker-provided PKRU value.  Now the attacker has PKRU==0.  It
effectively build a WRPKRU primitive out of those other pieces.
  

On Wed, May 17, 2023 at 8:29 AM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 5/17/23 08:21, Jeff Xu wrote:
> >>> I’m not sure I follow the details, can you give an example of an asynchronous
> >>> mechanism to do this? E.g. would this be the kernel writing to the memory in a
> >>> syscall for example?
> >> I was thinking of all of the IORING_OP_*'s that can write to memory or
> >> aio(7).
> > IORING is challenging from security perspectives, for now, it is
> > disabled in ChromeOS. Though I'm not sure how aio is related ?
>
> Let's say you're the attacking thread and you're the *only* attacking
> thread.  You have three things at your disposal:
>
>  1. A benign thread doing aio_read()
>  2. An arbitrary write primitive
>  3. You can send signals to yourself
>  4. You can calculate where your signal stack will be
>
> You calculate the address of PKRU on the future signal stack.  You then
> leverage the otherwise benign aio_write() to write a 0 to that PKRU
> location.  Then, send a signal to yourself.  The attacker's PKRU value
> will be written to the stack.  If you can time it right, the AIO will
> complete while the signal handler is in progress and PKRU is on the
> stack.  On sigreturn, the kernel restores the aio_read()-placed,
> attacker-provided PKRU value.  Now the attacker has PKRU==0.  It
> effectively build a WRPKRU primitive out of those other pieces.
>
>
Ah, I understand the question now, thanks for the explanation.
Signalling handling is the next project that I will be working on.

I'm leaning towards saving PKRU register to the thread struct, similar
to how context switch works. This will address the attack scenario you
described.
However, there are a few challenges I have not yet worked through.
First, the code needs to track when the first signaling entry occurs
(saving the PKRU register to the thread struct) and when it is last
returned (restoring the PKRU register from the thread struct). One way
to do this would be to add another member to the thread struct to
track the level of signaling re-entry. Second, signal is used in
error handling, including the kernel's own signaling handling code
path. I haven't worked through this part of code logic completely.

If the first approach is too complicated or considered intrusive,  I
could take a different approach. In this approach, I would not track
signaling re-entry. Instead, I would modify the PKRU saved in AltStack
during handling of the signal, the steps are:
a> save PKRU to tmp variable.
b> modify PKRU to allow writing to the PKEY protected AltStack
c> XSAVE.
d> write tmp to the memory address of PKRU in  AltStack at the
correct offset.
Since the thread's PKRU is saved to stack, XRSTOR will restore the
thread's original PKRU during sigreturn in normal situations. This
approach might be a little hacky because it overwrites XSAVE results.
If we go with this route, I need someone's help on the overwriting
function, it is CPU specific.
However this approach will not work if an attacker can install its own
signaling handling (therefore gains the ability to overwrite PKRU stored
in stack, as you described), the application will want to install all the
signaling handling with PKEY protected AltStack at startup time, and
disallow additional signaling handling after that, this is programmatically
achievable in V8, as Stephan mentioned.

I would appreciate getting more comments in the signaling handling
area on those two approaches, or are there  better ways to do what we
want? Do you think we could continue signaling handling discussion
from the original thread that Kees started [1] ? There were already
lots of discussions there about signalling handling,  so it will be
easier for future readers to understand the context. I can repost
there. Or I can start a new thread for signaling handling, I'm
worried that those discussions will get lengthy and context get lost
with patch version update.

Although the signaling handling project is related,  I think VMA
protection using the PKRU project can stand on its own. We could solve
this for V8 first then move next to Signaling handling, the work here
could also pave the way to add mseal() in future, I expect lots of
code logic will be similar.

[1] https://lore.kernel.org/all/202208221331.71C50A6F@keescook/

Thanks!
Best regards,
-Jeff Xu








On Wed, May 17, 2023 at 8:29 AM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 5/17/23 08:21, Jeff Xu wrote:
> >>> I’m not sure I follow the details, can you give an example of an asynchronous
> >>> mechanism to do this? E.g. would this be the kernel writing to the memory in a
> >>> syscall for example?
> >> I was thinking of all of the IORING_OP_*'s that can write to memory or
> >> aio(7).
> > IORING is challenging from security perspectives, for now, it is
> > disabled in ChromeOS. Though I'm not sure how aio is related ?
>
> Let's say you're the attacking thread and you're the *only* attacking
> thread.  You have three things at your disposal:
>
>  1. A benign thread doing aio_read()
>  2. An arbitrary write primitive
>  3. You can send signals to yourself
>  4. You can calculate where your signal stack will be
>
> You calculate the address of PKRU on the future signal stack.  You then
> leverage the otherwise benign aio_write() to write a 0 to that PKRU
> location.  Then, send a signal to yourself.  The attacker's PKRU value
> will be written to the stack.  If you can time it right, the AIO will
> complete while the signal handler is in progress and PKRU is on the
> stack.  On sigreturn, the kernel restores the aio_read()-placed,
> attacker-provided PKRU value.  Now the attacker has PKRU==0.  It
> effectively build a WRPKRU primitive out of those other pieces.
>
>
  

On 5/17/23 16:48, Jeff Xu wrote:
> However, there are a few challenges I have not yet worked through.
> First, the code needs to track when the first signaling entry occurs
> (saving the PKRU register to the thread struct) and when it is last
> returned (restoring the PKRU register from the thread struct). 

Would tracking signal "depth" work in the face of things like siglongjmp?

Taking a step back...

Here's my concern about this whole thing: it's headed down a rabbit hole
which is *highly* specialized both in the apps that will use it and the
attacks it will mitigate.  It probably *requires* turning off a bunch of
syscalls (like io_uring) that folks kinda like in general.

We're balancing that highly specialized mitigation with a feature that
add new ABI, touches core memory management code and signal handling.

On the x86 side, PKRU is a painfully special snowflake.  It's exposed in
the "XSAVE" ABIs, but not actually managed *with* XSAVE in the kernel.
This would be making it an even more special snowflake because it would
need new altstack ABI and handling.

I'm just not sure the gain is worth the pain.
  

Hello Dave,

Thanks for your email.

On Thu, May 18, 2023 at 8:38 AM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 5/17/23 16:48, Jeff Xu wrote:
> > However, there are a few challenges I have not yet worked through.
> > First, the code needs to track when the first signaling entry occurs
> > (saving the PKRU register to the thread struct) and when it is last
> > returned (restoring the PKRU register from the thread struct).
>
> Would tracking signal "depth" work in the face of things like siglongjmp?
>
Thank you for your question! I am eager to learn more about this area
and I worry about blind spots. I will investigate and get back to you.

> Taking a step back...
>
> Here's my concern about this whole thing: it's headed down a rabbit hole
> which is *highly* specialized both in the apps that will use it and the
> attacks it will mitigate.  It probably *requires* turning off a bunch of
> syscalls (like io_uring) that folks kinda like in general.
>
ChromeOS currently disabled io_uring, but it is not required to do so.
io_uring supports the IORING_OP_MADVICE operation, which calls the
do_madvise() function. This means that io_uring will have the same
pkey checks as the madvice() system call.  From that perspective, we
will fully support io_uring for this feature.

> We're balancing that highly specialized mitigation with a feature that
> add new ABI, touches core memory management code and signal handling.
>
The ABI change uses the existing flag field in pkey_alloc() which is
reserved. The implementation is backward compatible with all existing
pkey usages in both kernel and user space.  Or do you have other
concerns about ABI in mind ?

Yes, you are right about the risk of touching core mm code. To
minimize the risk, I try to control the scope of the change (it is
about 3 lines in mprotect, more in munmap but really just 3 effective
lines from syscall entry). I added new self-tests in mm to make sure
it doesn't regress in api behavior. I run those tests before and after
my kernel code change to make sure the behavior remains the same, I
tested it on 5.15 and 6.1 and 6.4-rc1.  Actually, the testing
discovered a behavior change for mprotect() between 6.1 and 6.4  (not
from this patch, there are refactoring works going on in mm) see this
thread [1]
I hope those steps will help to mitigate the risk.

Agreed on signaling handling is a tough part: what do you think about
the approach (modifying PKRU from saved stack after XSAVE), is there a
blocker ?

> On the x86 side, PKRU is a painfully special snowflake.  It's exposed in
> the "XSAVE" ABIs, but not actually managed *with* XSAVE in the kernel.
> This would be making it an even more special snowflake because it would

I admit I'm quite ignorant on XSAVE  to understand the above
statement, and how that is related. Could you explain it to me please
? And what is in your mind that might improve the situation ?

> need new altstack ABI and handling.
>
I thought adding protected memory support to signaling handling is an
independent project with its own weight. As Jann Horn points out in
[2]:  "we could prevent the attacker from corrupting the signal
context if we can protect the signal stack with a pkey."   However,
the kernel will send SIGSEGV when the stack is protected by PKEY,  so
there is a benefit to make this work.  (Maybe Jann can share some more
thoughts on the benefits)

And I believe we could do this in a way with minimum ABI change, as below:
- allocate PKEY with a new flag (PKEY_ALTSTACK)
- at sigaltstack() call, detect the memory is PKEY_ALTSTACK protected,
(similar as what mprotect does in this patch) and save it along with
stack address/size.
- at signaling handling, use the saved info to fill in PKRU.
The ABI change is similar to PKEY_ENFORCE_API, and there is no
backward compatibility issue.

Will these mentioned help our case ? What do you think ?

(Stephan has more info on gains,  as far as I know, V8 engineers have
worked/thought really hard to come to a suitable solution to make
chrome browser safer)

[1] https://lore.kernel.org/linux-mm/20230516165754.pocx4kaagn3yyw3r@revolver/T/
[2] https://docs.google.com/document/d/1OlnJbR5TMoaOAJsf4hHOc-FdTmYK2aDUI7d2hfCZSOo/edit?resourcekey=0-v9UJXONYsnG5PlCBbcYqIw#

Thanks!
Best regards,
-Jeff
  

On 5/18/23 13:20, Jeff Xu wrote:>> Here's my concern about this whole
thing: it's headed down a rabbit hole
>> which is *highly* specialized both in the apps that will use it and the
>> attacks it will mitigate.  It probably *requires* turning off a bunch of
>> syscalls (like io_uring) that folks kinda like in general.
>>
> ChromeOS currently disabled io_uring, but it is not required to do so.
> io_uring supports the IORING_OP_MADVICE operation, which calls the
> do_madvise() function. This means that io_uring will have the same
> pkey checks as the madvice() system call.  From that perspective, we
> will fully support io_uring for this feature.

io_uring fundamentally doesn't have the same checks.  The kernel side
work can be done from an asynchronous kernel thread.  That kernel thread
doesn't have a meaningful PKRU value.  The register has a value, but
it's not really related to the userspace threads that are sending it
requests.

>> We're balancing that highly specialized mitigation with a feature that
>> add new ABI, touches core memory management code and signal handling.
>>
> The ABI change uses the existing flag field in pkey_alloc() which is
> reserved. The implementation is backward compatible with all existing
> pkey usages in both kernel and user space.  Or do you have other
> concerns about ABI in mind ?

I'm not worried about the past, I'm worried any time we add a new ABI
since we need to support it forever.

> Yes, you are right about the risk of touching core mm code. To
> minimize the risk, I try to control the scope of the change (it is
> about 3 lines in mprotect, more in munmap but really just 3 effective
> lines from syscall entry). I added new self-tests in mm to make sure
> it doesn't regress in api behavior. I run those tests before and after
> my kernel code change to make sure the behavior remains the same, I
> tested it on 5.15 and 6.1 and 6.4-rc1.  Actually, the testing
> discovered a behavior change for mprotect() between 6.1 and 6.4  (not
> from this patch, there are refactoring works going on in mm) see this
> thread [1]
> I hope those steps will help to mitigate the risk.
> 
> Agreed on signaling handling is a tough part: what do you think about
> the approach (modifying PKRU from saved stack after XSAVE), is there a
> blocker ?

Yes, signal entry and sigreturn are not necessarily symmetric so you
can't really have a stack.

>> On the x86 side, PKRU is a painfully special snowflake.  It's exposed in
>> the "XSAVE" ABIs, but not actually managed *with* XSAVE in the kernel.
>> This would be making it an even more special snowflake because it would
> 
> I admit I'm quite ignorant on XSAVE  to understand the above
> statement, and how that is related. Could you explain it to me please
> ? And what is in your mind that might improve the situation ?

In a nutshell: XSAVE components are classified as either user or
supervisor.  User components can be modified from userspace and
supervisor ones only from the kernel.  In general, user components don't
affect the kernel; the kernel doesn't care what is in ZMM11 (an
XSAVE-managed register).  That lets us do fun stuff like be lazy about
when ZMM11 is saved/restored.  Being lazy is good because it give us
things like faster context switches and KVM VMEXIT handling.

PKRU is a user component, but it affects the kernel when the kernel does
copy_to/from_user() and friends.  That means that the kernel can't do
any "fun stuff" with PKRU.  As soon as userspace provides a new value,
the kernel needs to start respecting it.  That makes PKRU a very special
snowflake.

So, even though PKRU can be managed by XSAVE, it isn't.  It isn't kept
in the kernel XSAVE buffer.  But it *IS* in the signal stack XSAVE
buffer.  You *can* save/restore it with the other XSAVE components with
ptrace().  The user<->kernel ABI pretends that PKRU is XSAVE managed
even though it is not.

All of this is special-cased.  There's a ton of code to handle this
mess.  It's _complicated_.  I haven't even started talking about how
this interacts with KVM and guests.

How could we improve it?  A time machine would help to either change the
architecture or have Linux ignore the fact that XSAVE knows anything
about PKRU.

So, the bar is pretty high for things that want to further muck with
PKRU.  Add signal and sigaltstack in particular into the fray, and we've
got a recipe for disaster.  sigaltstack and XSAVE don't really get along
very well.  https://lwn.net/Articles/862541/

>> need new altstack ABI and handling.
>>
> I thought adding protected memory support to signaling handling is an
> independent project with its own weight. As Jann Horn points out in
> [2]:  "we could prevent the attacker from corrupting the signal
> context if we can protect the signal stack with a pkey."   However,
> the kernel will send SIGSEGV when the stack is protected by PKEY,  so
> there is a benefit to make this work.  (Maybe Jann can share some more
> thoughts on the benefits)
> 
> And I believe we could do this in a way with minimum ABI change, as below:
> - allocate PKEY with a new flag (PKEY_ALTSTACK)
> - at sigaltstack() call, detect the memory is PKEY_ALTSTACK protected,
> (similar as what mprotect does in this patch) and save it along with
> stack address/size.
> - at signaling handling, use the saved info to fill in PKRU.
> The ABI change is similar to PKEY_ENFORCE_API, and there is no
> backward compatibility issue.
> 
> Will these mentioned help our case ? What do you think ?

To be honest, no.

What you've laid out here is the tip of the complexity iceberg.  There
are a lot of pieces of the kernel that are not yet factored in.

Let's also remember: protection keys is *NOT* a security feature.  It's
arguable that pkeys is a square peg trying to go into a round security hole.
  

On Thu, May 18, 2023 at 11:04 PM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 5/18/23 13:20, Jeff Xu wrote:>> Here's my concern about this whole
> thing: it's headed down a rabbit hole
> >> which is *highly* specialized both in the apps that will use it and the
> >> attacks it will mitigate.  It probably *requires* turning off a bunch of
> >> syscalls (like io_uring) that folks kinda like in general.
> >>
> > ChromeOS currently disabled io_uring, but it is not required to do so.
> > io_uring supports the IORING_OP_MADVICE operation, which calls the
> > do_madvise() function. This means that io_uring will have the same
> > pkey checks as the madvice() system call.  From that perspective, we
> > will fully support io_uring for this feature.
>
> io_uring fundamentally doesn't have the same checks.  The kernel side
> work can be done from an asynchronous kernel thread.  That kernel thread
> doesn't have a meaningful PKRU value.  The register has a value, but
> it's not really related to the userspace threads that are sending it
> requests.
>
> >> We're balancing that highly specialized mitigation with a feature that
> >> add new ABI, touches core memory management code and signal handling.
> >>
> > The ABI change uses the existing flag field in pkey_alloc() which is
> > reserved. The implementation is backward compatible with all existing
> > pkey usages in both kernel and user space.  Or do you have other
> > concerns about ABI in mind ?
>
> I'm not worried about the past, I'm worried any time we add a new ABI
> since we need to support it forever.
>
> > Yes, you are right about the risk of touching core mm code. To
> > minimize the risk, I try to control the scope of the change (it is
> > about 3 lines in mprotect, more in munmap but really just 3 effective
> > lines from syscall entry). I added new self-tests in mm to make sure
> > it doesn't regress in api behavior. I run those tests before and after
> > my kernel code change to make sure the behavior remains the same, I
> > tested it on 5.15 and 6.1 and 6.4-rc1.  Actually, the testing
> > discovered a behavior change for mprotect() between 6.1 and 6.4  (not
> > from this patch, there are refactoring works going on in mm) see this
> > thread [1]
> > I hope those steps will help to mitigate the risk.
> >
> > Agreed on signaling handling is a tough part: what do you think about
> > the approach (modifying PKRU from saved stack after XSAVE), is there a
> > blocker ?
>
> Yes, signal entry and sigreturn are not necessarily symmetric so you
> can't really have a stack.
>
> >> On the x86 side, PKRU is a painfully special snowflake.  It's exposed in
> >> the "XSAVE" ABIs, but not actually managed *with* XSAVE in the kernel.
> >> This would be making it an even more special snowflake because it would
> >
> > I admit I'm quite ignorant on XSAVE  to understand the above
> > statement, and how that is related. Could you explain it to me please
> > ? And what is in your mind that might improve the situation ?
>
> In a nutshell: XSAVE components are classified as either user or
> supervisor.  User components can be modified from userspace and
> supervisor ones only from the kernel.  In general, user components don't
> affect the kernel; the kernel doesn't care what is in ZMM11 (an
> XSAVE-managed register).  That lets us do fun stuff like be lazy about
> when ZMM11 is saved/restored.  Being lazy is good because it give us
> things like faster context switches and KVM VMEXIT handling.
>
> PKRU is a user component, but it affects the kernel when the kernel does
> copy_to/from_user() and friends.  That means that the kernel can't do
> any "fun stuff" with PKRU.  As soon as userspace provides a new value,
> the kernel needs to start respecting it.  That makes PKRU a very special
> snowflake.
>
> So, even though PKRU can be managed by XSAVE, it isn't.  It isn't kept
> in the kernel XSAVE buffer.  But it *IS* in the signal stack XSAVE
> buffer.  You *can* save/restore it with the other XSAVE components with
> ptrace().  The user<->kernel ABI pretends that PKRU is XSAVE managed
> even though it is not.
>
> All of this is special-cased.  There's a ton of code to handle this
> mess.  It's _complicated_.  I haven't even started talking about how
> this interacts with KVM and guests.
>
> How could we improve it?  A time machine would help to either change the
> architecture or have Linux ignore the fact that XSAVE knows anything
> about PKRU.
>
> So, the bar is pretty high for things that want to further muck with
> PKRU.  Add signal and sigaltstack in particular into the fray, and we've
> got a recipe for disaster.  sigaltstack and XSAVE don't really get along
> very well.  https://lwn.net/Articles/862541/
>
> >> need new altstack ABI and handling.
> >>
> > I thought adding protected memory support to signaling handling is an
> > independent project with its own weight. As Jann Horn points out in
> > [2]:  "we could prevent the attacker from corrupting the signal
> > context if we can protect the signal stack with a pkey."   However,
> > the kernel will send SIGSEGV when the stack is protected by PKEY,  so
> > there is a benefit to make this work.  (Maybe Jann can share some more
> > thoughts on the benefits)
> >
> > And I believe we could do this in a way with minimum ABI change, as below:
> > - allocate PKEY with a new flag (PKEY_ALTSTACK)
> > - at sigaltstack() call, detect the memory is PKEY_ALTSTACK protected,
> > (similar as what mprotect does in this patch) and save it along with
> > stack address/size.
> > - at signaling handling, use the saved info to fill in PKRU.
> > The ABI change is similar to PKEY_ENFORCE_API, and there is no
> > backward compatibility issue.
> >
> > Will these mentioned help our case ? What do you think ?
>
> To be honest, no.
>
> What you've laid out here is the tip of the complexity iceberg.  There
> are a lot of pieces of the kernel that are not yet factored in.
>
> Let's also remember: protection keys is *NOT* a security feature.  It's
> arguable that pkeys is a square peg trying to go into a round security hole.

While they're not a security feature, they're pretty close to providing us with
exactly what we need: per-thread memory permissions that we can use for
in-process isolation.
We've spent quite some effort up front thinking about potential attacks and
we're confident we can build something that will pose a meaningful boundary.

> On the x86 side, PKRU is a painfully special snowflake.  It's exposed in
> the "XSAVE" ABIs, but not actually managed *with* XSAVE in the kernel.
> This would be making it an even more special snowflake because it would
> need new altstack ABI and handling.

Most of the complexity in the signal handling proposal seems to come from the
saving/restoring pkru before/after the signal handler execution. However, this
is just nice to have. We just need the kernel to allow us to register
pkey-tagged memory as a sigaltstack, i.e. it shouldn't crash when trying to
write the register state to the stack. Everything else, we can do in userland.

> It probably *requires* turning off a bunch of
> syscalls (like io_uring) that folks kinda like in general.

Kind of. This approach only works in combination with an effort in userland to
restrict the syscalls. Though that doesn't mean you have to turn them off,
there's also the option of adding validation before it.
The same applies to the memory management syscalls in this patchset. We can add
validation for these in userland, but we're hoping to do it in kernel instead
for the reasons I mentioned before (e.g. they're very common and it's much
easier to validate in the kernel). Also subjectively it seems like a
nice property
if the pkey protections would not just apply to the memory contents, but also
apply to the metadata.
  

Thanks for bringing this to my attention. Regarding io_uring:
>
> io_uring fundamentally doesn't have the same checks.  The kernel side
> work can be done from an asynchronous kernel thread.  That kernel thread
> doesn't have a meaningful PKRU value.  The register has a value, but
> it's not really related to the userspace threads that are sending it
> requests.
>

I asked the question to the io_uring list [1].  io_uring thread will
respect PKRU of the user thread, async or not,  the behavior is the
same as regular syscall. There will be no issue for io_uring, i.e if
it decides to add more memory mapping syscalls to supported cmd in
future.

[1] https://lore.kernel.org/io-uring/CABi2SkUp45HEt7eQ6a47Z7b3LzW=4m3xAakG35os7puCO2dkng@mail.gmail.com/

Thanks.
-Jeff
  

Hi Dave,

Regarding siglongjmp:

On Thu, May 18, 2023 at 8:37 AM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 5/17/23 16:48, Jeff Xu wrote:
> > However, there are a few challenges I have not yet worked through.
> > First, the code needs to track when the first signaling entry occurs
> > (saving the PKRU register to the thread struct) and when it is last
> > returned (restoring the PKRU register from the thread struct).
>
> Would tracking signal "depth" work in the face of things like siglongjmp?
>
siglongjmp is interesting, thanks for bringing this up.

With siglongjmp, the thread doesn't go back to the place where signal is
raised, indeed, this idea of tracking the first signaling entry
doesn't work well with siglongjmp.

Thanks for your insight!
-Jeff


-Jeff
  

Hi Dave,
Thanks for feedback, regarding sigaltstack:

On Thu, May 18, 2023 at 2:04 PM Dave Hansen <dave.hansen@intel.com> wrote:
> >
> > Agreed on signaling handling is a tough part: what do you think about
> > the approach (modifying PKRU from saved stack after XSAVE), is there a
> > blocker ?
>
> Yes, signal entry and sigreturn are not necessarily symmetric so you
> can't really have a stack.
>

To clarify: I mean this option below:
- before get_sigframe(), save PKUR => tmp
- modify thread's PKRU so it can write to sigframe
- XSAVE
- save tmp => sigframe

I believe you proposed this in a previous discussion [1]:
and I quote here:
"There's a delicate point when building the stack frame that the
kernel would need to move over to the new PKRU value to build the
frame before it writes the *OLD* value to the frame.  But, it's far
from impossible."

sigreturn will restore thread's original PKRU from sigframe.
In case of asymmetrics caused by siglongjmp, user space doesn't call
sigreturn, the application needs to set desired PKRU before siglongjmp.

I think this solution should work.

[1] https://lore.kernel.org/lkml/b4f0dca5-1d15-67f7-4600-9a0a91e9d0bd@intel.com/

Best regards,
-Jeff
  

On 5/31/23 18:39, Jeff Xu wrote:
> I think this solution should work.

By "work" I think you mean that if laser-focused on this one use case,
without a full implementation, it looks like it can work.

I'll give you a "maybe" on that.

But that leaves out the bigger picture.  How many other things will we
regress doing this?  What's the opportunity cost?  What other things
will get neglected because we did _this_ one?  Are there more users out
there?

Looking at the big picture, I'm not convinced those tradeoffs are good
ones (and you're not going to find anyone that's a bigger fan of pkeys
than me).