| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <cc8b653e-29ad-4cfb-91c3-1e001a811c4c@efficios.com>
Date: Mon, 24 Feb 2025 14:48:55 -0500
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: Dmitry Vyukov <dvyukov@...gle.com>, peterz@...radead.org,
boqun.feng@...il.com, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com, hpa@...or.com, aruna.ramakrishna@...cle.com,
elver@...gle.com
Cc: "Paul E. McKenney" <paulmck@...nel.org>, x86@...nel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 4/4] selftests/rseq: Add test for rseq+pkeys
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@...gle.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: "Paul E. McKenney" <paulmck@...nel.org>
> Cc: Boqun Feng <boqun.feng@...il.com>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Borislav Petkov <bp@...en8.de>
> Cc: Dave Hansen <dave.hansen@...ux.intel.com>
> Cc: "H. Peter Anvin" <hpa@...or.com>
> Cc: Aruna Ramakrishna <aruna.ramakrishna@...cle.com>
> Cc: x86@...nel.org
> Cc: linux-kernel@...r.kernel.org
> Acked-by: Dave Hansen <dave.hansen@...ux.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@...ux.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
Powered by blists - more mailing lists