[PATCH 1/4] pkeys: add API to switch to permissive pkey register

[PATCH 1/4] pkeys: add API to switch to permissive pkey register

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

Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
 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(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index be2c311f5118d..43af2840d098f 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -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
diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
index 2e6c04d8a45b4..1cacfe184b9d4 100644
--- a/arch/x86/include/asm/pkeys.h
+++ b/arch/x86/include/asm/pkeys.h
@@ -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 */
diff --git a/arch/x86/include/asm/pkru.h b/arch/x86/include/asm/pkru.h
index 74f0a2d34ffdd..b9bf9b7f2753b 100644
--- a/arch/x86/include/asm/pkru.h
+++ b/arch/x86/include/asm/pkru.h
@@ -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)
diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
index 86be8bf27b41b..d94a0ae7a784b 100644
--- a/include/linux/pkeys.h
+++ b/include/linux/pkeys.h
@@ -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 */
diff --git a/mm/Kconfig b/mm/Kconfig
index 1b501db064172..9e874f7713a2b 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -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
-- 
2.48.1.601.g30ceb7b040-goog

[PATCH 2/4] x86/signal: Use switch_to_permissive_pkey_reg() helper

[Dmitry Vyukov]

Use the new switch_to_permissive_pkey_reg() helper instead of the
custom code. No functional changes intended.

Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
 arch/x86/kernel/signal.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 5f441039b5725..b753de278257a 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -28,6 +28,7 @@
 #include <linux/entry-common.h>
 #include <linux/syscalls.h>
 #include <linux/rseq.h>
+#include <linux/pkeys.h>
 
 #include <asm/processor.h>
 #include <asm/ucontext.h>
@@ -60,24 +61,6 @@ static inline int is_x32_frame(struct ksignal *ksig)
 		ksig->ka.sa.sa_flags & SA_X32_ABI;
 }
 
-/*
- * 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
+	 * 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();
 	/* save i387 and extended state */
 	if (!copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_size, pkru)) {
 		/*
-- 
2.48.1.601.g30ceb7b040-goog

[PATCH 3/4] rseq: Make rseq work with protection keys

[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
to signal delivery accesses to altstack.

Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
 kernel/rseq.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/kernel/rseq.c b/kernel/rseq.c
index 442aba29bc4cf..31cd94b370ef3 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -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);
 	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
+	 * 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).
+	 *
+	 * If the restricted code installs rseq_cs in inaccessible to it
+	 * due to pkeys memory, we still let this function read the rseq_cs.
+	 * It's unclear what benefits the resticted code gets by doing this
+	 * (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).
+	 */
+	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);
 }
-- 
2.48.1.601.g30ceb7b040-goog

[PATCH 4/4] selftests/rseq: Add test for rseq+pkeys

[Dmitry Vyukov]

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

Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
 tools/testing/selftests/rseq/Makefile    |  2 +-
 tools/testing/selftests/rseq/pkey_test.c | 61 ++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/rseq/Makefile b/tools/testing/selftests/rseq/Makefile
index 5a3432fceb586..9111d25fea3af 100644
--- a/tools/testing/selftests/rseq/Makefile
+++ b/tools/testing/selftests/rseq/Makefile
@@ -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
 
diff --git a/tools/testing/selftests/rseq/pkey_test.c b/tools/testing/selftests/rseq/pkey_test.c
new file mode 100644
index 0000000000000..ba5c1f6e99ab5
--- /dev/null
+++ b/tools/testing/selftests/rseq/pkey_test.c
@@ -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;
+}
-- 
2.48.1.601.g30ceb7b040-goog

Re: [PATCH 1/4] pkeys: add API to switch to permissive pkey register

[Mathieu Desnoyers]

On 2025-02-17 06:07, Dmitry Vyukov wrote:
> 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,

"This functionality"

Other than that,

Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Thanks,

Mathieu

