pkeys: add API to switch to permissive pkey register

The API allows to switch to permissive pkey register that allows accesses to all PKEYs. This is functionality is already used in x86 signal delivery,

and will be needed for rseq.

Signed-off-by Dmitry Vyukov <[email protected]>

Cc Mathieu Desnoyers <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Cc Boqun Feng <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Aruna Ramakrishna <[email protected]>

Cc [email protected]

Cc [email protected]

---
  arch/x86/Kconfig             |  1 +
  arch/x86/include/asm/pkeys.h | 14 ++++++++++++++
  arch/x86/include/asm/pkru.h  | 10 +++++++---
  include/linux/pkeys.h        | 22 ++++++++++++++++++++++
  mm/Kconfig                   |  2 ++
  5 files changed, 46 insertions(+), 3 deletions(-)

arch/x86/Kconfig +1 -0

index be2c311f5118d..43af2840d098f 100644
@@ -1881,6 +1881,7 @@ config X86_INTEL_MEMORY_PROTECTION_KEYS
  	depends on X86_64 && (CPU_SUP_INTEL || CPU_SUP_AMD)
  	select ARCH_USES_HIGH_VMA_FLAGS
  	select ARCH_HAS_PKEYS
+	select ARCH_HAS_PERMISSIVE_PKEY
  	help
  	  Memory Protection Keys provides a mechanism for enforcing
  	  page-based protections, but without requiring modification of the

arch/x86/include/asm/pkeys.h +14 -0

index 2e6c04d8a45b4..1cacfe184b9d4 100644
@@ -2,6 +2,8 @@
  #ifndef _ASM_X86_PKEYS_H
  #define _ASM_X86_PKEYS_H
 
+#include "pkru.h"
+
  /*
   * If more than 16 keys are ever supported, a thorough audit
   * will be necessary to ensure that the types that store key

@@ -123,4 +125,16 @@ static inline int vma_pkey(struct vm_area_struct *vma)
  	return (vma->vm_flags & vma_pkey_mask) >> VM_PKEY_SHIFT;
  }
 
+typedef u32 pkey_reg_t;
+
+static inline pkey_reg_t switch_to_permissive_pkey_reg(void)
+{
+	return write_pkru(0);
+}
+
+static inline void write_pkey_reg(pkey_reg_t val)
+{
+	write_pkru(val);
+}
+
  #endif /*_ASM_X86_PKEYS_H */

arch/x86/include/asm/pkru.h +7 -3

index 74f0a2d34ffdd..b9bf9b7f2753b 100644
@@ -39,16 +39,20 @@ static inline u32 read_pkru(void)
  	return 0;
  }
 
-static inline void write_pkru(u32 pkru)
+static inline u32 write_pkru(u32 pkru)
  {
+	u32 old_pkru;
+
  	if (!cpu_feature_enabled(X86_FEATURE_OSPKE))
-		return;
+		return 0;
  	/*
  	 * WRPKRU is relatively expensive compared to RDPKRU.
  	 * Avoid WRPKRU when it would not change the value.
  	 */
-	if (pkru != rdpkru())
+	old_pkru = rdpkru();
+	if (pkru != old_pkru)
  		wrpkru(pkru);
+	return old_pkru;
  }
 
  static inline void pkru_write_default(void)

include/linux/pkeys.h +22 -0

index 86be8bf27b41b..d94a0ae7a784b 100644
@@ -48,4 +48,26 @@ static inline bool arch_pkeys_enabled(void)
 
  #endif /* ! CONFIG_ARCH_HAS_PKEYS */
 
+#ifndef CONFIG_ARCH_HAS_PERMISSIVE_PKEY
+
+/*
+ * Common name for value of the register that controls access to PKEYs
+ * (called differently on different arches: PKRU, POR, AMR).
+ */
+typedef int pkey_reg_t;
+
+/*
+ * Sets PKEY access register to the most permissive value that allows
+ * accesses to all PKEYs. Returns the current value of PKEY register.
+ * Code should generally arrange switching back to the old value
+ * using write_pkey_reg(old_value).
+ */
+static inline pkey_reg_t switch_to_permissive_pkey_reg(void)
+{
+	return 0;
+}
+
+static inline void write_pkey_reg(pkey_reg_t val) {}
+#endif /* ! CONFIG_ARCH_HAS_PERMISSIVE_PKEY */
+
  #endif /* _LINUX_PKEYS_H */

mm/Kconfig +2 -0

index 1b501db064172..9e874f7713a2b 100644
@@ -1147,6 +1147,8 @@ config ARCH_USES_HIGH_VMA_FLAGS
  	bool
  config ARCH_HAS_PKEYS
  	bool
+config ARCH_HAS_PERMISSIVE_PKEY
+	bool
 
  config ARCH_USES_PG_ARCH_2
  	bool

The API allows to switch to permissive pkey register that allows accesses to all PKEYs. This is functionality is already used in x86 signal delivery, and will be needed for rseq.

Signed-off-by Dmitry Vyukov <[email protected]>

Cc Mathieu Desnoyers <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Cc Boqun Feng <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Aruna Ramakrishna <[email protected]>

Cc [email protected]

Cc [email protected]

---
  arch/x86/Kconfig             |  1 +
  arch/x86/include/asm/pkeys.h | 14 ++++++++++++++
  arch/x86/include/asm/pkru.h  | 10 +++++++---
  include/linux/pkeys.h        | 22 ++++++++++++++++++++++
  mm/Kconfig                   |  2 ++
  5 files changed, 46 insertions(+), 3 deletions(-)

arch/x86/Kconfig +1 -0

index be2c311f5118d..43af2840d098f 100644
@@ -1881,6 +1881,7 @@ config X86_INTEL_MEMORY_PROTECTION_KEYS
  	depends on X86_64 && (CPU_SUP_INTEL || CPU_SUP_AMD)
  	select ARCH_USES_HIGH_VMA_FLAGS
  	select ARCH_HAS_PKEYS
+	select ARCH_HAS_PERMISSIVE_PKEY
  	help
  	  Memory Protection Keys provides a mechanism for enforcing
  	  page-based protections, but without requiring modification of the

arch/x86/include/asm/pkeys.h +14 -0

index 2e6c04d8a45b4..1cacfe184b9d4 100644
@@ -2,6 +2,8 @@
  #ifndef _ASM_X86_PKEYS_H
  #define _ASM_X86_PKEYS_H
 
+#include "pkru.h"
+
  /*
   * If more than 16 keys are ever supported, a thorough audit
   * will be necessary to ensure that the types that store key

@@ -123,4 +125,16 @@ static inline int vma_pkey(struct vm_area_struct *vma)
  	return (vma->vm_flags & vma_pkey_mask) >> VM_PKEY_SHIFT;
  }
 
+typedef u32 pkey_reg_t;
+
+static inline pkey_reg_t switch_to_permissive_pkey_reg(void)
+{
+	return write_pkru(0);
+}
+
+static inline void write_pkey_reg(pkey_reg_t val)
+{
+	write_pkru(val);
+}
+
  #endif /*_ASM_X86_PKEYS_H */

arch/x86/include/asm/pkru.h +7 -3

index 74f0a2d34ffdd..b9bf9b7f2753b 100644
@@ -39,16 +39,20 @@ static inline u32 read_pkru(void)
  	return 0;
  }
 
