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

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

[Dmitry Vyukov]

The API allows to switch to permissive pkey register that allows accesses
to all PKEYs, and to a value that allows acccess to the 0 (default) PKEY.
This 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
Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Fixes: d7822b1e24f2 ("rseq: Introduce restartable sequences system call")

---
Changes in v4:
 - Added Fixes tag

Changes in v3:
 - Renamed API functions to write_permissive_pkey_val/write_pkey_val
 - Added enable_zero_pkey_val for rseq
 - Added Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Changes in v2:
 - Fixed typo in commit description
---
 arch/x86/Kconfig             |  1 +
 arch/x86/include/asm/pkeys.h | 33 +++++++++++++++++++++++++++++++++
 arch/x86/include/asm/pkru.h  | 10 +++++++---
 include/linux/pkeys.h        | 31 +++++++++++++++++++++++++++++++
 mm/Kconfig                   |  2 ++
 5 files changed, 74 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..d6e35ab5c3d22 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,35 @@ 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 write_permissive_pkey_val(void)
+{
+	return write_pkru(0);
+}
+
+static inline pkey_reg_t enable_zero_pkey_val(void)
+{
+	u32 pkru;
+
+	if (!cpu_feature_enabled(X86_FEATURE_OSPKE))
+		return 0;
+	/*
+	 * WRPKRU is relatively expensive compared to RDPKRU,
+	 * avoid it if possible.
+	 */
+	pkru = rdpkru();
+	if ((pkru & (PKRU_AD_BIT|PKRU_WD_BIT)) != 0)
+		wrpkru(pkru & ~(PKRU_AD_BIT|PKRU_WD_BIT));
+	return pkru;
+
+
+	return write_pkru(0);
+}
+
+static inline void write_pkey_val(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..262d60f6a15f8 100644
--- a/include/linux/pkeys.h
+++ b/include/linux/pkeys.h
@@ -48,4 +48,35 @@ 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 char 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_val(old_value).
+ */
+static inline pkey_reg_t write_permissive_pkey_val(void)
+{
+	return 0;
+}
+
+/*
+ * Sets PKEY access register to a value that allows access to the 0 (default)
+ * PKEY. Returns the current value of PKEY register.
+ */
+static inline pkey_reg_t enable_zero_pkey_val(void)
+{
+	return 0;
+}
+
+static inline void write_pkey_val(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 v4 2/4] x86/signal: Use write_permissive_pkey_val() 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

---
Changes in v3:
 - restore sig_prepare_pkru with the large comment and
   make it call the new write_permissive_pkey_val
---
 arch/x86/kernel/signal.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 5f441039b5725..27a66a0697dd2 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>
@@ -72,10 +73,7 @@ static inline int is_x32_frame(struct ksignal *ksig)
  */
 static inline u32 sig_prepare_pkru(void)
 {
-	u32 orig_pkru = read_pkru();
-
-	write_pkru(0);
-	return orig_pkru;
+	return write_permissive_pkey_val();
 }
 
 /*
-- 
2.48.1.601.g30ceb7b040-goog

[PATCH v4 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
pkey value that allows access to the 0 (default) PKEY.

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
Fixes: d7822b1e24f2 ("rseq: Introduce restartable sequences system call")

---
Changes in v4:
 - Added Fixes tag

Changes in v3:
 - simplify control flow to always enable access to 0 pkey

Changes in v2:
 - fixed typos and reworded the comment
---
 kernel/rseq.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/kernel/rseq.c b/kernel/rseq.c
index 2cb16091ec0ae..9d9c976d3b78c 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>
@@ -402,11 +403,19 @@ static int rseq_ip_fixup(struct pt_regs *regs)
 void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)
 {
 	struct task_struct *t = current;
+	pkey_reg_t saved_pkey;
 	int ret, sig;
 
 	if (unlikely(t->flags & PF_EXITING))
 		return;
 
+	/*
+	 * Enable access to the default (0) pkey in case the thread has
+	 * currently disabled access to it and struct rseq/rseq_cs has
+	 * 0 pkey assigned (the only supported value for now).
+	 */
+	saved_pkey = enable_zero_pkey_val();
+
 	/*
 	 * 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 +428,11 @@ void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)
 	}
 	if (unlikely(rseq_update_cpu_node_id(t)))
 		goto error;
+	write_pkey_val(saved_pkey);
 	return;
 
 error:
+	write_pkey_val(saved_pkey);
 	sig = ksig ? ksig->sig : 0;
 	force_sigsegv(sig);
 }
-- 
2.48.1.601.g30ceb7b040-goog

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

[Dmitry Vyukov]

Add a test that ensures that PKEY-protected struct rseq_cs
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
Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
Fixes: d7822b1e24f2 ("rseq: Introduce restartable sequences system call")

---
Changes in v4:
 - Added Fixes tag

Changes in v3:
 - added Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
 - rework the test to work when only pkey 0 is supported for rseq

Changes in v2:
 - change test to install protected rseq_cs instead of rseq
---
 tools/testing/selftests/rseq/Makefile    |  2 +-
 tools/testing/selftests/rseq/pkey_test.c | 99 ++++++++++++++++++++++++
 tools/testing/selftests/rseq/rseq.h      |  1 +
 3 files changed, 101 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..8752ecea21ba8
--- /dev/null
+++ b/tools/testing/selftests/rseq/pkey_test.c
@@ -0,0 +1,99 @@
+// SPDX-License-Identifier: LGPL-2.1
+/*
+ * Ensure that rseq works when rseq data is inaccessible due to 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 <ucontext.h>
+#include <unistd.h>
+
+#include "rseq.h"
+#include "rseq-abi.h"
+
+int pkey;
+ucontext_t ucp0, ucp1;
+
+void coroutine(void)
+{
+	int i, orig_pk0, old_pk0, old_pk1, pk0, pk1;
+	/*
+	 * When we disable access to pkey 0, globals and TLS become
+	 * inaccessible too, so we need to tread carefully.
+	 * Pkey is global so we need to copy it to onto stack.
+	 * If ts is not volatile, then compiler may try to init it
+	 * by loading a global 16-byte value.
+	 */
+	volatile int pk = pkey;
+	volatile struct timespec ts;
+
+	orig_pk0 = pkey_get(0);
+	if (pkey_set(0, PKEY_DISABLE_ACCESS))
+		err(1, "pkey_set failed");
+	old_pk0 = pkey_get(0);
+	old_pk1 = pkey_get(pk);
+
+	/*
+	 * If the kernel misbehaves, context switches in the following loop
+	 * will terminate the process with SIGSEGV.
+	 */
+	ts.tv_sec = 0;
+	ts.tv_nsec = 10 * 1000;
+	/*
+	 * Trigger preemption w/o accessing TLS.
+	 * Note that glibc's usleep touches errno always.
+	 */
+	for (i = 0; i < 10; i++)
+		syscall(SYS_clock_nanosleep, CLOCK_MONOTONIC, 0, &ts, NULL);
+
+	pk0 = pkey_get(0);
+	pk1 = pkey_get(pk);
+	if (pkey_set(0, orig_pk0))
+		err(1, "pkey_set failed");
+
+	/*
+	 * Ensure that the kernel has restored the previous value of pkeys
+	 * register after changing them.
+	 */
+	if (old_pk0 != pk0)
+		errx(1, "pkey 0 changed %d->%d", old_pk0, pk0);
+	if (old_pk1 != pk1)
+		errx(1, "pkey 1 changed %d->%d", old_pk1, pk1);
+
+	swapcontext(&ucp1, &ucp0);
+	abort();
+}
+
+int main(int argc, char **argv)
+{
+	pkey = pkey_alloc(0, 0);
+	if (pkey == -1) {
+		printf("[SKIP]\tKernel does not support PKEYs: %s\n",
+			strerror(errno));
+		return 0;
+	}
+
+	if (rseq_register_current_thread())
+		err(1, "rseq_register_current_thread failed");
+
+	if (getcontext(&ucp1))
+		err(1, "getcontext failed");
+	ucp1.uc_stack.ss_size = getpagesize() * 4;
+	ucp1.uc_stack.ss_sp = mmap(NULL, ucp1.uc_stack.ss_size,
+		PROT_READ | PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0);
+	if (ucp1.uc_stack.ss_sp == MAP_FAILED)
+		err(1, "mmap failed");
+	if (pkey_mprotect(ucp1.uc_stack.ss_sp, ucp1.uc_stack.ss_size,
+			PROT_READ | PROT_WRITE, pkey))
+		err(1, "pkey_mprotect failed");
+	makecontext(&ucp1, coroutine, 0);
+	if (swapcontext(&ucp0, &ucp1))
+		err(1, "swapcontext failed");
+	return 0;
+}
diff --git a/tools/testing/selftests/rseq/rseq.h b/tools/testing/selftests/rseq/rseq.h
index ba424ce80a719..65da4a727c550 100644
--- a/tools/testing/selftests/rseq/rseq.h
+++ b/tools/testing/selftests/rseq/rseq.h
@@ -8,6 +8,7 @@
 #ifndef RSEQ_H
 #define RSEQ_H
 
