From: Carlos O'Donell <carlos@redhat.com>
To: "H.J. Lu" <hjl.tools@gmail.com>,
Adhemerval Zanella <adhemerval.zanella@linaro.org>
Cc: GNU C Library <libc-alpha@sourceware.org>,
Florian Weimer <fweimer@redhat.com>
Subject: Re: V2 [PATCH] x86: Properly set usable CET feature bits [BZ #26625]
Date: Thu, 28 Jan 2021 21:42:34 -0500 [thread overview]
Message-ID: <c0322c07-7751-678f-5c7b-72cdfb3d9adf@redhat.com> (raw)
In-Reply-To: <CAMe9rOo5xUnqKGWgDoLV5mCyh_U1MEHJn8cjyJqdx+FinJOOvQ@mail.gmail.com>
On 1/28/21 1:57 PM, H.J. Lu wrote:
> Here is the updated patch. OK for master?
Yes.
I don't think this patch will have any impact on the x86_64 and i686 testing
that we've done on RHEL7, RHEL8, and versions under development. It should
improve our ability to test different scenarios on Tigerlake hardware. We
should include this in the release to support the hardware. The change is
fairly minor, it just looks like a lot because we have to make the change
in several places and add a regression test.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> From 65a9ca9c20db7ffd61f3defb4083b513589c7231 Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Tue, 26 Jan 2021 20:48:45 -0800
> Subject: [PATCH] x86: Properly set usable CET feature bits [BZ #26625]
>
> commit 94cd37ebb293321115a36a422b091fdb72d2fb08
> Author: H.J. Lu <hjl.tools@gmail.com>
> Date: Wed Sep 16 05:27:32 2020 -0700
>
> x86: Use HAS_CPU_FEATURE with IBT and SHSTK [BZ #26625]
>
> broke
>
> GLIBC_TUNABLES=glibc.cpu.hwcaps=-IBT,-SHSTK
>
> since it can no longer disable IBT nor SHSTK. Handle IBT and SHSTK with:
>
> 1. Revert commit 94cd37ebb293321115a36a422b091fdb72d2fb08.
> 2. Clears the usable CET feature bits if kernel doesn't support CET.
> 3. Add GLIBC_TUNABLES tests without dlopen.
> 4. Add tests to verify that CPU_FEATURE_USABLE on IBT and SHSTK matches
> _get_ssp.
> 5. Update GLIBC_TUNABLES tests with dlopen to verify that CET is disabled
> with GLIBC_TUNABLES.
> ---
> sysdeps/x86/Makefile | 8 ++++-
> sysdeps/x86/cpu-features.c | 11 +++++--
> sysdeps/x86/dl-cet.c | 4 +--
> sysdeps/x86/tst-cet-legacy-10-static.c | 1 +
> sysdeps/x86/tst-cet-legacy-10.c | 43 ++++++++++++++++++++++++++
> sysdeps/x86/tst-cet-legacy-5.c | 11 ++++---
> sysdeps/x86/tst-cet-legacy-6.c | 11 ++++---
> sysdeps/x86/tst-cet-legacy-9-static.c | 1 +
> sysdeps/x86/tst-cet-legacy-9.c | 42 +++++++++++++++++++++++++
> sysdeps/x86/tst-get-cpu-features.c | 2 ++
> 10 files changed, 121 insertions(+), 13 deletions(-)
> create mode 100644 sysdeps/x86/tst-cet-legacy-10-static.c
> create mode 100644 sysdeps/x86/tst-cet-legacy-10.c
> create mode 100644 sysdeps/x86/tst-cet-legacy-9-static.c
> create mode 100644 sysdeps/x86/tst-cet-legacy-9.c
>
> diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile
> index 7549507a9a..dd82674342 100644
> --- a/sysdeps/x86/Makefile
> +++ b/sysdeps/x86/Makefile
> @@ -82,7 +82,9 @@ sysdep-dl-routines += dl-cet
> tests += tst-cet-legacy-1 tst-cet-legacy-1a tst-cet-legacy-2 \
> tst-cet-legacy-2a tst-cet-legacy-3 tst-cet-legacy-4 \
> tst-cet-legacy-5a tst-cet-legacy-6a tst-cet-legacy-7 \
> - tst-cet-legacy-8
> + tst-cet-legacy-8 tst-cet-legacy-9 tst-cet-legacy-9-static \
> + tst-cet-legacy-10 tst-cet-legacy-10-static
OK. Add 2 new testes with static variants.
> +tests-static += tst-cet-legacy-9-static tst-cet-legacy-10-static
OK. List static variants.
> tst-cet-legacy-1a-ARGS = -- $(host-test-program-cmd)
> ifneq (no,$(have-tunables))
> tests += tst-cet-legacy-4a tst-cet-legacy-4b tst-cet-legacy-4c \
> @@ -123,6 +125,8 @@ CFLAGS-tst-cet-legacy-mod-6b.c += -fcf-protection
> CFLAGS-tst-cet-legacy-mod-6c.c += -fcf-protection
> CFLAGS-tst-cet-legacy-7.c += -fcf-protection=none
> CFLAGS-tst-cet-legacy-8.c += -mshstk
> +CFLAGS-tst-cet-legacy-10.c += -mshstk
> +CFLAGS-tst-cet-legacy-10-static.c += -mshstk
OK. Build the 10th test with shadow stack enabled explicitly (maybe
the current toolchain doesn't do that).
>
> $(objpfx)tst-cet-legacy-1: $(objpfx)tst-cet-legacy-mod-1.so \
> $(objpfx)tst-cet-legacy-mod-2.so
> @@ -163,6 +167,8 @@ $(objpfx)tst-cet-legacy-6b: $(libdl)
> $(objpfx)tst-cet-legacy-6b.out: $(objpfx)tst-cet-legacy-mod-6a.so \
> $(objpfx)tst-cet-legacy-mod-6b.so
> tst-cet-legacy-6b-ENV = GLIBC_TUNABLES=glibc.cpu.hwcaps=-IBT,-SHSTK
> +tst-cet-legacy-9-ENV = GLIBC_TUNABLES=glibc.cpu.hwcaps=-IBT,-SHSTK
> +tst-cet-legacy-9-static-ENV = GLIBC_TUNABLES=glibc.cpu.hwcaps=-IBT,-SHSTK
OK. Run a test with IBT and SHSTK disabled by tunables.
> endif
> endif
>
> diff --git a/sysdeps/x86/cpu-features.c b/sysdeps/x86/cpu-features.c
> index 6496512a0d..73b0a4dc9a 100644
> --- a/sysdeps/x86/cpu-features.c
> +++ b/sysdeps/x86/cpu-features.c
> @@ -75,6 +75,7 @@ update_usable (struct cpu_features *cpu_features)
> CPU_FEATURE_SET_USABLE (cpu_features, PREFETCHWT1);
> CPU_FEATURE_SET_USABLE (cpu_features, OSPKE);
> CPU_FEATURE_SET_USABLE (cpu_features, WAITPKG);
> + CPU_FEATURE_SET_USABLE (cpu_features, SHSTK);
OK. Copy SHSTK bit from cpuid.
> CPU_FEATURE_SET_USABLE (cpu_features, GFNI);
> CPU_FEATURE_SET_USABLE (cpu_features, RDPID);
> CPU_FEATURE_SET_USABLE (cpu_features, RDRAND);
> @@ -84,6 +85,7 @@ update_usable (struct cpu_features *cpu_features)
> CPU_FEATURE_SET_USABLE (cpu_features, FSRM);
> CPU_FEATURE_SET_USABLE (cpu_features, SERIALIZE);
> CPU_FEATURE_SET_USABLE (cpu_features, TSXLDTRK);
> + CPU_FEATURE_SET_USABLE (cpu_features, IBT);
OK. Copy IBT bit from cpuid.
> CPU_FEATURE_SET_USABLE (cpu_features, LAHF64_SAHF64);
> CPU_FEATURE_SET_USABLE (cpu_features, LZCNT);
> CPU_FEATURE_SET_USABLE (cpu_features, SSE4A);
> @@ -705,6 +707,11 @@ no_cpuid:
> /* Check CET status. */
> unsigned int cet_status = get_cet_status ();
>
> + if ((cet_status & GNU_PROPERTY_X86_FEATURE_1_IBT) == 0)
> + CPU_FEATURE_UNSET (cpu_features, IBT)
> + if ((cet_status & GNU_PROPERTY_X86_FEATURE_1_SHSTK) == 0)
> + CPU_FEATURE_UNSET (cpu_features, SHSTK)
OK. Disable the bit in cpu_features if process has CET disabled
(uses prctl ARCH_CET_STATUS).
> +
> if (cet_status)
> {
> GL(dl_x86_feature_1) = cet_status;
> @@ -720,9 +727,9 @@ no_cpuid:
> GLIBC_TUNABLES=glibc.cpu.hwcaps=-IBT,-SHSTK
> */
> unsigned int cet_feature = 0;
> - if (!HAS_CPU_FEATURE (IBT))
> + if (!CPU_FEATURE_USABLE (IBT))
OK. Use CPU_FEATURE_USABLE to check IBT bit because we are
consistently using CPU_FEATURE_* macros. The dyanmic loader
will have recorded tunable preferences in GLRO(dl_x86_cpu_features)
and we check them here.
> cet_feature |= GNU_PROPERTY_X86_FEATURE_1_IBT;
> - if (!HAS_CPU_FEATURE (SHSTK))
> + if (!CPU_FEATURE_USABLE (SHSTK))
OK.
> cet_feature |= GNU_PROPERTY_X86_FEATURE_1_SHSTK;
>
> if (cet_feature)
> diff --git a/sysdeps/x86/dl-cet.c b/sysdeps/x86/dl-cet.c
> index a63b9c7164..c74e577289 100644
> --- a/sysdeps/x86/dl-cet.c
> +++ b/sysdeps/x86/dl-cet.c
> @@ -77,11 +77,11 @@ dl_cet_check (struct link_map *m, const char *program)
>
> GLIBC_TUNABLES=glibc.cpu.hwcaps=-IBT,-SHSTK
> */
> - enable_ibt &= (HAS_CPU_FEATURE (IBT)
> + enable_ibt &= (CPU_FEATURE_USABLE (IBT)
OK.
> && (enable_ibt_type == cet_always_on
> || (m->l_x86_feature_1_and
> & GNU_PROPERTY_X86_FEATURE_1_IBT) != 0));
> - enable_shstk &= (HAS_CPU_FEATURE (SHSTK)
> + enable_shstk &= (CPU_FEATURE_USABLE (SHSTK)
OK.
> && (enable_shstk_type == cet_always_on
> || (m->l_x86_feature_1_and
> & GNU_PROPERTY_X86_FEATURE_1_SHSTK) != 0));
> diff --git a/sysdeps/x86/tst-cet-legacy-10-static.c b/sysdeps/x86/tst-cet-legacy-10-static.c
> new file mode 100644
> index 0000000000..ecc1208e35
> --- /dev/null
> +++ b/sysdeps/x86/tst-cet-legacy-10-static.c
> @@ -0,0 +1 @@
> +#include "tst-cet-legacy-10.c"
OK.
> diff --git a/sysdeps/x86/tst-cet-legacy-10.c b/sysdeps/x86/tst-cet-legacy-10.c
> new file mode 100644
> index 0000000000..a618557f45
> --- /dev/null
> +++ b/sysdeps/x86/tst-cet-legacy-10.c
> @@ -0,0 +1,43 @@
> +/* Check CPU_FEATURE_USABLE on IBT and SHSTK.
> + Copyright (C) 2021 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#include <x86intrin.h>
> +#include <sys/platform/x86.h>
> +#include <support/test-driver.h>
> +#include <support/xunistd.h>
> +
> +/* Check that CPU_FEATURE_USABLE on IBT and SHSTK matches _get_ssp. */
> +
> +static int
> +do_test (void)
> +{
> + if (_get_ssp () != 0)
> + {
> + if (CPU_FEATURE_USABLE (IBT) && CPU_FEATURE_USABLE (SHSTK))
> + return EXIT_SUCCESS;
> + }
> + else
> + {
> + if (!CPU_FEATURE_USABLE (IBT) && !CPU_FEATURE_USABLE (SHSTK))
> + return EXIT_SUCCESS;
> + }
> +
> + return EXIT_FAILURE;
> +}
OK. Test looks good and uses _get_ssp (CET intrinsic) from the compiler
to test what glibc sees matches what the hardware reports.
> +
> +#include <support/test-driver.c>
> diff --git a/sysdeps/x86/tst-cet-legacy-5.c b/sysdeps/x86/tst-cet-legacy-5.c
> index e3efeb1f4e..d870de3eae 100644
> --- a/sysdeps/x86/tst-cet-legacy-5.c
> +++ b/sysdeps/x86/tst-cet-legacy-5.c
> @@ -37,6 +37,12 @@ do_test_1 (const char *modname, bool fail)
> int (*fp) (void);
> void *h;
>
> + /* NB: dlopen should never fail on non-CET platforms. If SHSTK is
> + disabled, assuming IBT is also disabled. */
> + bool cet_enabled = _get_ssp () != 0 && !CET_MAYBE_DISABLED;
> + if (!cet_enabled)
> + fail = false;
OK. Move *AHEAD* of use of fail in h==NULL case.
> +
> h = dlopen (modname, RTLD_LAZY);
> if (h == NULL)
> {
> @@ -53,10 +59,7 @@ do_test_1 (const char *modname, bool fail)
> FAIL_EXIT1 ("cannot open '%s': %s\n", modname, err);
> }
>
> - /* NB: dlopen should never fail on non-CET platforms. If SHSTK is
> - disabled, assuming IBT is also disabled. */
> - bool cet_enabled = _get_ssp () != 0 && !CET_MAYBE_DISABLED;
> - if (fail && cet_enabled)
> + if (fail)
OK.
> FAIL_EXIT1 ("dlopen should have failed\n");
>
> fp = dlsym (h, "test");
> diff --git a/sysdeps/x86/tst-cet-legacy-6.c b/sysdeps/x86/tst-cet-legacy-6.c
> index 44b2ef5c7a..8e82ee2246 100644
> --- a/sysdeps/x86/tst-cet-legacy-6.c
> +++ b/sysdeps/x86/tst-cet-legacy-6.c
> @@ -37,6 +37,12 @@ do_test_1 (const char *modname, bool fail)
> int (*fp) (void);
> void *h;
>
> + /* NB: dlopen should never fail on non-CET platforms. If SHSTK is
> + disabled, assuming IBT is also disabled. */
> + bool cet_enabled = _get_ssp () != 0 && !CET_MAYBE_DISABLED;
> + if (!cet_enabled)
> + fail = false;
OK. Move fail setting ahead of use.
> +
> h = dlopen (modname, RTLD_LAZY);
> if (h == NULL)
> {
> @@ -53,10 +59,7 @@ do_test_1 (const char *modname, bool fail)
> FAIL_EXIT1 ("cannot open '%s': %s\n", modname, err);
> }
>
> - /* NB: dlopen should never fail on non-CET platforms. If SHSTK is
> - disabled, assuming IBT is also disabled. */
> - bool cet_enabled = _get_ssp () != 0 && !CET_MAYBE_DISABLED;
> - if (fail && cet_enabled)
> + if (fail)
OK.
> FAIL_EXIT1 ("dlopen should have failed\n");
>
> fp = dlsym (h, "test");
> diff --git a/sysdeps/x86/tst-cet-legacy-9-static.c b/sysdeps/x86/tst-cet-legacy-9-static.c
> new file mode 100644
> index 0000000000..f9a8518b99
> --- /dev/null
> +++ b/sysdeps/x86/tst-cet-legacy-9-static.c
> @@ -0,0 +1 @@
> +#include "tst-cet-legacy-9.c"
OK.
> diff --git a/sysdeps/x86/tst-cet-legacy-9.c b/sysdeps/x86/tst-cet-legacy-9.c
> new file mode 100644
> index 0000000000..fe4d8e73c8
> --- /dev/null
> +++ b/sysdeps/x86/tst-cet-legacy-9.c
> @@ -0,0 +1,42 @@
> +/* Check CET compatibility with legacy JIT engine via GLIBC_TUNABLES.
> + Copyright (C) 2021 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/mman.h>
> +#include <support/test-driver.h>
> +#include <support/xunistd.h>
> +
> +/* Check that mmapped legacy code won't trigger segfault with
> + -fcf-protection and GLIBC_TUNABLES=glibc.cpu.hwcaps=-IBT,-SHSTK. */
> +
> +static int
> +do_test (void)
> +{
> + void (*funcp) (void);
> + size_t page_size = xsysconf (_SC_PAGESIZE);
> + funcp = xmmap (NULL, page_size, PROT_EXEC | PROT_READ | PROT_WRITE,
> + MAP_ANONYMOUS | MAP_PRIVATE, -1);
> + printf ("mmap = %p\n", funcp);
> + /* Write RET instruction. */
> + *(char *) funcp = 0xc3;
> + funcp ();
> + return EXIT_SUCCESS;
OK.
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/sysdeps/x86/tst-get-cpu-features.c b/sysdeps/x86/tst-get-cpu-features.c
> index dcdb86bb93..b5e7f6e7b0 100644
> --- a/sysdeps/x86/tst-get-cpu-features.c
> +++ b/sysdeps/x86/tst-get-cpu-features.c
> @@ -301,6 +301,7 @@ do_test (void)
> CHECK_CPU_FEATURE_USABLE (OSPKE);
> CHECK_CPU_FEATURE_USABLE (WAITPKG);
> CHECK_CPU_FEATURE_USABLE (AVX512_VBMI2);
> + CHECK_CPU_FEATURE_USABLE (SHSTK);
OK.
> CHECK_CPU_FEATURE_USABLE (GFNI);
> CHECK_CPU_FEATURE_USABLE (VAES);
> CHECK_CPU_FEATURE_USABLE (VPCLMULQDQ);
> @@ -324,6 +325,7 @@ do_test (void)
> CHECK_CPU_FEATURE_USABLE (HYBRID);
> CHECK_CPU_FEATURE_USABLE (TSXLDTRK);
> CHECK_CPU_FEATURE_USABLE (PCONFIG);
> + CHECK_CPU_FEATURE_USABLE (IBT);
OK.
> CHECK_CPU_FEATURE_USABLE (AMX_BF16);
> CHECK_CPU_FEATURE_USABLE (AVX512_FP16);
> CHECK_CPU_FEATURE_USABLE (AMX_TILE);
> --
> 2.29.2
>
--
Cheers,
Carlos.
next prev parent reply other threads:[~2021-01-29 2:42 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-27 12:27 H.J. Lu
2021-01-28 17:51 ` Adhemerval Zanella
2021-01-28 18:57 ` V2 " H.J. Lu
2021-01-29 2:42 ` Carlos O'Donell [this message]
2021-01-29 11:45 ` Adhemerval Zanella
2021-01-29 2:45 ` Carlos O'Donell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c0322c07-7751-678f-5c7b-72cdfb3d9adf@redhat.com \
--to=carlos@redhat.com \
--cc=adhemerval.zanella@linaro.org \
--cc=fweimer@redhat.com \
--cc=hjl.tools@gmail.com \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).