-static inline void write_pkru(u32 pkru)
+static inline u32 write_pkru(u32 pkru)
  {
+	u32 old_pkru;
+
  	if (!cpu_feature_enabled(X86_FEATURE_OSPKE))
-		return;
+		return 0;
  	/*
  	 * WRPKRU is relatively expensive compared to RDPKRU.
  	 * Avoid WRPKRU when it would not change the value.
  	 */
-	if (pkru != rdpkru())
+	old_pkru = rdpkru();
+	if (pkru != old_pkru)
  		wrpkru(pkru);
+	return old_pkru;
  }
 
  static inline void pkru_write_default(void)

include/linux/pkeys.h +22 -0

index 86be8bf27b41b..d94a0ae7a784b 100644
@@ -48,4 +48,26 @@ static inline bool arch_pkeys_enabled(void)
 
  #endif /* ! CONFIG_ARCH_HAS_PKEYS */
 
+#ifndef CONFIG_ARCH_HAS_PERMISSIVE_PKEY
+
+/*
+ * Common name for value of the register that controls access to PKEYs
+ * (called differently on different arches: PKRU, POR, AMR).
+ */
+typedef int pkey_reg_t;
+
+/*
+ * Sets PKEY access register to the most permissive value that allows
+ * accesses to all PKEYs. Returns the current value of PKEY register.
+ * Code should generally arrange switching back to the old value
+ * using write_pkey_reg(old_value).
+ */
+static inline pkey_reg_t switch_to_permissive_pkey_reg(void)
+{
+	return 0;
+}
+
+static inline void write_pkey_reg(pkey_reg_t val) {}
+#endif /* ! CONFIG_ARCH_HAS_PERMISSIVE_PKEY */
+
  #endif /* _LINUX_PKEYS_H */

mm/Kconfig +2 -0

index 1b501db064172..9e874f7713a2b 100644
@@ -1147,6 +1147,8 @@ config ARCH_USES_HIGH_VMA_FLAGS
  	bool
  config ARCH_HAS_PKEYS
  	bool
+config ARCH_HAS_PERMISSIVE_PKEY
+	bool
 
  config ARCH_USES_PG_ARCH_2
  	bool

Quoted 3 lines · Dmitry Vyukov

The API allows to switch to permissive pkey register that allows accesses to all PKEYs. This is functionality is already used in x86 signal delivery, and will be needed for rseq.

Reviewed-by Mathieu Desnoyers <[email protected]>

include/linux/pkeys.h +0 -0

index 86be8bf27b41b..d94a0ae7a784b 100644

On 2/17/25 03:07, Dmitry Vyukov wrote:
...

arch/x86/include/asm/pkeys.h static inline int vma_pkey(struct vm_area_struct *vma) · line 133

Quoted 13 lines

/*
* If more than 16 keys are ever supported, a thorough audit
* will be necessary to ensure that the types that store key

arch/x86/include/asm/pkeys.h +6 -0

@@ -123,4 +125,16 @@ static inline int vma_pkey(struct vm_area_struct *vma)
      return (vma->vm_flags & vma_pkey_mask) >> VM_PKEY_SHIFT;
 }
 
+typedef u32 pkey_reg_t;
+
+static inline pkey_reg_t switch_to_permissive_pkey_reg(void)
+{
+     return write_pkru(0);
+}

Just a naming nit: the "switch_to" and "reg" parts of this don't quite parse for me. This is writing a _value_ to a register. Maybe:

write_permissive_pkey_val()
or
set_permissive_pkey_val()

would be a better name.

Quoted 5 lines

include/linux/pkeys.h +0 -0

index 86be8bf27b41b..d94a0ae7a784b 100644

include/linux/pkeys.h static inline bool arch_pkeys_enabled(void) · line 57

Quoted 10 lines

include/linux/pkeys.h +7 -0

@@ -48,4 +48,26 @@ static inline bool arch_pkeys_enabled(void)
 
 #endif /* ! CONFIG_ARCH_HAS_PKEYS */
 
+#ifndef CONFIG_ARCH_HAS_PERMISSIVE_PKEY
+
+/*
+ * Common name for value of the register that controls access to PKEYs
+ * (called differently on different arches: PKRU, POR, AMR).
+ */
+typedef int pkey_reg_t;

Tiny nit: Should this be an unsigned type?

Nobody should be manipulating it, but I'd be surprised if any of the architectures have a signed type for it.

Quoted 9 lines

include/linux/pkeys.h static inline bool arch_pkeys_enabled(void) · line 57

Quoted 7 lines

include/linux/pkeys.h +7 -0

+#ifndef CONFIG_ARCH_HAS_PERMISSIVE_PKEY
+
+/*
+ * Common name for value of the register that controls access to PKEYs
+ * (called differently on different arches: PKRU, POR, AMR).
+ */
+typedef int pkey_reg_t;

Tiny nit: Should this be an unsigned type?

Quoted 2 lines

Nobody should be manipulating it, but I'd be surprised if any of the architectures have a signed type for it.

Since this is a stub type, can matching the real types do any good besides masking programming errors?
I've changed it to char in v4 to surface more potential programming errors.

Quoted 9 lines · Dmitry Vyukov

Quoted 9 lines

include/linux/pkeys.h static inline bool arch_pkeys_enabled(void) · line 57

Quoted 7 lines

include/linux/pkeys.h +7 -0

+#ifndef CONFIG_ARCH_HAS_PERMISSIVE_PKEY
+
+/*
+ * Common name for value of the register that controls access to PKEYs
+ * (called differently on different arches: PKRU, POR, AMR).
+ */
+typedef int pkey_reg_t;

Tiny nit: Should this be an unsigned type?

Quoted 5 lines

Quoted 2 lines

Nobody should be manipulating it, but I'd be surprised if any of the architectures have a signed type for it.

Since this is a stub type, can matching the real types do any good besides masking programming errors?
I've changed it to char in v4 to surface more potential programming errors.

I was more worried about copy-and-paste.