+#include <assert.h>
 #include <stdint.h>
 #include <stdbool.h>
 #include <pthread.h>
-- 
2.48.1.601.g30ceb7b040-goog

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

[Dmitry Vyukov]

On Mon, 24 Feb 2025 at 14:21, Dmitry Vyukov <dvyukov@google.com> 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 enable access
> to 0 (default) PKEY to read/write rseq/rseq_cs.
> 0 is the only PKEY supported for rseq for now.
> Theoretically other PKEYs can be supported, but it's unclear
> how/if that can work. So for now we don't support that to simplify
> code.
>
> Dmitry Vyukov (4):
>   pkeys: add API to switch to permissive/zero pkey register
>   x86/signal: Use write_permissive_pkey_val() helper
>   rseq: Make rseq work with protection keys
>   selftests/rseq: Add test for rseq+pkeys
>
>  arch/x86/Kconfig                         |  1 +
>  arch/x86/include/asm/pkeys.h             | 33 ++++++++
>  arch/x86/include/asm/pkru.h              | 10 ++-
>  arch/x86/kernel/signal.c                 |  6 +-
>  include/linux/pkeys.h                    | 31 ++++++++
>  kernel/rseq.c                            | 11 +++
>  mm/Kconfig                               |  2 +
>  tools/testing/selftests/rseq/Makefile    |  2 +-
>  tools/testing/selftests/rseq/pkey_test.c | 99 ++++++++++++++++++++++++
>  tools/testing/selftests/rseq/rseq.h      |  1 +
>  10 files changed, 188 insertions(+), 8 deletions(-)
>  create mode 100644 tools/testing/selftests/rseq/pkey_test.c
>
>
> base-commit: d082ecbc71e9e0bf49883ee4afd435a77a5101b6
> --
> 2.48.1.601.g30ceb7b040-goog

+LKML/x86 lists for archiving purposes

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

[Mathieu Desnoyers]

On 2025-02-24 08:20, Dmitry Vyukov wrote:
> The API allows to switch to permissive pkey register that allows accesses
> to all PKEYs, and to a value that allows acccess to the 0 (default) PKEY.
> This functionality is already used in x86 signal delivery, and will be
> needed for rseq.

AFAIU the signal delivery uses the "allow access to all PKEYs"
approach only, not the "allow access to 0-pkey". It would be
good to clarify this in the commit message.

> 
> 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
> Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Fixes: d7822b1e24f2 ("rseq: Introduce restartable sequences system call")
> 
> ---
> Changes in v4:
>   - Added Fixes tag
> 
> Changes in v3:
>   - Renamed API functions to write_permissive_pkey_val/write_pkey_val
>   - Added enable_zero_pkey_val for rseq
>   - Added Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> 
> Changes in v2:
>   - Fixed typo in commit description
> ---
>   arch/x86/Kconfig             |  1 +
>   arch/x86/include/asm/pkeys.h | 33 +++++++++++++++++++++++++++++++++
>   arch/x86/include/asm/pkru.h  | 10 +++++++---
>   include/linux/pkeys.h        | 31 +++++++++++++++++++++++++++++++
>   mm/Kconfig                   |  2 ++
>   5 files changed, 74 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..d6e35ab5c3d22 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,35 @@ 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 write_permissive_pkey_val(void)
> +{
> +	return write_pkru(0);
> +}
> +
> +static inline pkey_reg_t enable_zero_pkey_val(void)
> +{
> +	u32 pkru;
> +
> +	if (!cpu_feature_enabled(X86_FEATURE_OSPKE))
> +		return 0;
> +	/*
> +	 * WRPKRU is relatively expensive compared to RDPKRU,
> +	 * avoid it if possible.
> +	 */
> +	pkru = rdpkru();
> +	if ((pkru & (PKRU_AD_BIT|PKRU_WD_BIT)) != 0)
> +		wrpkru(pkru & ~(PKRU_AD_BIT|PKRU_WD_BIT));
> +	return pkru;
> +
> +
> +	return write_pkru(0);

This is dead code. What I am missing ?

With those fixed, please keep my reviewed-by :)

Thanks,

Mathieu