> and will be needed for rseq.
> 
> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>   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(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index be2c311f5118d..43af2840d098f 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -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
> diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
> index 2e6c04d8a45b4..1cacfe184b9d4 100644
> --- a/arch/x86/include/asm/pkeys.h
> +++ b/arch/x86/include/asm/pkeys.h
> @@ -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 */
> diff --git a/arch/x86/include/asm/pkru.h b/arch/x86/include/asm/pkru.h
> index 74f0a2d34ffdd..b9bf9b7f2753b 100644
> --- a/arch/x86/include/asm/pkru.h
> +++ b/arch/x86/include/asm/pkru.h
> @@ -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)
> diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
> index 86be8bf27b41b..d94a0ae7a784b 100644
> --- a/include/linux/pkeys.h
> +++ b/include/linux/pkeys.h
> @@ -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 */
> diff --git a/mm/Kconfig b/mm/Kconfig
> index 1b501db064172..9e874f7713a2b 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -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


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

Re: [PATCH 1/4] pkeys: add API to switch to permissive pkey register

[Mathieu Desnoyers]

On 2025-02-17 06:07, Dmitry Vyukov wrote:
> 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 <mathieu.desnoyers@efficios.com>

> 
> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>   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(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index be2c311f5118d..43af2840d098f 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -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
> diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
> index 2e6c04d8a45b4..1cacfe184b9d4 100644
> --- a/arch/x86/include/asm/pkeys.h
> +++ b/arch/x86/include/asm/pkeys.h
> @@ -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 */
> diff --git a/arch/x86/include/asm/pkru.h b/arch/x86/include/asm/pkru.h
> index 74f0a2d34ffdd..b9bf9b7f2753b 100644
> --- a/arch/x86/include/asm/pkru.h
> +++ b/arch/x86/include/asm/pkru.h
> @@ -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)
> diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
> index 86be8bf27b41b..d94a0ae7a784b 100644
> --- a/include/linux/pkeys.h
> +++ b/include/linux/pkeys.h
> @@ -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 */
> diff --git a/mm/Kconfig b/mm/Kconfig
> index 1b501db064172..9e874f7713a2b 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -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


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

Re: [PATCH 1/4] pkeys: add API to switch to permissive pkey register

[Mathieu Desnoyers]

On 2025-02-17 15:08, Mathieu Desnoyers wrote:
> On 2025-02-17 06:07, Dmitry Vyukov wrote:
>> 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 <mathieu.desnoyers@efficios.com>

Sorry for double-review, I'll jump to the next patch.

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

Re: [PATCH 3/4] rseq: Make rseq work with protection keys

[Mathieu Desnoyers]

On 2025-02-17 06:07, Dmitry Vyukov wrote:
> 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

> to signal delivery accesses to altstack.
> 
> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>   kernel/rseq.c | 36 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 36 insertions(+)
> 
> diff --git a/kernel/rseq.c b/kernel/rseq.c
> index 442aba29bc4cf..31cd94b370ef3 100644
> --- a/kernel/rseq.c
> +++ b/kernel/rseq.c
> @@ -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);
>   	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".

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

permissive

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

Remove "We".

> +	 * 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.
> +	 * It's unclear what benefits the resticted code gets by doing this

restricted

> +	 * (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


> +	 */
> +	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

Re: [PATCH 4/4] selftests/rseq: Add test for rseq+pkeys

[Mathieu Desnoyers]

On 2025-02-17 06:07, Dmitry Vyukov wrote:
> Add a test that ensures that PKEY-protected struct rseq works
> and does not lead to process kills.

Rather than depend on the glibc rseq tunable to disable glibc
rseq support, you could simply setup a rseq_cs in a separate
page and get the kernel to hit _that_ page through the
rseq_cs pointer.

Thanks,

Mathieu

> 
> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>   tools/testing/selftests/rseq/Makefile    |  2 +-
>   tools/testing/selftests/rseq/pkey_test.c | 61 ++++++++++++++++++++++++
>   2 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/rseq/Makefile b/tools/testing/selftests/rseq/Makefile
> index 5a3432fceb586..9111d25fea3af 100644
> --- a/tools/testing/selftests/rseq/Makefile
> +++ b/tools/testing/selftests/rseq/Makefile
> @@ -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
>   
> diff --git a/tools/testing/selftests/rseq/pkey_test.c b/tools/testing/selftests/rseq/pkey_test.c
> new file mode 100644
> index 0000000000000..ba5c1f6e99ab5
> --- /dev/null
> +++ b/tools/testing/selftests/rseq/pkey_test.c
> @@ -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;
> +}


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