I agree that 'char' is the most fragile, but it's going to fragile in subtle ways and I'm not sure subtly broken code (whether it's expected to be compiled in or not) is great to have in a code base.

Do we have any types in sparse that would be appropriate? Could we mark the pkey_reg_t as being in a different address space when pkeys is compiled out so that sparse knows not to let it interact with other types?

Quoted 21 lines

Quoted 9 lines

Quoted 9 lines

include/linux/pkeys.h static inline bool arch_pkeys_enabled(void) · line 57

Quoted 7 lines

include/linux/pkeys.h +7 -0

+#ifndef CONFIG_ARCH_HAS_PERMISSIVE_PKEY
+
+/*
+ * Common name for value of the register that controls access to PKEYs
+ * (called differently on different arches: PKRU, POR, AMR).
+ */
+typedef int pkey_reg_t;

Tiny nit: Should this be an unsigned type?

Quoted 5 lines

Quoted 2 lines

Nobody should be manipulating it, but I'd be surprised if any of the architectures have a signed type for it.

Since this is a stub type, can matching the real types do any good besides masking programming errors?
I've changed it to char in v4 to surface more potential programming errors.

I was more worried about copy-and-paste.

I agree that 'char' is the most fragile, but it's going to fragile in subtle ways and I'm not sure subtly broken code (whether it's expected to be compiled in or not) is great to have in a code base.

Quoted 3 lines

Do we have any types in sparse that would be appropriate? Could we mark the pkey_reg_t as being in a different address space when pkeys is compiled out so that sparse knows not to let it interact with other types?

We could typedef it to some fake struct. Such a struct can't be passed to any function accepting an integer type (real pkeys), and any arithmetic won't work on it.

Quoted 3 lines

We could typedef it to some fake struct. Such a struct can't be passed to any function accepting an integer type (real pkeys), and any arithmetic won't work on it.

Dave, how should we proceed? Do you think this is a potential misuse worth preventing proactively? If yes, I can send v7.

Quoted 5 lines · Dmitry Vyukov

Quoted 3 lines

We could typedef it to some fake struct. Such a struct can't be passed to any function accepting an integer type (real pkeys), and any arithmetic won't work on it.

Dave, how should we proceed? Do you think this is a potential misuse worth preventing proactively? If yes, I can send v7.

I don't think it's worth doing anything too weird. Defining some kind of special struct would be pretty weird.

 arch/x86/Kconfig             |  1 +
 arch/x86/include/asm/pkeys.h | 14 ++++++++++++++
 arch/x86/include/asm/pkru.h  | 10 +++++++---
 include/linux/pkeys.h        | 22 ++++++++++++++++++++++
 mm/Kconfig                   |  2 ++

arch/x86/kernel/signal.c +2 -20

-/*
- * Enable all pkeys temporarily, so as to ensure that both the current
- * execution stack as well as the alternate signal stack are writeable.
- * The application can use any of the available pkeys to protect the
- * alternate signal stack, and we don't know which one it is, so enable
- * all. The PKRU register will be reset to init_pkru later in the flow,
- * in fpu__clear_user_states(), and it is the application's responsibility
- * to enable the appropriate pkey as the first step in the signal handler
- * so that the handler does not segfault.
- */
-static inline u32 sig_prepare_pkru(void)
-{
-	u32 orig_pkru = read_pkru();
-
-	write_pkru(0);
-	return orig_pkru;
-}
-
 /*
  * Set up a signal frame.
  */

@@ -157,8 +140,18 @@ get_sigframe(struct ksignal *ksig, struct pt_regs *regs, size_t frame_size,
 		return (void __user *)-1L;
 	}
 
-	/* Update PKRU to enable access to the alternate signal stack. */
-	pkru = sig_prepare_pkru();
+	/*
+	 * Enable all pkeys temporarily, so as to ensure that both the current

...

Quoted 29 lines

arch/x86/kernel/signal.c +2 -20

-/*
- * Enable all pkeys temporarily, so as to ensure that both the current
- * execution stack as well as the alternate signal stack are writeable.
- * The application can use any of the available pkeys to protect the
- * alternate signal stack, and we don't know which one it is, so enable
- * all. The PKRU register will be reset to init_pkru later in the flow,
- * in fpu__clear_user_states(), and it is the application's responsibility
- * to enable the appropriate pkey as the first step in the signal handler
- * so that the handler does not segfault.
- */
-static inline u32 sig_prepare_pkru(void)
-{
-     u32 orig_pkru = read_pkru();
-
-     write_pkru(0);
-     return orig_pkru;
-}
-
 /*
  * Set up a signal frame.
  */

@@ -157,8 +140,18 @@ get_sigframe(struct ksignal *ksig, struct pt_regs *regs, size_t frame_size,
              return (void __user *)-1L;
      }
 
-     /* Update PKRU to enable access to the alternate signal stack. */
-     pkru = sig_prepare_pkru();
+     /*
+      * Enable all pkeys temporarily, so as to ensure that both the current

arch/x86/kernel/signal.c get_sigframe(struct ksignal *ksig, struct pt_regs *regs, size_t frame_size, · line 154

Quoted 10 lines

arch/x86/kernel/signal.c +10 -0

+      * execution stack as well as the alternate signal stack are
+      * writeable. The application can use any of the available pkeys to
+      * protect the alternate signal stack, and we don't know which one it
+      * is, so enable all. The PKRU register will be reset to init_pkru
+      * later in the flow, in fpu__clear_user_states(), and it is the
+      * application's responsibility to enable the appropriate pkey as the
+      * first step in the signal handler so that the handler does not
+      * segfault.
+      */
+     pkru = switch_to_permissive_pkey_reg();

I think this hurts readability too much in the get_sigframe() code. On some level, it's silly to have a basically empty helper. But in this case, it does help keep the signal code readable.

In other words, this would be preferred:

/*
* Keep existing big comment
*/
static inline u32 sig_prepare_pkru(void)

{
        return switch_to_permissive_pkey_reg();
}

If an application registers rseq, and ever switches to another pkey protection (such that the rseq becomes inaccessible), then any context switch will cause failure in __rseq_handle_notify_resume() attempting to read/write struct rseq and/or rseq_cs. Since context switches are asynchronous and are outside of the application control (not part of the restricted code scope), temporarily switch to premissive pkey register to read/write rseq/rseq_cs, similarly

to signal delivery accesses to altstack.

Signed-off-by Dmitry Vyukov <[email protected]>

Cc Mathieu Desnoyers <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Cc Boqun Feng <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Aruna Ramakrishna <[email protected]>

Cc [email protected]

Cc [email protected]

---
  kernel/rseq.c | 36 ++++++++++++++++++++++++++++++++++++
  1 file changed, 36 insertions(+)

kernel/rseq.c +6 -0

index 442aba29bc4cf..31cd94b370ef3 100644
@@ -10,6 +10,7 @@
 
  #include <linux/sched.h>
  #include <linux/uaccess.h>
+#include <linux/pkeys.h>
  #include <linux/syscalls.h>
  #include <linux/rseq.h>
  #include <linux/types.h>

@@ -403,10 +404,13 @@ void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)
  {
  	struct task_struct *t = current;
  	int ret, sig;
+	pkey_reg_t saved;
+	bool switched_pkey_reg = false;
 
  	if (unlikely(t->flags & PF_EXITING))
  		return;
 
+retry:
  	/*
  	 * regs is NULL if and only if the caller is in a syscall path.  Skip
  	 * fixup and leave rseq_cs as is so that rseq_sycall() will detect and

@@ -419,9 +423,41 @@ void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)
  	}
  	if (unlikely(rseq_update_cpu_node_id(t)))
  		goto error;
+	if (switched_pkey_reg)
+		write_pkey_reg(saved);

kernel/rseq.c +8 -0

+	 */
+	if (!switched_pkey_reg) {
+		switched_pkey_reg = true;
+		saved = switch_to_permissive_pkey_reg();
+		goto retry;
+	} else {
+		write_pkey_reg(saved);
+	}
  	sig = ksig ? ksig->sig : 0;
  	force_sigsegv(sig);
  }

Quoted 7 lines · Dmitry Vyukov

If an application registers rseq, and ever switches to another pkey protection (such that the rseq becomes inaccessible), then any context switch will cause failure in __rseq_handle_notify_resume() attempting to read/write struct rseq and/or rseq_cs. Since context switches are asynchronous and are outside of the application control (not part of the restricted code scope), temporarily switch to premissive pkey register to read/write rseq/rseq_cs, similarly

permissive

Quoted 51 lines

to signal delivery accesses to altstack.

Signed-off-by Dmitry Vyukov <[email protected]>

Cc Mathieu Desnoyers <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Cc Boqun Feng <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Aruna Ramakrishna <[email protected]>

Cc [email protected]

Cc [email protected]

---
  kernel/rseq.c | 36 ++++++++++++++++++++++++++++++++++++
  1 file changed, 36 insertions(+)

kernel/rseq.c +6 -0

index 442aba29bc4cf..31cd94b370ef3 100644
@@ -10,6 +10,7 @@
 
  #include <linux/sched.h>
  #include <linux/uaccess.h>
+#include <linux/pkeys.h>
  #include <linux/syscalls.h>
  #include <linux/rseq.h>
  #include <linux/types.h>

@@ -403,10 +404,13 @@ void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)
  {
      struct task_struct *t = current;
      int ret, sig;
+     pkey_reg_t saved;
+     bool switched_pkey_reg = false;
 
      if (unlikely(t->flags & PF_EXITING))
              return;
 
+retry:
      /*
       * regs is NULL if and only if the caller is in a syscall path.  Skip
       * fixup and leave rseq_cs as is so that rseq_sycall() will detect and

@@ -419,9 +423,41 @@ void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)
      }
      if (unlikely(rseq_update_cpu_node_id(t)))
              goto error;
+     if (switched_pkey_reg)
+             write_pkey_reg(saved);

kernel/rseq.c void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs) · line 437

Quoted 10 lines

kernel/rseq.c +7 -0

      return;
 
  error:
+     /*
+      * If the application registers rseq, and ever switches to another
+      * pkey protection (such that the rseq becomes inaccessible), then
+      * any context switch will cause failure here attempting to read/write
+      * struct rseq and/or rseq_cs. Since context switches are
+      * asynchronous and are outside of the application control
+      * (not part of the restricted code scope), we temporarily switch

Remove "we".

kernel/rseq.c void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs) · line 438

Quoted 1 line

kernel/rseq.c +1 -0

+      * to premissive pkey register to read/write rseq/rseq_cs,

permissive

kernel/rseq.c void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs) · line 441

Quoted 3 lines

kernel/rseq.c +3 -0

+      * similarly to signal delivery accesses to altstack.
+      *
+      * We don't bother to check if the failure really happened due to

Remove "We".

kernel/rseq.c void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)

Quoted 5 lines

kernel/rseq.c +5 -0

+      * pkeys or not, since it does not matter (performance-wise and
+      * otherwise).
+      *
+      * If the restricted code installs rseq_cs in inaccessible to it
+      * due to pkeys memory,

This sentence should be reworded.

we still let this function read the rseq_cs.

kernel/rseq.c void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs) · line 447

Quoted 1 line

kernel/rseq.c +1 -0

+      * It's unclear what benefits the resticted code gets by doing this

restricted

kernel/rseq.c void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs) · line 452

Quoted 5 lines

kernel/rseq.c +5 -0

+      * (it probably already hijacked control flow at this point), and
+      * presumably any sane sandbox should prohibit restricted code
+      * from accessing struct rseq, and this is still better than
+      * terminating the app unconditionally (it always has a choice
+      * of not using rseq and pkeys together).

Note that because userspace can complete an rseq critical section without clearing the rseq_cs pointer, this could happen simply because the kernel is preempting the task after it has:

1) completed an rseq critical section, without clearing rseq_cs,
2) changed pkey.

So allowing this is important, and I would remove the comment about hijacked control flow and such. This can happen with normal use of the ABI.

Thanks,

Mathieu

kernel/rseq.c void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs) · line 463

Quoted 11 lines

kernel/rseq.c +8 -0

+      */
+     if (!switched_pkey_reg) {
+             switched_pkey_reg = true;
+             saved = switch_to_permissive_pkey_reg();
+             goto retry;
+     } else {
+             write_pkey_reg(saved);
+     }
      sig = ksig ? ksig->sig : 0;
      force_sigsegv(sig);
  }