> +}
> +
> +static inline void write_pkey_val(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..262d60f6a15f8 100644
> --- a/include/linux/pkeys.h
> +++ b/include/linux/pkeys.h
> @@ -48,4 +48,35 @@ 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 char 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_val(old_value).
> + */
> +static inline pkey_reg_t write_permissive_pkey_val(void)
> +{
> +	return 0;
> +}
> +
> +/*
> + * Sets PKEY access register to a value that allows access to the 0 (default)
> + * PKEY. Returns the current value of PKEY register.
> + */
> +static inline pkey_reg_t enable_zero_pkey_val(void)
> +{
> +	return 0;
> +}
> +
> +static inline void write_pkey_val(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 v4 2/4] x86/signal: Use write_permissive_pkey_val() helper

[Mathieu Desnoyers]

On 2025-02-24 08:20, Dmitry Vyukov wrote:
> 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
> 
> ---
> Changes in v3:
>   - restore sig_prepare_pkru with the large comment and
>     make it call the new write_permissive_pkey_val
> ---
>   arch/x86/kernel/signal.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index 5f441039b5725..27a66a0697dd2 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>
> @@ -72,10 +73,7 @@ static inline int is_x32_frame(struct ksignal *ksig)
>    */
>   static inline u32 sig_prepare_pkru(void)
>   {
> -	u32 orig_pkru = read_pkru();
> -
> -	write_pkru(0);
> -	return orig_pkru;
> +	return write_permissive_pkey_val();

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

Note that I would be curious to see if we could get away with changing
this to enable_zero_pkey_val() without breaking any real-life user.
AFAIU this would remove a WRPKRU from signal delivery, but would require
the signal frames to be pkey-0, which is technically be a new ABI
constraint never expressed before.

Thanks,

Mathieu


>   }
>   
>   /*


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

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

[Mathieu Desnoyers]

On 2025-02-24 08:20, 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
> pkey value that allows access to the 0 (default) PKEY.

This is a good start, but the plan Dave and I discussed went further
than this. Those additions are needed:

1) Add validation at rseq registration that the struct rseq is indeed
    pkey-0 memory (return failure if not).

2) The pkey-0 requirement is only for struct rseq, which we can check
    for at rseq registration, and happens to be the fast path. For struct
    rseq_cs, this is not the same tradeoff: we cannot easily check its
    associated pkey because the rseq_cs pointer is updated by userspace
    when entering a critical section. But the good news is that reading
    the content of struct rseq_cs is *not* a fast-path: it's only done
    when preempting/delivering a signal over a thread which has a
    non-NULL rseq_cs pointer.

    Therefore reading the struct rseq_cs content should be done with
    write_permissive_pkey_val(), giving access to all pkeys.

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
> Fixes: d7822b1e24f2 ("rseq: Introduce restartable sequences system call")
> 
> ---
> Changes in v4:
>   - Added Fixes tag
> 
> Changes in v3:
>   - simplify control flow to always enable access to 0 pkey
> 
> Changes in v2:
>   - fixed typos and reworded the comment
> ---
>   kernel/rseq.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/kernel/rseq.c b/kernel/rseq.c
> index 2cb16091ec0ae..9d9c976d3b78c 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>
> @@ -402,11 +403,19 @@ static int rseq_ip_fixup(struct pt_regs *regs)
>   void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)
>   {
>   	struct task_struct *t = current;
> +	pkey_reg_t saved_pkey;
>   	int ret, sig;
>   
>   	if (unlikely(t->flags & PF_EXITING))
>   		return;
>   
> +	/*
> +	 * Enable access to the default (0) pkey in case the thread has
> +	 * currently disabled access to it and struct rseq/rseq_cs has
> +	 * 0 pkey assigned (the only supported value for now).
> +	 */
> +	saved_pkey = enable_zero_pkey_val();
> +
>   	/*
>   	 * 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 +428,11 @@ void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)
>   	}
>   	if (unlikely(rseq_update_cpu_node_id(t)))
>   		goto error;
> +	write_pkey_val(saved_pkey);
>   	return;
>   
>   error:
> +	write_pkey_val(saved_pkey);
>   	sig = ksig ? ksig->sig : 0;
>   	force_sigsegv(sig);
>   }


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

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

[Mathieu Desnoyers]

On 2025-02-24 08:20, Dmitry Vyukov wrote:
> Add a test that ensures that PKEY-protected struct rseq_cs
> 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
> Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
> Fixes: d7822b1e24f2 ("rseq: Introduce restartable sequences system call")
> 
> ---
> Changes in v4:
>   - Added Fixes tag
> 
> Changes in v3:
>   - added Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
>   - rework the test to work when only pkey 0 is supported for rseq
> 
> Changes in v2:
>   - change test to install protected rseq_cs instead of rseq
> ---
>   tools/testing/selftests/rseq/Makefile    |  2 +-
>   tools/testing/selftests/rseq/pkey_test.c | 99 ++++++++++++++++++++++++
>   tools/testing/selftests/rseq/rseq.h      |  1 +
>   3 files changed, 101 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..8752ecea21ba8
> --- /dev/null
> +++ b/tools/testing/selftests/rseq/pkey_test.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: LGPL-2.1
> +/*
> + * Ensure that rseq works when rseq data is inaccessible due to 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 <ucontext.h>
> +#include <unistd.h>
> +
> +#include "rseq.h"
> +#include "rseq-abi.h"
> +
> +int pkey;
> +ucontext_t ucp0, ucp1;

Why use an external linkage entity rather than static ?

> +
> +void coroutine(void)
> +{
> +	int i, orig_pk0, old_pk0, old_pk1, pk0, pk1;
> +	/*
> +	 * When we disable access to pkey 0, globals and TLS become
> +	 * inaccessible too, so we need to tread carefully.
> +	 * Pkey is global so we need to copy it to onto stack.

to onto -> onto the ?

> +	 * If ts is not volatile, then compiler may try to init it
> +	 * by loading a global 16-byte value.
> +	 */
> +	volatile int pk = pkey;
> +	volatile struct timespec ts;

I think you are looking for RSEQ_READ_ONCE() when loading from the
global variables to prevent re-fetch. AFAIU the volatile on the stack
variables are not what you are looking for.

> +
> +	orig_pk0 = pkey_get(0);
> +	if (pkey_set(0, PKEY_DISABLE_ACCESS))

AFAIU the pkey_set() call needs to act as a memory clobber. Therefore
having RSEQ_READ_ONCE() before the clobber to copy the global variables
onto the stack should be OK.

Thanks,

Mathieu


> +		err(1, "pkey_set failed");
> +	old_pk0 = pkey_get(0);
> +	old_pk1 = pkey_get(pk);
> +
> +	/*
> +	 * If the kernel misbehaves, context switches in the following loop
> +	 * will terminate the process with SIGSEGV.
> +	 */
> +	ts.tv_sec = 0;
> +	ts.tv_nsec = 10 * 1000;
> +	/*
> +	 * Trigger preemption w/o accessing TLS.
> +	 * Note that glibc's usleep touches errno always.
> +	 */
> +	for (i = 0; i < 10; i++)
> +		syscall(SYS_clock_nanosleep, CLOCK_MONOTONIC, 0, &ts, NULL);
> +
> +	pk0 = pkey_get(0);
> +	pk1 = pkey_get(pk);
> +	if (pkey_set(0, orig_pk0))
> +		err(1, "pkey_set failed");
> +
> +	/*
> +	 * Ensure that the kernel has restored the previous value of pkeys
> +	 * register after changing them.
> +	 */
> +	if (old_pk0 != pk0)
> +		errx(1, "pkey 0 changed %d->%d", old_pk0, pk0);
> +	if (old_pk1 != pk1)
> +		errx(1, "pkey 1 changed %d->%d", old_pk1, pk1);
> +
> +	swapcontext(&ucp1, &ucp0);
> +	abort();
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	pkey = pkey_alloc(0, 0);
> +	if (pkey == -1) {
> +		printf("[SKIP]\tKernel does not support PKEYs: %s\n",
> +			strerror(errno));
> +		return 0;
> +	}
> +
> +	if (rseq_register_current_thread())
> +		err(1, "rseq_register_current_thread failed");
> +
> +	if (getcontext(&ucp1))
> +		err(1, "getcontext failed");
> +	ucp1.uc_stack.ss_size = getpagesize() * 4;
> +	ucp1.uc_stack.ss_sp = mmap(NULL, ucp1.uc_stack.ss_size,
> +		PROT_READ | PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0);
> +	if (ucp1.uc_stack.ss_sp == MAP_FAILED)
> +		err(1, "mmap failed");
> +	if (pkey_mprotect(ucp1.uc_stack.ss_sp, ucp1.uc_stack.ss_size,
> +			PROT_READ | PROT_WRITE, pkey))
> +		err(1, "pkey_mprotect failed");
> +	makecontext(&ucp1, coroutine, 0);
> +	if (swapcontext(&ucp0, &ucp1))
> +		err(1, "swapcontext failed");
> +	return 0;
> +}
> diff --git a/tools/testing/selftests/rseq/rseq.h b/tools/testing/selftests/rseq/rseq.h
> index ba424ce80a719..65da4a727c550 100644
> --- a/tools/testing/selftests/rseq/rseq.h
> +++ b/tools/testing/selftests/rseq/rseq.h
> @@ -8,6 +8,7 @@
>   #ifndef RSEQ_H
>   #define RSEQ_H
>   
> +#include <assert.h>
>   #include <stdint.h>
>   #include <stdbool.h>
>   #include <pthread.h>


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

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