Re: [PATCH 3/4] rseq: Make rseq work with protection keys

[Dmitry Vyukov]

On Mon, 17 Feb 2025 at 21:21, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> On 2025-02-17 06:07, Dmitry Vyukov wrote:
> > 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
>
> > to signal delivery accesses to altstack.
> >
> > Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Cc: Boqun Feng <boqun.feng@gmail.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>
> > Cc: x86@kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >   kernel/rseq.c | 36 ++++++++++++++++++++++++++++++++++++
> >   1 file changed, 36 insertions(+)
> >
> > diff --git a/kernel/rseq.c b/kernel/rseq.c
> > index 442aba29bc4cf..31cd94b370ef3 100644
> > --- a/kernel/rseq.c
> > +++ b/kernel/rseq.c
> > @@ -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);
> >       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".
>
> > +      * to premissive pkey register to read/write rseq/rseq_cs,
>
> permissive
>
> > +      * similarly to signal delivery accesses to altstack.
> > +      *
> > +      * We don't bother to check if the failure really happened due to
>
> Remove "We".
>
> > +      * 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.
> > +      * It's unclear what benefits the resticted code gets by doing this
>
> restricted
>
> > +      * (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 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.


> Thanks,
>
> Mathieu
>
>
> > +      */
> > +     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

Re: [PATCH 3/4] rseq: Make rseq work with protection keys

[Mathieu Desnoyers]

On 2025-02-18 02:55, Dmitry Vyukov wrote:
> On Mon, 17 Feb 2025 at 21:21, Mathieu Desnoyers
[...]
>>
>>    we still let this function read the rseq_cs.
>>> +      * It's unclear what benefits the resticted code gets by doing this
>>
>> restricted
>>
>>> +      * (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 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.

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 ?

Thanks,

Mathieu

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

Re: [PATCH 3/4] rseq: Make rseq work with protection keys

[Dmitry Vyukov]

On Tue, 18 Feb 2025 at 15:57, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> On 2025-02-18 02:55, Dmitry Vyukov wrote:
> > On Mon, 17 Feb 2025 at 21:21, Mathieu Desnoyers
> [...]
> >>
> >>    we still let this function read the rseq_cs.
> >>> +      * It's unclear what benefits the resticted code gets by doing this
> >>
> >> restricted
> >>
> >>> +      * (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 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.
>
> 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 ?

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.

Re: [PATCH 3/4] rseq: Make rseq work with protection keys

[Mathieu Desnoyers]

On 2025-02-18 10:10, Dmitry Vyukov wrote:
> On Tue, 18 Feb 2025 at 15:57, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> On 2025-02-18 02:55, Dmitry Vyukov wrote:
>>> On Mon, 17 Feb 2025 at 21:21, Mathieu Desnoyers
>> [...]
>>>>
>>>>     we still let this function read the rseq_cs.
>>>>> +      * It's unclear what benefits the resticted code gets by doing this
>>>>
>>>> restricted
>>>>
>>>>> +      * (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 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.
>>
>> 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 ?
> 
> 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

> 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

Re: [PATCH 3/4] rseq: Make rseq work with protection keys

[Dmitry Vyukov]

On Tue, 18 Feb 2025 at 16:27, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> On 2025-02-18 10:10, Dmitry Vyukov wrote:
> > On Tue, 18 Feb 2025 at 15:57, Mathieu Desnoyers
> > <mathieu.desnoyers@efficios.com> wrote:
> >>
> >> On 2025-02-18 02:55, Dmitry Vyukov wrote:
> >>> On Mon, 17 Feb 2025 at 21:21, Mathieu Desnoyers
> >> [...]
> >>>>
> >>>>     we still let this function read the rseq_cs.
> >>>>> +      * It's unclear what benefits the resticted code gets by doing this
> >>>>
> >>>> restricted
> >>>>
> >>>>> +      * (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 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.
> >>
> >> 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 ?
> >
> > 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.

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.


> Thanks,
>
> Mathieu
>
> > 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

Re: [PATCH 3/4] rseq: Make rseq work with protection keys

[Dmitry Vyukov]

On Tue, 18 Feb 2025 at 16:37, Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Tue, 18 Feb 2025 at 16:27, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
> >
> > On 2025-02-18 10:10, Dmitry Vyukov wrote:
> > > On Tue, 18 Feb 2025 at 15:57, Mathieu Desnoyers
> > > <mathieu.desnoyers@efficios.com> wrote:
> > >>
> > >> On 2025-02-18 02:55, Dmitry Vyukov wrote:
> > >>> On Mon, 17 Feb 2025 at 21:21, Mathieu Desnoyers
> > >> [...]
> > >>>>
> > >>>>     we still let this function read the rseq_cs.
> > >>>>> +      * It's unclear what benefits the resticted code gets by doing this
> > >>>>
> > >>>> restricted
> > >>>>
> > >>>>> +      * (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 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.
> > >>
> > >> 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 ?
> > >
> > > 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.
>
> 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?

Re: [PATCH 2/4] x86/signal: Use switch_to_permissive_pkey_reg() helper

[Dave Hansen]

...
> -/*
> - * 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
> +	 * 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();
}

Re: [PATCH 1/4] pkeys: add API to switch to permissive pkey register

[Dave Hansen]

On 2/17/25 03:07, Dmitry Vyukov wrote:
...
>  /*
>   * 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);
> +}

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.

> diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
> index 86be8bf27b41b..d94a0ae7a784b 100644
> --- a/include/linux/pkeys.h
> +++ b/include/linux/pkeys.h
> @@ -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.

Re: [PATCH 3/4] rseq: Make rseq work with protection keys

[Dave Hansen]

On 2/17/25 03:07, Dmitry Vyukov wrote:
>  
>  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
> +	 * 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).
> +	 *
> +	 * If the restricted code installs rseq_cs in inaccessible to it
> +	 * due to pkeys memory, we still let this function read the rseq_cs.
> +	 * It's unclear what benefits the resticted code gets by doing this
> +	 * (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).
> +	 */

I would trim this comment down. I'd keep the discussion more in the
changelog than in here. I'd also suggest breaking out the spell checker.

Also, as usual, changing this to imperative voice makes it more compact too:

	Don't bother to check if the failure really happened due to
	pkeys or not, since it does not matter (performance-wise and
	otherwise).

Basically, zap the "we's".

> +	if (!switched_pkey_reg) {
> +		switched_pkey_reg = true;
> +		saved = switch_to_permissive_pkey_reg();
> +		goto retry;
> +	} else {
> +		write_pkey_reg(saved);
> +	}

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.

In either case, it would be great to comment that design choice in the
changelog.

Oh, and cover letters are most appreciated for these kinds of things.
I'd normally reply to the cover letter and say this, but I'll put it
here instead:

The series overall looks fine. It just needs a few cosmetic tweaks.

I don't see any Cc:stable@ or Fixes: tags. Is this a bug fix that you
want backported? If so, those tags would be appropriate and it woudl be
appreciated if you could dig out what it actually fixes.

Re: [PATCH 4/4] selftests/rseq: Add test for rseq+pkeys

[Dave Hansen]

On 2/17/25 03:07, Dmitry Vyukov wrote:
> +++ 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 <dave.hansen@linux.intel.com>

Re: [PATCH 1/4] pkeys: add API to switch to permissive pkey register

[Dave Hansen]

On 2/17/25 03:07, Dmitry Vyukov wrote:
>  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 ++

One last thought...

This is touching x86, mm and rseq code. So, who's going to send it to Linus?

I'm happy to send this in one of the tip branches and send it upstream.
But I'd also be happy to provide acks and have it go up with other rseq
bits.

Does anyone have a preference?

Re: [PATCH 3/4] rseq: Make rseq work with protection keys

[Mathieu Desnoyers]

On 2025-02-21 12:17, Dave Hansen wrote:
> 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.

I'd favor the simpler approach unless performance end up being an
issue.

Thanks,

Mathieu


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

Re: [PATCH 3/4] rseq: Make rseq work with protection keys

[Mathieu Desnoyers]

On 2025-02-21 06:22, Dmitry Vyukov wrote:
> On Tue, 18 Feb 2025 at 16:37, Dmitry Vyukov <dvyukov@google.com> wrote:
>>
>> On Tue, 18 Feb 2025 at 16:27, Mathieu Desnoyers
>> <mathieu.desnoyers@efficios.com> wrote:
>>>
>>> On 2025-02-18 10:10, Dmitry Vyukov wrote:
>>>> On Tue, 18 Feb 2025 at 15:57, Mathieu Desnoyers
>>>> <mathieu.desnoyers@efficios.com> wrote:
>>>>>
>>>>> On 2025-02-18 02:55, Dmitry Vyukov wrote:
>>>>>> On Mon, 17 Feb 2025 at 21:21, Mathieu Desnoyers
>>>>> [...]
>>>>>>>
>>>>>>>      we still let this function read the rseq_cs.
>>>>>>>> +      * It's unclear what benefits the resticted code gets by doing this
>>>>>>>
>>>>>>> restricted
>>>>>>>
>>>>>>>> +      * (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 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.
>>>>>
>>>>> 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 ?
>>>>
>>>> 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.
>>
>> 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?

After discussion with Dave Hansen, it appears that pkey is not really
meant to be secure against a malicious actor nowadays, it's more a
page protection tool that helps identifying bad memory use.

So I don't think we need to care about the security-related concerns
I raised above.

Please see my comment in reply to Dave about making the code simpler.
Once that is done, please send an updated patch, and the rest should
be OK.

Thanks,

Mathieu

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

Re: [PATCH 3/4] rseq: Make rseq work with protection keys

[Dave Hansen]

On 2/21/25 11:38, Mathieu Desnoyers wrote:
> 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.

Re: [PATCH 3/4] rseq: Make rseq work with protection keys

[Mathieu Desnoyers]

On 2025-02-21 14:48, Dave Hansen wrote:
> On 2/21/25 11:38, Mathieu Desnoyers wrote:
>> 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.

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.

Thoughts ?

Thanks,

Mathieu



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

Re: [PATCH 3/4] rseq: Make rseq work with protection keys

[Dave Hansen]

On 2/21/25 12:05, Mathieu Desnoyers wrote:
> On 2025-02-21 14:48, Dave Hansen wrote:
>> On 2/21/25 11:38, Mathieu Desnoyers wrote:
>>> 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.
> 
> 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.

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

Re: [PATCH 3/4] rseq: Make rseq work with protection keys

[Mathieu Desnoyers]

On 2025-02-21 15:50, Dave Hansen wrote:
> On 2/21/25 12:05, Mathieu Desnoyers wrote:
>> On 2025-02-21 14:48, Dave Hansen wrote:
>>> On 2/21/25 11:38, Mathieu Desnoyers wrote:
>>>> 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.
>>
>> 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.

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

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

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

Re: [PATCH 3/4] rseq: Make rseq work with protection keys

[Mathieu Desnoyers]

On 2025-02-21 16:11, Mathieu Desnoyers wrote:
> On 2025-02-21 15:50, Dave Hansen wrote:
>> On 2/21/25 12:05, Mathieu Desnoyers wrote:
>>> On 2025-02-21 14:48, Dave Hansen wrote:
>>>> On 2/21/25 11:38, Mathieu Desnoyers wrote:
>>>>> 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.
>>>
>>> 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.
> 
>>
>>> 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.
> 
> 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 ?

Thanks,

Mathieu

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


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

Re: [PATCH 3/4] rseq: Make rseq work with protection keys

[Dave Hansen]

On 2/21/25 13:11, Mathieu Desnoyers wrote:
> I have no clue what a "remote" pkey access is. Is this the io_uring
> use-case ?

Yeah, that's one of them.

It's basically all of the *_user_pages_remote() sites which are places
that the kernel does userspace memory manipulation but doesn't have a
good user *context* with which to do it, thus no good PKRU value.

The "remote" nomenclature is because these are mostly (all??) one
process accessing another process without that other process really
being involved.

Re: [PATCH 3/4] rseq: Make rseq work with protection keys

[Dave Hansen]

On 2/21/25 13:36, Mathieu Desnoyers wrote:
>>>
>>
>> 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 ?
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.

Re: [PATCH 2/4] x86/signal: Use switch_to_permissive_pkey_reg() helper

[Dmitry Vyukov]

On Fri, 21 Feb 2025 at 17:26, Dave Hansen <dave.hansen@intel.com> wrote:
>
> ...
> > -/*
> > - * 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
> > +      * 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();
> }

Makes sense. Done in v3.

Re: [PATCH 4/4] selftests/rseq: Add test for rseq+pkeys

[Dmitry Vyukov]

On Fri, 21 Feb 2025 at 18:24, Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 2/17/25 03:07, Dmitry Vyukov wrote:
> > +++ 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 <dave.hansen@linux.intel.com>

I dunno. It also depends on the rseq helper header and helper source
file that is currently compiled into tests only by rseq Makefile.
v2 of the test was already checking that the pkey register wasn't messed with.
But I had to rework the test for v3/v4.

Re: [PATCH 1/4] pkeys: add API to switch to permissive pkey register

[Dmitry Vyukov]

On Fri, 21 Feb 2025 at 18:01, Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 2/17/25 03:07, Dmitry Vyukov wrote:
> ...
> >  /*
> >   * 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);
> > +}
>
> 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.

Changed them to write_permissive_pkey_val/write_pkey_val in v4.

> > diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
> > index 86be8bf27b41b..d94a0ae7a784b 100644
> > --- a/include/linux/pkeys.h
> > +++ b/include/linux/pkeys.h
> > @@ -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.

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.

Re: [PATCH 3/4] rseq: Make rseq work with protection keys

[Dmitry Vyukov]

On Fri, 21 Feb 2025 at 22:45, Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 2/21/25 13:36, Mathieu Desnoyers wrote:
> >>>
> >>
> >> 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 ?
> 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.


Thanks for the thoughtful review.

I've tried to incorporate all suggestions in v4:
https://lore.kernel.org/all/68427864e0ca38af06482c96728216c3e0973418.1740403209.git.dvyukov@google.com/T/#m431c13b6140e447d41d228fb942d9e4b8a89874a

Yes, the intention for doing the switch only on the error path was to
avoid WRPKRU on the hot path.

For my current use case (at least how I currently have it implemented)
struct rseq and altstack are indeed protected with pkey 0. So I added
a new enable_zero_pkey_val() function that lazily enables only pkey 0.

Also added Fixes tag (for everything except for signal.c refactoring).
The "fixed" commit seems to be the original rseq patch d7822b1e24f2
("rseq: Introduce restartable sequences system call") b/c PKEYs
introduction is older.

I also had to significantly rework the test to make it work with rseq
protected by pkey 0 (which means we need to revoke access to pkey 0,
which means stack/tls/errno also become inaccessible).

Please take another look at v4.

I don't have preference as to how this should get into the Linus tree.
Hopefully the changes are not too pervasive for all subsystems to
cause massive conflicts when merging.

Re: [PATCH 1/4] pkeys: add API to switch to permissive pkey register

[Dave Hansen]

On 2/24/25 05:25, Dmitry Vyukov wrote:
>>> +#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.
> 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?

Re: [PATCH 1/4] pkeys: add API to switch to permissive pkey register

[Dmitry Vyukov]

On Tue, 25 Feb 2025 at 17:15, Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 2/24/25 05:25, Dmitry Vyukov wrote:
> >>> +#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.
> > 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?

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.

Re: [PATCH 1/4] pkeys: add API to switch to permissive pkey register

[Dmitry Vyukov]

On Tue, 25 Feb 2025 at 22:56, Dmitry Vyukov <dvyukov@google.com> wrote:
> > >>> +#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.
> > > 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?
>
> 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.

Re: [PATCH 1/4] pkeys: add API to switch to permissive pkey register

[Dave Hansen]

On 2/26/25 02:00, Dmitry Vyukov wrote:
>> 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.

Re: [PATCH 1/4] pkeys: add API to switch to permissive pkey register

[Dmitry Vyukov]

On Wed, 26 Feb 2025 at 18:21, Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 2/26/25 02:00, Dmitry Vyukov wrote:
> >> 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.

Isn't sparse-specific attributes, or address spaces more weird that a
normal C struct?
If you suggest a concrete type I can re-send the series with the
change. But for now I am going to leave it as it is. A fact that a bug
may be introduced does not mean it will be introduced (there is an
infinite set of possible bugs that can be introduced in future, and
generally there is no reliable way to ensure no bugs will be
introduced, or we would have 0 bugs in the kernel :)).