--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

On Mon, 17 Feb 2025 at 21:21, Mathieu Desnoyers

Quoted 16 lines

we still let this function read the rseq_cs.

Quoted 1 line

+ * It's unclear what benefits the resticted code gets by doing this

restricted

kernel/rseq.c void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs) · line 452

Quoted 5 lines

kernel/rseq.c +5 -0

+      * (it probably already hijacked control flow at this point), and
+      * presumably any sane sandbox should prohibit restricted code
+      * from accessing struct rseq, and this is still better than
+      * terminating the app unconditionally (it always has a choice
+      * of not using rseq and pkeys together).

Note that because userspace can complete an rseq critical section without clearing the rseq_cs pointer, this could happen simply because the kernel is preempting the task after it has:

Quoted 6 lines

1) completed an rseq critical section, without clearing rseq_cs,
2) changed pkey.

So allowing this is important, and I would remove the comment about hijacked control flow and such. This can happen with normal use of the ABI.

Thanks for the review!

I've addressed all comments in the series in v2.

I've reworded this paragraph to simplify sentences, but I still kept the note aboud malicious rseq_cs.

If we would not be circumventing normal protection, then, yes, these cases would be the same. But since we are circumventing protection that otherwise exists, I think it's important to think about potentially malicious cases. In this context inaccessible rseq_cs values that resulted from normal execution are very different from malicious onces. Normal ones will point to a fixed set of real well-formed rseq_cs objects, while malicious ones may point to god-knows-where in an attempt of an attacker to do things we can't even imagine right now (e.g. rseq_cs overlapping with protected crypto keys).

It's as if a particular instance of copy_to_user would allow user-space to write arbitrary kernel memory, and memory of other processes circumventing all normal protections. In that context we would need to be very careful regarding what we actually allow.

Quoted 1 line · Dmitry Vyukov

On Mon, 17 Feb 2025 at 21:21, Mathieu Desnoyers

[...]

Quoted 41 lines

Quoted 16 lines

we still let this function read the rseq_cs.

Quoted 1 line

+ * It's unclear what benefits the resticted code gets by doing this

restricted

kernel/rseq.c void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs) · line 452

Quoted 5 lines

kernel/rseq.c +5 -0

+      * (it probably already hijacked control flow at this point), and
+      * presumably any sane sandbox should prohibit restricted code
+      * from accessing struct rseq, and this is still better than
+      * terminating the app unconditionally (it always has a choice
+      * of not using rseq and pkeys together).

Note that because userspace can complete an rseq critical section without clearing the rseq_cs pointer, this could happen simply because the kernel is preempting the task after it has:

Quoted 6 lines

1) completed an rseq critical section, without clearing rseq_cs,
2) changed pkey.

So allowing this is important, and I would remove the comment about hijacked control flow and such. This can happen with normal use of the ABI.

Thanks for the review!

I've addressed all comments in the series in v2.

I've reworded this paragraph to simplify sentences, but I still kept the note aboud malicious rseq_cs.