[Dmitry Vyukov]

On Mon, 24 Feb 2025 at 20:04, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> On 2025-02-24 08:20, Dmitry Vyukov wrote:
> > The API allows to switch to permissive pkey register that allows accesses
> > to all PKEYs, and to a value that allows acccess to the 0 (default) PKEY.
> > This functionality is already used in x86 signal delivery, and will be
> > needed for rseq.
>
> AFAIU the signal delivery uses the "allow access to all PKEYs"
> approach only, not the "allow access to 0-pkey". It would be
> good to clarify this in the commit message.

Done in v5.

> > 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
> > Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Fixes: d7822b1e24f2 ("rseq: Introduce restartable sequences system call")
> >
> > ---
> > Changes in v4:
> >   - Added Fixes tag
> >
> > Changes in v3:
> >   - Renamed API functions to write_permissive_pkey_val/write_pkey_val
> >   - Added enable_zero_pkey_val for rseq
> >   - Added Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> >
> > Changes in v2:
> >   - Fixed typo in commit description
> > ---
> >   arch/x86/Kconfig             |  1 +
> >   arch/x86/include/asm/pkeys.h | 33 +++++++++++++++++++++++++++++++++
> >   arch/x86/include/asm/pkru.h  | 10 +++++++---
> >   include/linux/pkeys.h        | 31 +++++++++++++++++++++++++++++++
> >   mm/Kconfig                   |  2 ++
> >   5 files changed, 74 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..d6e35ab5c3d22 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,35 @@ 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 write_permissive_pkey_val(void)
> > +{
> > +     return write_pkru(0);
> > +}
> > +
> > +static inline pkey_reg_t enable_zero_pkey_val(void)
> > +{
> > +     u32 pkru;
> > +
> > +     if (!cpu_feature_enabled(X86_FEATURE_OSPKE))
> > +             return 0;
> > +     /*
> > +      * WRPKRU is relatively expensive compared to RDPKRU,
> > +      * avoid it if possible.
> > +      */
> > +     pkru = rdpkru();
> > +     if ((pkru & (PKRU_AD_BIT|PKRU_WD_BIT)) != 0)
> > +             wrpkru(pkru & ~(PKRU_AD_BIT|PKRU_WD_BIT));
> > +     return pkru;
> > +
> > +
> > +     return write_pkru(0);
>
> This is dead code. What I am missing ?

Done in v5.

> With those fixed, please keep my reviewed-by :)
>
> Thanks,
>
> Mathieu
>
> > +}
> > +
> > +static inline void write_pkey_val(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..262d60f6a15f8 100644
> > --- a/include/linux/pkeys.h
> > +++ b/include/linux/pkeys.h
> > @@ -48,4 +48,35 @@ 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 char 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_val(old_value).
> > + */
> > +static inline pkey_reg_t write_permissive_pkey_val(void)
> > +{
> > +     return 0;
> > +}
> > +
> > +/*
> > + * Sets PKEY access register to a value that allows access to the 0 (default)
> > + * PKEY. Returns the current value of PKEY register.
> > + */
> > +static inline pkey_reg_t enable_zero_pkey_val(void)
> > +{
> > +     return 0;
> > +}
> > +
> > +static inline void write_pkey_val(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 v4 4/4] selftests/rseq: Add test for rseq+pkeys

[Dmitry Vyukov]

On Mon, 24 Feb 2025 at 20:48, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> On 2025-02-24 08:20, Dmitry Vyukov wrote:
> > Add a test that ensures that PKEY-protected struct rseq_cs
> > 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
> > Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
> > Fixes: d7822b1e24f2 ("rseq: Introduce restartable sequences system call")
> >
> > ---
> > Changes in v4:
> >   - Added Fixes tag
> >
> > Changes in v3:
> >   - added Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
> >   - rework the test to work when only pkey 0 is supported for rseq
> >
> > Changes in v2:
> >   - change test to install protected rseq_cs instead of rseq
> > ---
> >   tools/testing/selftests/rseq/Makefile    |  2 +-
> >   tools/testing/selftests/rseq/pkey_test.c | 99 ++++++++++++++++++++++++
> >   tools/testing/selftests/rseq/rseq.h      |  1 +
> >   3 files changed, 101 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..8752ecea21ba8
> > --- /dev/null
> > +++ b/tools/testing/selftests/rseq/pkey_test.c
> > @@ -0,0 +1,99 @@
> > +// SPDX-License-Identifier: LGPL-2.1
> > +/*
> > + * Ensure that rseq works when rseq data is inaccessible due to 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 <ucontext.h>
> > +#include <unistd.h>
> > +
> > +#include "rseq.h"
> > +#include "rseq-abi.h"
> > +
> > +int pkey;
> > +ucontext_t ucp0, ucp1;
>
> Why use an external linkage entity rather than static ?

Done in v5.

> > +
> > +void coroutine(void)
> > +{
> > +     int i, orig_pk0, old_pk0, old_pk1, pk0, pk1;
> > +     /*
> > +      * When we disable access to pkey 0, globals and TLS become
> > +      * inaccessible too, so we need to tread carefully.
> > +      * Pkey is global so we need to copy it to onto stack.
>
> to onto -> onto the ?

Done in v5.

> > +      * If ts is not volatile, then compiler may try to init it
> > +      * by loading a global 16-byte value.
> > +      */
> > +     volatile int pk = pkey;
> > +     volatile struct timespec ts;
>
> I think you are looking for RSEQ_READ_ONCE() when loading from the
> global variables to prevent re-fetch. AFAIU the volatile on the stack
> variables are not what you are looking for.

Done in v5.

> > +
> > +     orig_pk0 = pkey_get(0);
> > +     if (pkey_set(0, PKEY_DISABLE_ACCESS))
>
> AFAIU the pkey_set() call needs to act as a memory clobber. Therefore
> having RSEQ_READ_ONCE() before the clobber to copy the global variables
> onto the stack should be OK.

Done in v5.

> Thanks,
>
> Mathieu
>
>
> > +             err(1, "pkey_set failed");
> > +     old_pk0 = pkey_get(0);
> > +     old_pk1 = pkey_get(pk);
> > +
> > +     /*
> > +      * If the kernel misbehaves, context switches in the following loop
> > +      * will terminate the process with SIGSEGV.
> > +      */
> > +     ts.tv_sec = 0;
> > +     ts.tv_nsec = 10 * 1000;
> > +     /*
> > +      * Trigger preemption w/o accessing TLS.
> > +      * Note that glibc's usleep touches errno always.
> > +      */
> > +     for (i = 0; i < 10; i++)
> > +             syscall(SYS_clock_nanosleep, CLOCK_MONOTONIC, 0, &ts, NULL);
> > +
> > +     pk0 = pkey_get(0);
> > +     pk1 = pkey_get(pk);
> > +     if (pkey_set(0, orig_pk0))
> > +             err(1, "pkey_set failed");
> > +
> > +     /*
> > +      * Ensure that the kernel has restored the previous value of pkeys
> > +      * register after changing them.
> > +      */
> > +     if (old_pk0 != pk0)
> > +             errx(1, "pkey 0 changed %d->%d", old_pk0, pk0);
> > +     if (old_pk1 != pk1)
> > +             errx(1, "pkey 1 changed %d->%d", old_pk1, pk1);
> > +
> > +     swapcontext(&ucp1, &ucp0);
> > +     abort();
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > +     pkey = pkey_alloc(0, 0);
> > +     if (pkey == -1) {
> > +             printf("[SKIP]\tKernel does not support PKEYs: %s\n",
> > +                     strerror(errno));
> > +             return 0;
> > +     }
> > +
> > +     if (rseq_register_current_thread())
> > +             err(1, "rseq_register_current_thread failed");
> > +
> > +     if (getcontext(&ucp1))
> > +             err(1, "getcontext failed");
> > +     ucp1.uc_stack.ss_size = getpagesize() * 4;
> > +     ucp1.uc_stack.ss_sp = mmap(NULL, ucp1.uc_stack.ss_size,
> > +             PROT_READ | PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0);
> > +     if (ucp1.uc_stack.ss_sp == MAP_FAILED)
> > +             err(1, "mmap failed");
> > +     if (pkey_mprotect(ucp1.uc_stack.ss_sp, ucp1.uc_stack.ss_size,
> > +                     PROT_READ | PROT_WRITE, pkey))
> > +             err(1, "pkey_mprotect failed");
> > +     makecontext(&ucp1, coroutine, 0);
> > +     if (swapcontext(&ucp0, &ucp1))
> > +             err(1, "swapcontext failed");
> > +     return 0;
> > +}
> > diff --git a/tools/testing/selftests/rseq/rseq.h b/tools/testing/selftests/rseq/rseq.h
> > index ba424ce80a719..65da4a727c550 100644
> > --- a/tools/testing/selftests/rseq/rseq.h
> > +++ b/tools/testing/selftests/rseq/rseq.h
> > @@ -8,6 +8,7 @@
> >   #ifndef RSEQ_H
> >   #define RSEQ_H
> >
> > +#include <assert.h>
> >   #include <stdint.h>
> >   #include <stdbool.h>
> >   #include <pthread.h>
>
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> https://www.efficios.com

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

