| 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: <CACT4Y+Z6BgdeSJjQ6uMrhJsVt72WzpFMU0TA6d5WzQs42gzPjw@mail.gmail.com>
Date: Tue, 25 Feb 2025 14:55:04 +0100
From: Dmitry Vyukov <dvyukov@...gle.com>
To: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc: 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,
"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 Mon, 24 Feb 2025 at 20:48, Mathieu Desnoyers
<mathieu.desnoyers@...icios.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@...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 ?
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
Powered by blists - more mailing lists