If we would not be circumventing normal protection, then, yes, these cases would be the same. But since we are circumventing protection that otherwise exists, I think it's important to think about potentially malicious cases. In this context inaccessible rseq_cs values that resulted from normal execution are very different from malicious onces. Normal ones will point to a fixed set of real well-formed rseq_cs objects, while malicious ones may point to god-knows-where in an attempt of an attacker to do things we can't even imagine right now (e.g. rseq_cs overlapping with protected crypto keys).

Quoted 4 lines

It's as if a particular instance of copy_to_user would allow user-space to write arbitrary kernel memory, and memory of other processes circumventing all normal protections. In that context we would need to be very careful regarding what we actually allow.

I'm considering that we should clear the rseq_cs pointer whenever userspace issues pkey_mprotect.

This would ensure that no legitimate scenario can trigger a load from a rseq_cs area which has the wrong pkey, and therefore we could accept read/write from/to a struct rseq which has the wrong pkey, but kill the process if trying to read/write from a struct rseq_cs with the wrong key. This would prevent userspace from making the kernel read/write from/to memory with the wrong pkey through a pointer it controls (rseq_cs pointer).

Thoughts ?

Quoted 61 lines · Mathieu Desnoyers

Quoted 1 line · Dmitry Vyukov

On Mon, 17 Feb 2025 at 21:21, Mathieu Desnoyers

[...]

Quoted 41 lines

Quoted 16 lines

we still let this function read the rseq_cs.

Quoted 1 line

+ * It's unclear what benefits the resticted code gets by doing this

restricted

kernel/rseq.c void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs) · line 452

Quoted 5 lines

kernel/rseq.c +5 -0

+      * (it probably already hijacked control flow at this point), and
+      * presumably any sane sandbox should prohibit restricted code
+      * from accessing struct rseq, and this is still better than
+      * terminating the app unconditionally (it always has a choice
+      * of not using rseq and pkeys together).

Note that because userspace can complete an rseq critical section without clearing the rseq_cs pointer, this could happen simply because the kernel is preempting the task after it has:

Quoted 6 lines

1) completed an rseq critical section, without clearing rseq_cs,
2) changed pkey.

So allowing this is important, and I would remove the comment about hijacked control flow and such. This can happen with normal use of the ABI.

Thanks for the review!

I've addressed all comments in the series in v2.

I've reworded this paragraph to simplify sentences, but I still kept the note aboud malicious rseq_cs.

If we would not be circumventing normal protection, then, yes, these cases would be the same. But since we are circumventing protection that otherwise exists, I think it's important to think about potentially malicious cases. In this context inaccessible rseq_cs values that resulted from normal execution are very different from malicious onces. Normal ones will point to a fixed set of real well-formed rseq_cs objects, while malicious ones may point to god-knows-where in an attempt of an attacker to do things we can't even imagine right now (e.g. rseq_cs overlapping with protected crypto keys).

Quoted 4 lines

It's as if a particular instance of copy_to_user would allow user-space to write arbitrary kernel memory, and memory of other processes circumventing all normal protections. In that context we would need to be very careful regarding what we actually allow.

I'm considering that we should clear the rseq_cs pointer whenever userspace issues pkey_mprotect.

This would ensure that no legitimate scenario can trigger a load from a rseq_cs area which has the wrong pkey, and therefore we could accept read/write from/to a struct rseq which has the wrong pkey, but kill the process if trying to read/write from a struct rseq_cs with the wrong key. This would prevent userspace from making the kernel read/write from/to memory with the wrong pkey through a pointer it controls (rseq_cs pointer).

Quoted 1 line

Thoughts ?

I am not following.

There are pkey_mprotect calls, then independently installs on rseq_cs pointers that happen concurrently and after pkey_mprotect, and independent set of pkey_set calls that happens concurrently and after the previous 2.
I don't see how doing something at the pkey_mprotect call for the single thread avoids any scenarios.

Moreover, pkey 0 is preinstalled for all pages, but access to it can be revoked in future.

Quoted 65 lines · Dmitry Vyukov

Quoted 61 lines · Mathieu Desnoyers

Quoted 1 line · Dmitry Vyukov

On Mon, 17 Feb 2025 at 21:21, Mathieu Desnoyers

[...]

Quoted 41 lines

Quoted 16 lines

we still let this function read the rseq_cs.

Quoted 1 line

+ * It's unclear what benefits the resticted code gets by doing this

restricted

kernel/rseq.c void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs) · line 452

Quoted 5 lines

kernel/rseq.c +5 -0

+      * (it probably already hijacked control flow at this point), and
+      * presumably any sane sandbox should prohibit restricted code
+      * from accessing struct rseq, and this is still better than
+      * terminating the app unconditionally (it always has a choice
+      * of not using rseq and pkeys together).

Note that because userspace can complete an rseq critical section without clearing the rseq_cs pointer, this could happen simply because the kernel is preempting the task after it has:

Quoted 6 lines

1) completed an rseq critical section, without clearing rseq_cs,
2) changed pkey.

So allowing this is important, and I would remove the comment about hijacked control flow and such. This can happen with normal use of the ABI.

Thanks for the review!

I've addressed all comments in the series in v2.

I've reworded this paragraph to simplify sentences, but I still kept the note aboud malicious rseq_cs.

If we would not be circumventing normal protection, then, yes, these cases would be the same. But since we are circumventing protection that otherwise exists, I think it's important to think about potentially malicious cases. In this context inaccessible rseq_cs values that resulted from normal execution are very different from malicious onces. Normal ones will point to a fixed set of real well-formed rseq_cs objects, while malicious ones may point to god-knows-where in an attempt of an attacker to do things we can't even imagine right now (e.g. rseq_cs overlapping with protected crypto keys).

Quoted 4 lines

It's as if a particular instance of copy_to_user would allow user-space to write arbitrary kernel memory, and memory of other processes circumventing all normal protections. In that context we would need to be very careful regarding what we actually allow.

I'm considering that we should clear the rseq_cs pointer whenever userspace issues pkey_mprotect.

This would ensure that no legitimate scenario can trigger a load from a rseq_cs area which has the wrong pkey, and therefore we could accept read/write from/to a struct rseq which has the wrong pkey, but kill the process if trying to read/write from a struct rseq_cs with the wrong key. This would prevent userspace from making the kernel read/write from/to memory with the wrong pkey through a pointer it controls (rseq_cs pointer).

Quoted 1 line

Thoughts ?

Quoted 8 lines

I am not following.

There are pkey_mprotect calls, then independently installs on rseq_cs pointers that happen concurrently and after pkey_mprotect, and independent set of pkey_set calls that happens concurrently and after the previous 2.
I don't see how doing something at the pkey_mprotect call for the single thread avoids any scenarios.

Hrm. Sorry, I mixed up pkey_set() vs pkey_mprotect(). What I had in mind was actually pkey_set(). And that would need to clear rseq_cs for all threads belonging to the process, which may not be straightforward because those could legitimately be inside a rseq critical section.

OK, let's try another approach: rather than kill the process if read/write of the rseq_cs area with the wrong key fails, could we simply clear the rseq_cs pointer in that case ? Technically there would be no legitimate use of this except for the case where it is meant to be lazily cleared.

Thanks,

Mathieu

Quoted 2 lines

Moreover, pkey 0 is preinstalled for all pages, but access to it can be revoked in future.

--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Quoted 81 lines · Mathieu Desnoyers