[Dmitry Vyukov]

On Mon, 24 Feb 2025 at 20:18, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> On 2025-02-24 08:20, 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
> > pkey value that allows access to the 0 (default) PKEY.
>
> This is a good start, but the plan Dave and I discussed went further
> than this. Those additions are needed:
>
> 1) Add validation at rseq registration that the struct rseq is indeed
>     pkey-0 memory (return failure if not).

I don't think this is worth it for multiple reasons:
 - a program may first register it and then assign a key, which means
we also need to check in pkey_mprotect
 - pkey_mprotect may be applied to rseq of another thread, so ensuring
that will require complex code with non-trivial synchronization and
will add considerable overhead to pkey_mprotect call
 - a program may assign non-0 pkey but have it always accessible, such
programs will break by the new check
 - the misuse is already detected by rseq code, and UNIX errno-based
reporting is not very informative and does not add much value on top
of existing reporting
 - this is not different from registering rseq and then unmap'ing the
memory, checking that does not look like a good idea, and checking
only subset of misuses is inconsistent

Based on my experience with rseq, what would be useful is reporting a
meaningful siginfo for access errors (address/unique code) and fixing
signal delivery. That would solve all of the above problems, and
provide useful info for the user (not just confusing EINVAL from
mprotect/munmap).

But I would prefer to not mix these unrelated usability improvements
and bug fixes with this change. That's not related to this change.


> 2) The pkey-0 requirement is only for struct rseq, which we can check
>     for at rseq registration, and happens to be the fast path. For struct
>     rseq_cs, this is not the same tradeoff: we cannot easily check its
>     associated pkey because the rseq_cs pointer is updated by userspace
>     when entering a critical section. But the good news is that reading
>     the content of struct rseq_cs is *not* a fast-path: it's only done
>     when preempting/delivering a signal over a thread which has a
>     non-NULL rseq_cs pointer.

rseq_cs is usually accessed on a hot path since rseq_cs pointer is not
cleared on critical section exit (at least that's what we do).

>     Therefore reading the struct rseq_cs content should be done with
>     write_permissive_pkey_val(), giving access to all pkeys.

You just asked me to redo the code to simplify it, won't this
complicate it back again? ;)


> 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
> > Fixes: d7822b1e24f2 ("rseq: Introduce restartable sequences system call")
> >
> > ---
> > Changes in v4:
> >   - Added Fixes tag
> >
> > Changes in v3:
> >   - simplify control flow to always enable access to 0 pkey
> >
> > Changes in v2:
> >   - fixed typos and reworded the comment
> > ---
> >   kernel/rseq.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> >
> > diff --git a/kernel/rseq.c b/kernel/rseq.c
> > index 2cb16091ec0ae..9d9c976d3b78c 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>
> > @@ -402,11 +403,19 @@ static int rseq_ip_fixup(struct pt_regs *regs)
> >   void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)
> >   {
> >       struct task_struct *t = current;
> > +     pkey_reg_t saved_pkey;
> >       int ret, sig;
> >
> >       if (unlikely(t->flags & PF_EXITING))
> >               return;
> >
> > +     /*
> > +      * Enable access to the default (0) pkey in case the thread has
> > +      * currently disabled access to it and struct rseq/rseq_cs has
> > +      * 0 pkey assigned (the only supported value for now).
> > +      */
> > +     saved_pkey = enable_zero_pkey_val();
> > +
> >       /*
> >        * 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 +428,11 @@ void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)
> >       }
> >       if (unlikely(rseq_update_cpu_node_id(t)))
> >               goto error;
> > +     write_pkey_val(saved_pkey);
> >       return;
> >
> >   error:
> > +     write_pkey_val(saved_pkey);
> >       sig = ksig ? ksig->sig : 0;
> >       force_sigsegv(sig);
> >   }
>
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> https://www.efficios.com

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

[Mathieu Desnoyers]

On 2025-02-25 09:07, Dmitry Vyukov wrote:
> On Mon, 24 Feb 2025 at 20:18, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> On 2025-02-24 08:20, 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
>>> pkey value that allows access to the 0 (default) PKEY.
>>
>> This is a good start, but the plan Dave and I discussed went further
>> than this. Those additions are needed:
>>
>> 1) Add validation at rseq registration that the struct rseq is indeed
>>      pkey-0 memory (return failure if not).
> 
> I don't think this is worth it for multiple reasons:
>   - a program may first register it and then assign a key, which means
> we also need to check in pkey_mprotect
>   - pkey_mprotect may be applied to rseq of another thread, so ensuring
> that will require complex code with non-trivial synchronization and
> will add considerable overhead to pkey_mprotect call
>   - a program may assign non-0 pkey but have it always accessible, such
> programs will break by the new check
>   - the misuse is already detected by rseq code, and UNIX errno-based
> reporting is not very informative and does not add much value on top
> of existing reporting
>   - this is not different from registering rseq and then unmap'ing the
> memory, checking that does not look like a good idea, and checking
> only subset of misuses is inconsistent
> 
> Based on my experience with rseq, what would be useful is reporting a
> meaningful siginfo for access errors (address/unique code) and fixing
> signal delivery. That would solve all of the above problems, and
> provide useful info for the user (not just confusing EINVAL from
> mprotect/munmap).
> 
> But I would prefer to not mix these unrelated usability improvements
> and bug fixes with this change. That's not related to this change.

I agree with your arguments. If Dave is OK with it, I'd be fine with
leaving out the pkey-0 validation on rseq registration, and eventually
bring meaningful siginfo access errors as future improvements.

So the new behavior would be that both rseq and rseq_cs are required
to be pkey-0. If they are not and their pkey is not accessible in the
current context, it would trigger a segmentation fault. Ideally we'd
want to document this somewhere in the UAPI header.

> 
> 
>> 2) The pkey-0 requirement is only for struct rseq, which we can check
>>      for at rseq registration, and happens to be the fast path. For struct
>>      rseq_cs, this is not the same tradeoff: we cannot easily check its
>>      associated pkey because the rseq_cs pointer is updated by userspace
>>      when entering a critical section. But the good news is that reading
>>      the content of struct rseq_cs is *not* a fast-path: it's only done
>>      when preempting/delivering a signal over a thread which has a
>>      non-NULL rseq_cs pointer.
> 
> rseq_cs is usually accessed on a hot path since rseq_cs pointer is not
> cleared on critical section exit (at least that's what we do).

Fair point.

> 
>>      Therefore reading the struct rseq_cs content should be done with
>>      write_permissive_pkey_val(), giving access to all pkeys.
> 
> You just asked me to redo the code to simplify it, won't this
> complicate it back again? ;)

I'm fine with the pkey-0 approach for both rseq and rseq_cs if Dave is
also OK with it.

Thanks,

Mathieu

> 
> 
>> 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
>>> Fixes: d7822b1e24f2 ("rseq: Introduce restartable sequences system call")
>>>
>>> ---
>>> Changes in v4:
>>>    - Added Fixes tag
>>>
>>> Changes in v3:
>>>    - simplify control flow to always enable access to 0 pkey
>>>
>>> Changes in v2:
>>>    - fixed typos and reworded the comment
>>> ---
>>>    kernel/rseq.c | 11 +++++++++++
>>>    1 file changed, 11 insertions(+)
>>>
>>> diff --git a/kernel/rseq.c b/kernel/rseq.c
>>> index 2cb16091ec0ae..9d9c976d3b78c 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>
>>> @@ -402,11 +403,19 @@ static int rseq_ip_fixup(struct pt_regs *regs)
>>>    void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)
>>>    {
>>>        struct task_struct *t = current;
>>> +     pkey_reg_t saved_pkey;
>>>        int ret, sig;
>>>
>>>        if (unlikely(t->flags & PF_EXITING))
>>>                return;
>>>
>>> +     /*
>>> +      * Enable access to the default (0) pkey in case the thread has
>>> +      * currently disabled access to it and struct rseq/rseq_cs has
>>> +      * 0 pkey assigned (the only supported value for now).
>>> +      */
>>> +     saved_pkey = enable_zero_pkey_val();
>>> +
>>>        /*
>>>         * 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 +428,11 @@ void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)
>>>        }
>>>        if (unlikely(rseq_update_cpu_node_id(t)))
>>>                goto error;
>>> +     write_pkey_val(saved_pkey);
>>>        return;
>>>
>>>    error:
>>> +     write_pkey_val(saved_pkey);
>>>        sig = ksig ? ksig->sig : 0;
>>>        force_sigsegv(sig);
>>>    }
>>
>>
>> --
>> Mathieu Desnoyers
>> EfficiOS Inc.
>> https://www.efficios.com


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

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

[Dmitry Vyukov]

On Tue, 25 Feb 2025 at 15:28, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> On 2025-02-25 09:07, Dmitry Vyukov wrote:
> > On Mon, 24 Feb 2025 at 20:18, Mathieu Desnoyers
> > <mathieu.desnoyers@efficios.com> wrote:
> >>
> >> On 2025-02-24 08:20, 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
> >>> pkey value that allows access to the 0 (default) PKEY.
> >>
> >> This is a good start, but the plan Dave and I discussed went further
> >> than this. Those additions are needed:
> >>
> >> 1) Add validation at rseq registration that the struct rseq is indeed
> >>      pkey-0 memory (return failure if not).
> >
> > I don't think this is worth it for multiple reasons:
> >   - a program may first register it and then assign a key, which means
> > we also need to check in pkey_mprotect
> >   - pkey_mprotect may be applied to rseq of another thread, so ensuring
> > that will require complex code with non-trivial synchronization and
> > will add considerable overhead to pkey_mprotect call
> >   - a program may assign non-0 pkey but have it always accessible, such
> > programs will break by the new check
> >   - the misuse is already detected by rseq code, and UNIX errno-based
> > reporting is not very informative and does not add much value on top
> > of existing reporting
> >   - this is not different from registering rseq and then unmap'ing the
> > memory, checking that does not look like a good idea, and checking
> > only subset of misuses is inconsistent
> >
> > Based on my experience with rseq, what would be useful is reporting a
> > meaningful siginfo for access errors (address/unique code) and fixing
> > signal delivery. That would solve all of the above problems, and
> > provide useful info for the user (not just confusing EINVAL from
> > mprotect/munmap).
> >
> > But I would prefer to not mix these unrelated usability improvements
> > and bug fixes with this change. That's not related to this change.
>
> I agree with your arguments. If Dave is OK with it, I'd be fine with
> leaving out the pkey-0 validation on rseq registration, and eventually
> bring meaningful siginfo access errors as future improvements.
>
> So the new behavior would be that both rseq and rseq_cs are required
> to be pkey-0. If they are not and their pkey is not accessible in the
> current context, it would trigger a segmentation fault. Ideally we'd
> want to document this somewhere in the UAPI header.

Makes sense. I will wait for Dave comments/ack before sending v6. But
to save a round-trip, does this look reasonable?

--- a/include/uapi/linux/rseq.h
+++ b/include/uapi/linux/rseq.h
@@ -58,6 +58,10 @@ struct rseq_cs {
  * contained within a single cache-line.
  *
  * A single struct rseq per thread is allowed.
+ *
+ * If struct rseq or struct rseq_cs is used with Memory Protection Keys,
+ * then the assigned pkey should either be accessible whenever these structs
+ * are registered/installed, or they should be protected with pkey 0.
  */
 struct rseq {



> >> 2) The pkey-0 requirement is only for struct rseq, which we can check
> >>      for at rseq registration, and happens to be the fast path. For struct
> >>      rseq_cs, this is not the same tradeoff: we cannot easily check its
> >>      associated pkey because the rseq_cs pointer is updated by userspace
> >>      when entering a critical section. But the good news is that reading
> >>      the content of struct rseq_cs is *not* a fast-path: it's only done
> >>      when preempting/delivering a signal over a thread which has a
> >>      non-NULL rseq_cs pointer.
> >
> > rseq_cs is usually accessed on a hot path since rseq_cs pointer is not
> > cleared on critical section exit (at least that's what we do).
>
> Fair point.
>
> >
> >>      Therefore reading the struct rseq_cs content should be done with
> >>      write_permissive_pkey_val(), giving access to all pkeys.
> >
> > You just asked me to redo the code to simplify it, won't this
> > complicate it back again? ;)
>
> I'm fine with the pkey-0 approach for both rseq and rseq_cs if Dave is
> also OK with it.

It should work for my current use case, at least how I currently see
it. Ways people use pkeys are pretty unique, so it's hard to
extrapolate. But there is one more possibility: when a program
switches PKEYs, it may also clear stale rseq_cs pointer from rseq.
This way rseq_cs may have non-0 keys assigned, but they are always
accessible while installed.



> Thanks,
>
> Mathieu
>
> >
> >
> >> 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
> >>> Fixes: d7822b1e24f2 ("rseq: Introduce restartable sequences system call")
> >>>
> >>> ---
> >>> Changes in v4:
> >>>    - Added Fixes tag
> >>>
> >>> Changes in v3:
> >>>    - simplify control flow to always enable access to 0 pkey
> >>>
> >>> Changes in v2:
> >>>    - fixed typos and reworded the comment
> >>> ---
> >>>    kernel/rseq.c | 11 +++++++++++
> >>>    1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/kernel/rseq.c b/kernel/rseq.c
> >>> index 2cb16091ec0ae..9d9c976d3b78c 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>
> >>> @@ -402,11 +403,19 @@ static int rseq_ip_fixup(struct pt_regs *regs)
> >>>    void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)
> >>>    {
> >>>        struct task_struct *t = current;
> >>> +     pkey_reg_t saved_pkey;
> >>>        int ret, sig;
> >>>
> >>>        if (unlikely(t->flags & PF_EXITING))
> >>>                return;
> >>>
> >>> +     /*
> >>> +      * Enable access to the default (0) pkey in case the thread has
> >>> +      * currently disabled access to it and struct rseq/rseq_cs has
> >>> +      * 0 pkey assigned (the only supported value for now).
> >>> +      */
> >>> +     saved_pkey = enable_zero_pkey_val();
> >>> +
> >>>        /*
> >>>         * 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 +428,11 @@ void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)
> >>>        }
> >>>        if (unlikely(rseq_update_cpu_node_id(t)))
> >>>                goto error;
> >>> +     write_pkey_val(saved_pkey);
> >>>        return;
> >>>
> >>>    error:
> >>> +     write_pkey_val(saved_pkey);
> >>>        sig = ksig ? ksig->sig : 0;
> >>>        force_sigsegv(sig);
> >>>    }

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

[Mathieu Desnoyers]

On 2025-02-25 09:51, Dmitry Vyukov wrote:
> On Tue, 25 Feb 2025 at 15:28, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> On 2025-02-25 09:07, Dmitry Vyukov wrote:
>>> On Mon, 24 Feb 2025 at 20:18, Mathieu Desnoyers
>>> <mathieu.desnoyers@efficios.com> wrote:
>>>>
>>>> On 2025-02-24 08:20, 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
>>>>> pkey value that allows access to the 0 (default) PKEY.
>>>>
>>>> This is a good start, but the plan Dave and I discussed went further
>>>> than this. Those additions are needed:
>>>>
>>>> 1) Add validation at rseq registration that the struct rseq is indeed
>>>>       pkey-0 memory (return failure if not).
>>>
>>> I don't think this is worth it for multiple reasons:
>>>    - a program may first register it and then assign a key, which means
>>> we also need to check in pkey_mprotect
>>>    - pkey_mprotect may be applied to rseq of another thread, so ensuring
>>> that will require complex code with non-trivial synchronization and
>>> will add considerable overhead to pkey_mprotect call
>>>    - a program may assign non-0 pkey but have it always accessible, such
>>> programs will break by the new check
>>>    - the misuse is already detected by rseq code, and UNIX errno-based
>>> reporting is not very informative and does not add much value on top
>>> of existing reporting
>>>    - this is not different from registering rseq and then unmap'ing the
>>> memory, checking that does not look like a good idea, and checking
>>> only subset of misuses is inconsistent
>>>
>>> Based on my experience with rseq, what would be useful is reporting a
>>> meaningful siginfo for access errors (address/unique code) and fixing
>>> signal delivery. That would solve all of the above problems, and
>>> provide useful info for the user (not just confusing EINVAL from
>>> mprotect/munmap).
>>>
>>> But I would prefer to not mix these unrelated usability improvements
>>> and bug fixes with this change. That's not related to this change.
>>
>> I agree with your arguments. If Dave is OK with it, I'd be fine with
>> leaving out the pkey-0 validation on rseq registration, and eventually
>> bring meaningful siginfo access errors as future improvements.
>>
>> So the new behavior would be that both rseq and rseq_cs are required
>> to be pkey-0. If they are not and their pkey is not accessible in the
>> current context, it would trigger a segmentation fault. Ideally we'd
>> want to document this somewhere in the UAPI header.
> 
> Makes sense. I will wait for Dave comments/ack before sending v6. But
> to save a round-trip, does this look reasonable?
> 
> --- a/include/uapi/linux/rseq.h
> +++ b/include/uapi/linux/rseq.h
> @@ -58,6 +58,10 @@ struct rseq_cs {
>    * contained within a single cache-line.
>    *
>    * A single struct rseq per thread is allowed.
> + *
> + * If struct rseq or struct rseq_cs is used with Memory Protection Keys,
> + * then the assigned pkey should either be accessible whenever these structs
> + * are registered/installed, or they should be protected with pkey 0.

The wording is OK with me.

Thanks,

Mathieu

>    */
>   struct rseq {
> 
> 
> 
>>>> 2) The pkey-0 requirement is only for struct rseq, which we can check
>>>>       for at rseq registration, and happens to be the fast path. For struct
>>>>       rseq_cs, this is not the same tradeoff: we cannot easily check its
>>>>       associated pkey because the rseq_cs pointer is updated by userspace
>>>>       when entering a critical section. But the good news is that reading
>>>>       the content of struct rseq_cs is *not* a fast-path: it's only done
>>>>       when preempting/delivering a signal over a thread which has a
>>>>       non-NULL rseq_cs pointer.
>>>
>>> rseq_cs is usually accessed on a hot path since rseq_cs pointer is not
>>> cleared on critical section exit (at least that's what we do).
>>
>> Fair point.
>>
>>>
>>>>       Therefore reading the struct rseq_cs content should be done with
>>>>       write_permissive_pkey_val(), giving access to all pkeys.
>>>
>>> You just asked me to redo the code to simplify it, won't this
>>> complicate it back again? ;)
>>
>> I'm fine with the pkey-0 approach for both rseq and rseq_cs if Dave is
>> also OK with it.
> 
> It should work for my current use case, at least how I currently see
> it. Ways people use pkeys are pretty unique, so it's hard to
> extrapolate. But there is one more possibility: when a program
> switches PKEYs, it may also clear stale rseq_cs pointer from rseq.
> This way rseq_cs may have non-0 keys assigned, but they are always
> accessible while installed.
> 
> 
> 
>> Thanks,
>>
>> Mathieu
>>
>>>
>>>
>>>> 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
>>>>> Fixes: d7822b1e24f2 ("rseq: Introduce restartable sequences system call")
>>>>>
>>>>> ---
>>>>> Changes in v4:
>>>>>     - Added Fixes tag
>>>>>
>>>>> Changes in v3:
>>>>>     - simplify control flow to always enable access to 0 pkey
>>>>>
>>>>> Changes in v2:
>>>>>     - fixed typos and reworded the comment
>>>>> ---
>>>>>     kernel/rseq.c | 11 +++++++++++
>>>>>     1 file changed, 11 insertions(+)
>>>>>
>>>>> diff --git a/kernel/rseq.c b/kernel/rseq.c
>>>>> index 2cb16091ec0ae..9d9c976d3b78c 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>
>>>>> @@ -402,11 +403,19 @@ static int rseq_ip_fixup(struct pt_regs *regs)
>>>>>     void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)
>>>>>     {
>>>>>         struct task_struct *t = current;
>>>>> +     pkey_reg_t saved_pkey;
>>>>>         int ret, sig;
>>>>>
>>>>>         if (unlikely(t->flags & PF_EXITING))
>>>>>                 return;
>>>>>
>>>>> +     /*
>>>>> +      * Enable access to the default (0) pkey in case the thread has
>>>>> +      * currently disabled access to it and struct rseq/rseq_cs has
>>>>> +      * 0 pkey assigned (the only supported value for now).
>>>>> +      */
>>>>> +     saved_pkey = enable_zero_pkey_val();
>>>>> +
>>>>>         /*
>>>>>          * 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 +428,11 @@ void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)
>>>>>         }
>>>>>         if (unlikely(rseq_update_cpu_node_id(t)))
>>>>>                 goto error;
>>>>> +     write_pkey_val(saved_pkey);
>>>>>         return;
>>>>>
>>>>>     error:
>>>>> +     write_pkey_val(saved_pkey);
>>>>>         sig = ksig ? ksig->sig : 0;
>>>>>         force_sigsegv(sig);
>>>>>     }


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

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

[Dmitry Vyukov]

On Tue, 25 Feb 2025 at 15:53, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> On 2025-02-25 09:51, Dmitry Vyukov wrote:
> > On Tue, 25 Feb 2025 at 15:28, Mathieu Desnoyers
> > <mathieu.desnoyers@efficios.com> wrote:
> >>
> >> On 2025-02-25 09:07, Dmitry Vyukov wrote:
> >>> On Mon, 24 Feb 2025 at 20:18, Mathieu Desnoyers
> >>> <mathieu.desnoyers@efficios.com> wrote:
> >>>>
> >>>> On 2025-02-24 08:20, 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
> >>>>> pkey value that allows access to the 0 (default) PKEY.
> >>>>
> >>>> This is a good start, but the plan Dave and I discussed went further
> >>>> than this. Those additions are needed:
> >>>>
> >>>> 1) Add validation at rseq registration that the struct rseq is indeed
> >>>>       pkey-0 memory (return failure if not).
> >>>
> >>> I don't think this is worth it for multiple reasons:
> >>>    - a program may first register it and then assign a key, which means
> >>> we also need to check in pkey_mprotect
> >>>    - pkey_mprotect may be applied to rseq of another thread, so ensuring
> >>> that will require complex code with non-trivial synchronization and
> >>> will add considerable overhead to pkey_mprotect call
> >>>    - a program may assign non-0 pkey but have it always accessible, such
> >>> programs will break by the new check
> >>>    - the misuse is already detected by rseq code, and UNIX errno-based
> >>> reporting is not very informative and does not add much value on top
> >>> of existing reporting
> >>>    - this is not different from registering rseq and then unmap'ing the
> >>> memory, checking that does not look like a good idea, and checking
> >>> only subset of misuses is inconsistent
> >>>
> >>> Based on my experience with rseq, what would be useful is reporting a
> >>> meaningful siginfo for access errors (address/unique code) and fixing
> >>> signal delivery. That would solve all of the above problems, and
> >>> provide useful info for the user (not just confusing EINVAL from
> >>> mprotect/munmap).
> >>>
> >>> But I would prefer to not mix these unrelated usability improvements
> >>> and bug fixes with this change. That's not related to this change.
> >>
> >> I agree with your arguments. If Dave is OK with it, I'd be fine with
> >> leaving out the pkey-0 validation on rseq registration, and eventually
> >> bring meaningful siginfo access errors as future improvements.
> >>
> >> So the new behavior would be that both rseq and rseq_cs are required
> >> to be pkey-0. If they are not and their pkey is not accessible in the
> >> current context, it would trigger a segmentation fault. Ideally we'd
> >> want to document this somewhere in the UAPI header.
> >
> > Makes sense. I will wait for Dave comments/ack before sending v6. But
> > to save a round-trip, does this look reasonable?
> >
> > --- a/include/uapi/linux/rseq.h
> > +++ b/include/uapi/linux/rseq.h
> > @@ -58,6 +58,10 @@ struct rseq_cs {
> >    * contained within a single cache-line.
> >    *
> >    * A single struct rseq per thread is allowed.
> > + *
> > + * If struct rseq or struct rseq_cs is used with Memory Protection Keys,
> > + * then the assigned pkey should either be accessible whenever these structs
> > + * are registered/installed, or they should be protected with pkey 0.
>
> The wording is OK with me.
>
> Thanks,
>
> Mathieu

Sent v6 with this documentation change.

> >    */
> >   struct rseq {
> >
> >
> >
> >>>> 2) The pkey-0 requirement is only for struct rseq, which we can check
> >>>>       for at rseq registration, and happens to be the fast path. For struct
> >>>>       rseq_cs, this is not the same tradeoff: we cannot easily check its
> >>>>       associated pkey because the rseq_cs pointer is updated by userspace
> >>>>       when entering a critical section. But the good news is that reading
> >>>>       the content of struct rseq_cs is *not* a fast-path: it's only done
> >>>>       when preempting/delivering a signal over a thread which has a
> >>>>       non-NULL rseq_cs pointer.
> >>>
> >>> rseq_cs is usually accessed on a hot path since rseq_cs pointer is not
> >>> cleared on critical section exit (at least that's what we do).
> >>
> >> Fair point.
> >>
> >>>
> >>>>       Therefore reading the struct rseq_cs content should be done with
> >>>>       write_permissive_pkey_val(), giving access to all pkeys.
> >>>
> >>> You just asked me to redo the code to simplify it, won't this
> >>> complicate it back again? ;)
> >>
> >> I'm fine with the pkey-0 approach for both rseq and rseq_cs if Dave is
> >> also OK with it.
> >
> > It should work for my current use case, at least how I currently see
> > it. Ways people use pkeys are pretty unique, so it's hard to
> > extrapolate. But there is one more possibility: when a program
> > switches PKEYs, it may also clear stale rseq_cs pointer from rseq.
> > This way rseq_cs may have non-0 keys assigned, but they are always
> > accessible while installed.
> >
> >
> >
> >> Thanks,
> >>
> >> Mathieu
> >>
> >>>
> >>>
> >>>> 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
> >>>>> Fixes: d7822b1e24f2 ("rseq: Introduce restartable sequences system call")
> >>>>>
> >>>>> ---
> >>>>> Changes in v4:
> >>>>>     - Added Fixes tag
> >>>>>
> >>>>> Changes in v3:
> >>>>>     - simplify control flow to always enable access to 0 pkey
> >>>>>
> >>>>> Changes in v2:
> >>>>>     - fixed typos and reworded the comment
> >>>>> ---
> >>>>>     kernel/rseq.c | 11 +++++++++++
> >>>>>     1 file changed, 11 insertions(+)
> >>>>>
> >>>>> diff --git a/kernel/rseq.c b/kernel/rseq.c
> >>>>> index 2cb16091ec0ae..9d9c976d3b78c 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>
> >>>>> @@ -402,11 +403,19 @@ static int rseq_ip_fixup(struct pt_regs *regs)
> >>>>>     void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)
> >>>>>     {
> >>>>>         struct task_struct *t = current;
> >>>>> +     pkey_reg_t saved_pkey;
> >>>>>         int ret, sig;
> >>>>>
> >>>>>         if (unlikely(t->flags & PF_EXITING))
> >>>>>                 return;
> >>>>>
> >>>>> +     /*
> >>>>> +      * Enable access to the default (0) pkey in case the thread has
> >>>>> +      * currently disabled access to it and struct rseq/rseq_cs has
> >>>>> +      * 0 pkey assigned (the only supported value for now).
> >>>>> +      */
> >>>>> +     saved_pkey = enable_zero_pkey_val();
> >>>>> +
> >>>>>         /*
> >>>>>          * 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 +428,11 @@ void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)
> >>>>>         }
> >>>>>         if (unlikely(rseq_update_cpu_node_id(t)))
> >>>>>                 goto error;
> >>>>> +     write_pkey_val(saved_pkey);
> >>>>>         return;
> >>>>>
> >>>>>     error:
> >>>>> +     write_pkey_val(saved_pkey);
> >>>>>         sig = ksig ? ksig->sig : 0;
> >>>>>         force_sigsegv(sig);
> >>>>>     }
>
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> https://www.efficios.com