Quoted 65 lines · Dmitry Vyukov

Quoted 61 lines · Mathieu Desnoyers

Quoted 1 line · Dmitry Vyukov

On Mon, 17 Feb 2025 at 21:21, Mathieu Desnoyers

[...]

Quoted 41 lines

Quoted 16 lines

we still let this function read the rseq_cs.

Quoted 1 line

+ * It's unclear what benefits the resticted code gets by doing this

restricted

Review

Quoted 5 lines

(quoted diff) +5 -0

+      * (it probably already hijacked control flow at this point), and
+      * presumably any sane sandbox should prohibit restricted code
+      * from accessing struct rseq, and this is still better than
+      * terminating the app unconditionally (it always has a choice
+      * of not using rseq and pkeys together).

Note that because userspace can complete an rseq critical section without clearing the rseq_cs pointer, this could happen simply because the kernel is preempting the task after it has:

Quoted 6 lines

1) completed an rseq critical section, without clearing rseq_cs,
2) changed pkey.

So allowing this is important, and I would remove the comment about hijacked control flow and such. This can happen with normal use of the ABI.

Thanks for the review!

I've addressed all comments in the series in v2.

I've reworded this paragraph to simplify sentences, but I still kept the note aboud malicious rseq_cs.

If we would not be circumventing normal protection, then, yes, these cases would be the same. But since we are circumventing protection that otherwise exists, I think it's important to think about potentially malicious cases. In this context inaccessible rseq_cs values that resulted from normal execution are very different from malicious onces. Normal ones will point to a fixed set of real well-formed rseq_cs objects, while malicious ones may point to god-knows-where in an attempt of an attacker to do things we can't even imagine right now (e.g. rseq_cs overlapping with protected crypto keys).

Quoted 4 lines

It's as if a particular instance of copy_to_user would allow user-space to write arbitrary kernel memory, and memory of other processes circumventing all normal protections. In that context we would need to be very careful regarding what we actually allow.

I'm considering that we should clear the rseq_cs pointer whenever userspace issues pkey_mprotect.

This would ensure that no legitimate scenario can trigger a load from a rseq_cs area which has the wrong pkey, and therefore we could accept read/write from/to a struct rseq which has the wrong pkey, but kill the process if trying to read/write from a struct rseq_cs with the wrong key. This would prevent userspace from making the kernel read/write from/to memory with the wrong pkey through a pointer it controls (rseq_cs pointer).

Quoted 1 line

Thoughts ?

Quoted 8 lines

I am not following.

There are pkey_mprotect calls, then independently installs on rseq_cs pointers that happen concurrently and after pkey_mprotect, and independent set of pkey_set calls that happens concurrently and after the previous 2.
I don't see how doing something at the pkey_mprotect call for the single thread avoids any scenarios.

Hrm. Sorry, I mixed up pkey_set() vs pkey_mprotect(). What I had in mind was actually pkey_set(). And that would need to clear rseq_cs for all threads belonging to the process, which may not be straightforward because those could legitimately be inside a rseq critical section.

Quoted 5 lines

OK, let's try another approach: rather than kill the process if read/write of the rseq_cs area with the wrong key fails, could we simply clear the rseq_cs pointer in that case ? Technically there would be no legitimate use of this except for the case where it is meant to be lazily cleared.

This may work, but 2 concerns with this:
1. We don't know if the failure happened due to pkeys or not (at least not easily), and I am afraid of touching the logic for other failures. If the rseq_cs was a bogus pointer, or protected with normal mprotect, what does it mean? Are we masking a programming bug? Are we circumventing some other protections that were supposed to lead to the process termination?
2. This will complicate __rseq_handle_notify_resume() logic as it would need to handle failures when accessing rseq and rseq_cs differently (+plus there is signature check). The more complex the logic, the higher chances of adding a bug now or in future.

Quoted 90 lines · Dmitry Vyukov

Quoted 81 lines · Mathieu Desnoyers

Quoted 65 lines · Dmitry Vyukov

Quoted 61 lines · Mathieu Desnoyers

Quoted 1 line · Dmitry Vyukov

On Mon, 17 Feb 2025 at 21:21, Mathieu Desnoyers

[...]

Quoted 41 lines

Quoted 16 lines

we still let this function read the rseq_cs.

Quoted 1 line

+ * It's unclear what benefits the resticted code gets by doing this

restricted

Review

Quoted 5 lines

(quoted diff) +5 -0

+      * (it probably already hijacked control flow at this point), and
+      * presumably any sane sandbox should prohibit restricted code
+      * from accessing struct rseq, and this is still better than
+      * terminating the app unconditionally (it always has a choice
+      * of not using rseq and pkeys together).

Note that because userspace can complete an rseq critical section without clearing the rseq_cs pointer, this could happen simply because the kernel is preempting the task after it has:

Quoted 6 lines

1) completed an rseq critical section, without clearing rseq_cs,
2) changed pkey.

So allowing this is important, and I would remove the comment about hijacked control flow and such. This can happen with normal use of the ABI.

Thanks for the review!

I've addressed all comments in the series in v2.

I've reworded this paragraph to simplify sentences, but I still kept the note aboud malicious rseq_cs.

If we would not be circumventing normal protection, then, yes, these cases would be the same. But since we are circumventing protection that otherwise exists, I think it's important to think about potentially malicious cases. In this context inaccessible rseq_cs values that resulted from normal execution are very different from malicious onces. Normal ones will point to a fixed set of real well-formed rseq_cs objects, while malicious ones may point to god-knows-where in an attempt of an attacker to do things we can't even imagine right now (e.g. rseq_cs overlapping with protected crypto keys).

Quoted 4 lines

It's as if a particular instance of copy_to_user would allow user-space to write arbitrary kernel memory, and memory of other processes circumventing all normal protections. In that context we would need to be very careful regarding what we actually allow.

I'm considering that we should clear the rseq_cs pointer whenever userspace issues pkey_mprotect.

This would ensure that no legitimate scenario can trigger a load from a rseq_cs area which has the wrong pkey, and therefore we could accept read/write from/to a struct rseq which has the wrong pkey, but kill the process if trying to read/write from a struct rseq_cs with the wrong key. This would prevent userspace from making the kernel read/write from/to memory with the wrong pkey through a pointer it controls (rseq_cs pointer).

Quoted 1 line

Thoughts ?

Quoted 8 lines

I am not following.

There are pkey_mprotect calls, then independently installs on rseq_cs pointers that happen concurrently and after pkey_mprotect, and independent set of pkey_set calls that happens concurrently and after the previous 2.
I don't see how doing something at the pkey_mprotect call for the single thread avoids any scenarios.

Hrm. Sorry, I mixed up pkey_set() vs pkey_mprotect(). What I had in mind was actually pkey_set(). And that would need to clear rseq_cs for all threads belonging to the process, which may not be straightforward because those could legitimately be inside a rseq critical section.

Quoted 5 lines

OK, let's try another approach: rather than kill the process if read/write of the rseq_cs area with the wrong key fails, could we simply clear the rseq_cs pointer in that case ? Technically there would be no legitimate use of this except for the case where it is meant to be lazily cleared.

Quoted 11 lines

This may work, but 2 concerns with this:
1. We don't know if the failure happened due to pkeys or not (at least not easily), and I am afraid of touching the logic for other failures. If the rseq_cs was a bogus pointer, or protected with normal mprotect, what does it mean? Are we masking a programming bug? Are we circumventing some other protections that were supposed to lead to the process termination?
2. This will complicate __rseq_handle_notify_resume() logic as it would need to handle failures when accessing rseq and rseq_cs differently (+plus there is signature check). The more complex the logic, the higher chances of adding a bug now or in future.

Do you see any problem with the current code? What exactly is it? If not, can we merge it as is?

error:

kernel/rseq.c +13 -0

+	/*
+	 * If the application registers rseq, and ever switches to another
+	 * pkey protection (such that the rseq becomes inaccessible), then
+	 * any context switch will cause failure here attempting to read/write
+	 * struct rseq and/or rseq_cs. Since context switches are
+	 * asynchronous and are outside of the application control
+	 * (not part of the restricted code scope), we temporarily switch
+	 * to premissive pkey register to read/write rseq/rseq_cs,
+	 * similarly to signal delivery accesses to altstack.
+	 *
+	 * We don't bother to check if the failure really happened due to
+	 * pkeys or not, since it does not matter (performance-wise and
+	 * otherwise).

On 2/17/25 03:07, Dmitry Vyukov wrote:

This code flow is a bit hard to follow with the retry and all.

I think the assumption here is that overwriting the pkey register is too slow for the fast path. Instead, in the slow error path, there is a one-time operation to make the register permissive and retry.

I guess it's your rseq code. But I'd probably just put the switch_to_permissive_pkey_reg()/write_pkey_reg() in the fast/common path for simplicity unless I knew it was causing a measurable performance problem.

I agree that switching to permissive key in the fast path would be simpler. AFAIU, the switch_to_permissive_pkey_reg() is only a pkey read when the key is already permissive.

Quoted 3 lines · Mathieu Desnoyers

I agree that switching to permissive key in the fast path would be simpler. AFAIU, the switch_to_permissive_pkey_reg() is only a pkey read when the key is already permissive.

Unfortunately, on x86, PKRU is almost never in its permissive state. We chose a policy (stored in the global init_pkru_value variable) that allows R/W access to pkey 0, but disables access to everything else. It's 0xfffffff5, IIRC.

This ensures deny-by-default behavior and ensures that threads cloned off long ago don't have a dangerous PKRU value for newly-allocated and pkey-protected memory.

If I had a time machine, it'd be interesting to go back and try to make PKRU's default value be all 0's and also represent the logically most restrictive value.

Quoted 14 lines · Dave Hansen

Quoted 3 lines · Mathieu Desnoyers

I agree that switching to permissive key in the fast path would be simpler. AFAIU, the switch_to_permissive_pkey_reg() is only a pkey read when the key is already permissive.

Unfortunately, on x86, PKRU is almost never in its permissive state. We chose a policy (stored in the global init_pkru_value variable) that allows R/W access to pkey 0, but disables access to everything else. It's 0xfffffff5, IIRC.

This ensures deny-by-default behavior and ensures that threads cloned off long ago don't have a dangerous PKRU value for newly-allocated and pkey-protected memory.

Quoted 3 lines

If I had a time machine, it'd be interesting to go back and try to make PKRU's default value be all 0's and also represent the logically most restrictive value.

Can we assume (or require) that struct rseq and struct rseq_cs reside in pkey-0 memory ?

In that case, we could add something to the pkey API that switches to a permissive state only if pkey 0 cannot be accessed.

Therefore it would only trigger a pkey read in the common case, and issue a pkey write only if pkey 0 is not accessible.

Quoted 19 lines · Mathieu Desnoyers

Quoted 17 lines · Dave Hansen

Quoted 3 lines · Mathieu Desnoyers

I agree that switching to permissive key in the fast path would be simpler. AFAIU, the switch_to_permissive_pkey_reg() is only a pkey read when the key is already permissive.

Unfortunately, on x86, PKRU is almost never in its permissive state. We chose a policy (stored in the global init_pkru_value variable) that allows R/W access to pkey 0, but disables access to everything else. It's 0xfffffff5, IIRC.

This ensures deny-by-default behavior and ensures that threads cloned off long ago don't have a dangerous PKRU value for newly-allocated and pkey-protected memory.

If I had a time machine, it'd be interesting to go back and try to make PKRU's default value be all 0's and also represent the logically most restrictive value.

Quoted 2 lines

Can we assume (or require) that struct rseq and struct rseq_cs reside in pkey-0 memory ?

Maybe. Signal stacks are _practically_ only able to use pkey-0. You can technically protect them with anything you want and then WRPKRU as the first instruction once you hop into the signal handler (since instruction fetches aren't affected by x86 pkeys), but I seriously doubt anybody would go to the trouble.

Quoted 6 lines

In that case, we could add something to the pkey API that switches to a permissive state only if pkey 0 cannot be accessed.

Therefore it would only trigger a pkey read in the common case, and issue a pkey write only if pkey 0 is not accessible.

I think that's a sane policy. An rseq access can happen at any time (from the app's perspective) so the access would theoretically be done with a random PKRU value from a random point in the thread's lifetime.

But it is a different policy that we've chosen with signals and "remote" accesses, which is to just ignore pkeys entirely.

I don't have a strong opinion. It's hard to balance performance and consistency with the other ABI here.

Quoted 20 lines · Dave Hansen

Quoted 19 lines · Mathieu Desnoyers

Quoted 17 lines · Dave Hansen

Quoted 3 lines · Mathieu Desnoyers

I agree that switching to permissive key in the fast path would be simpler. AFAIU, the switch_to_permissive_pkey_reg() is only a pkey read when the key is already permissive.

Unfortunately, on x86, PKRU is almost never in its permissive state. We chose a policy (stored in the global init_pkru_value variable) that allows R/W access to pkey 0, but disables access to everything else. It's 0xfffffff5, IIRC.

This ensures deny-by-default behavior and ensures that threads cloned off long ago don't have a dangerous PKRU value for newly-allocated and pkey-protected memory.

If I had a time machine, it'd be interesting to go back and try to make PKRU's default value be all 0's and also represent the logically most restrictive value.

Quoted 8 lines

Quoted 2 lines

Can we assume (or require) that struct rseq and struct rseq_cs reside in pkey-0 memory ?

Maybe. Signal stacks are _practically_ only able to use pkey-0. You can technically protect them with anything you want and then WRPKRU as the first instruction once you hop into the signal handler (since instruction fetches aren't affected by x86 pkeys), but I seriously doubt anybody would go to the trouble.

And that would not work on arm64, AFAIU arm64 POR_EL0 also applies to instruction fetches, which somewhat prevents what can be done for signal handlers if the code intends to be portable.

Quoted 10 lines

Quoted 6 lines

In that case, we could add something to the pkey API that switches to a permissive state only if pkey 0 cannot be accessed.

Therefore it would only trigger a pkey read in the common case, and issue a pkey write only if pkey 0 is not accessible.

I think that's a sane policy. An rseq access can happen at any time (from the app's perspective) so the access would theoretically be done with a random PKRU value from a random point in the thread's lifetime.

Quoted 5 lines

But it is a different policy that we've chosen with signals and "remote" accesses, which is to just ignore pkeys entirely.

I don't have a strong opinion. It's hard to balance performance and consistency with the other ABI here.

Because the rseq return to userspace handler is called on every return to userspace after a task is scheduled back after preemption, I am concerned about the overhead that would be added by a WRPKRU on the fast-path, given that it acts as as barrier against speculation. Issuing WRPKRU only after checking that pkey-0 is not accessible appears to be moving the overhead to a much less common case.

And perhaps if we end up observing that for some reasons either the sigframe and/or "remote" pkey accesses really must use pkey-0 as well to work in real-life, then we could make them require pkey-0. That's of course assuming it would cause no observable ABI breakage. Once advantage here would be to speed up signal handler delivery.

I have no clue what a "remote" pkey access is. Is this the io_uring use-case ?

Thanks,

Mathieu

Quoted 8 lines

Quoted 1 line

Because the rseq return to userspace handler is called on every return to userspace after a task is scheduled back after preemption, I am concerned about the overhead that would be added by a WRPKRU on the fast-path, given that it acts as as barrier against speculation. Issuing WRPKRU only after checking that pkey-0 is not accessible appears to be moving the overhead to a much less common case.

Actually, we should distinguish between two accesses here:

A) loads/stores from/to struct rseq

B) loads from struct rseq_cs (only happens on rseq abort)

(A) is a fast-path executed on return to userspace after a preemption. In order to make it fast, we could require that struct rseq is pkey-0 and typically skip any WRPKRU for this access when pkey-0 is already accessible. We can add a check on rseq registration to make sure that struct rseq is indeed pkey-0, and reject it with an error if not. This should help make the ABI robust and less error-prone.

Now for (B), it's a slow path. When we observe that rseq->rseq_cs is not NULL, we can simply override with a permissive pkey to make sure the rseq_cs access will work.

Thoughts ?

Quoted 26 lines · Mathieu Desnoyers

Quoted 8 lines

Quoted 1 line

Because the rseq return to userspace handler is called on every return to userspace after a task is scheduled back after preemption, I am concerned about the overhead that would be added by a WRPKRU on the fast-path, given that it acts as as barrier against speculation. Issuing WRPKRU only after checking that pkey-0 is not accessible appears to be moving the overhead to a much less common case.

Actually, we should distinguish between two accesses here:

A) loads/stores from/to struct rseq

B) loads from struct rseq_cs (only happens on rseq abort)

(A) is a fast-path executed on return to userspace after a preemption. In order to make it fast, we could require that struct rseq is pkey-0 and typically skip any WRPKRU for this access when pkey-0 is already accessible. We can add a check on rseq registration to make sure that struct rseq is indeed pkey-0, and reject it with an error if not. This should help make the ABI robust and less error-prone.

Now for (B), it's a slow path. When we observe that rseq->rseq_cs is not NULL, we can simply override with a permissive pkey to make sure the rseq_cs access will work.

Quoted 1 line

Thoughts ?

I think this will be the first ABI which is explicitly pkey-0-only. I suspect there are a few more of these that are implicit but we just haven't found them yet.

I wouldn't have any objections about doing this, especially given sanity checking at rseq registration.

I have no clue what a "remote" pkey access is. Is this the io_uring use-case ?

Add a test that ensures that PKEY-protected struct rseq works and does not lead to process kills.

Signed-off-by Dmitry Vyukov <[email protected]>

Cc Mathieu Desnoyers <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Cc Boqun Feng <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Aruna Ramakrishna <[email protected]>

Cc [email protected]

Cc [email protected]

---
  tools/testing/selftests/rseq/Makefile    |  2 +-
  tools/testing/selftests/rseq/pkey_test.c | 61 ++++++++++++++++++++++++
  2 files changed, 62 insertions(+), 1 deletion(-)

tools/testing/selftests/rseq/Makefile +1 -1

index 5a3432fceb586..9111d25fea3af 100644
@@ -16,7 +16,7 @@ OVERRIDE_TARGETS = 1
 
  TEST_GEN_PROGS = basic_test basic_percpu_ops_test basic_percpu_ops_mm_cid_test param_test \
  		param_test_benchmark param_test_compare_twice param_test_mm_cid \
-		param_test_mm_cid_benchmark param_test_mm_cid_compare_twice
+		param_test_mm_cid_benchmark param_test_mm_cid_compare_twice pkey_test
 
  TEST_GEN_PROGS_EXTENDED = librseq.so
 

tools/testing/selftests/rseq/pkey_test.c new file +61 -0

new file mode 100644
index 0000000000000..ba5c1f6e99ab5
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: LGPL-2.1
+/*
+ * Ensure that rseq works when rseq data is protected with PKEYs.
+ */
+
+#define _GNU_SOURCE
+#include <err.h>
+#include <errno.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <sys/syscall.h>
+#include <unistd.h>
+
+int main(int argc, char **argv)
+{
+	void *rseq;
+	unsigned long page_size;
+	int pkey, i;
+
+	pkey = pkey_alloc(0, 0);
+	if (pkey == -1) {
+		printf("[SKIP]\tKernel does not support PKEYs: %s\n",
+			strerror(errno));
+		return 0;
+	}
+
+	/*
+	 * Prevent glibc from registering own struct rseq.
+	 * We need to know the rseq address to protect it, but also we need
+	 * it to be placed on own page that does not contain other data
+	 * (e.g. errno).
+	 */
+	if (!getenv("RSEQ_TEST_REEXECED")) {
+		setenv("RSEQ_TEST_REEXECED", "1", 1);
+		setenv("GLIBC_TUNABLES", "glibc.pthread.rseq=0", 1);
+		if (execvpe(argv[0], argv, environ))
+			err(1, "execvpe failed");
+	}
+
+	page_size = getpagesize();
+	rseq = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
+		MAP_ANON | MAP_PRIVATE, -1, 0);
+	if (rseq == MAP_FAILED)
+		err(1, "mmap failed");
+	if (pkey_mprotect(rseq, page_size, PROT_READ | PROT_WRITE, pkey))
+		err(1, "pkey_mprotect failed");
+	if (syscall(__NR_rseq, rseq, 32, 0, 0))
+		err(1, "rseq failed");
+	if (pkey_set(pkey, PKEY_DISABLE_ACCESS))
+		err(1, "pkey_set failed");
+
+	/*
+	 * If the kernel misbehaves, context switches in the following loop
+	 * will kill the process with SIGSEGV.
+	 */
+	for (i = 0; i < 10; i++)
+		usleep(100);
+	return 0;
+}

+++ b/tools/testing/selftests/rseq/pkey_test.c

Quoted 1 line · Dmitry Vyukov

+++ b/tools/testing/selftests/rseq/pkey_test.c

There's also a:

tools/testing/selftests/mm/protection_keys.c

The main thing that will get you is testing with a bunch of different pkey values and also a few different memory types including huge pages. It also keeps an eye on PKRU consistency by keeping a shadow. So if, for instance, the rseq code forgot to restore PKRU, that code would be likely to catch it. It's caught a few bugs during development for me when PKRU was getting wrongly-munged.

But, I'm not picky about selftests. Any test is better than no test. So, whatever you decide to do:

Acked-by Dave Hansen <[email protected]>