From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id DBDC63874C2C for ; Fri, 29 Jan 2021 02:42:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org DBDC63874C2C Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-237-PcPxdc6dOqqfC0jPjzU-vg-1; Thu, 28 Jan 2021 21:42:37 -0500 X-MC-Unique: PcPxdc6dOqqfC0jPjzU-vg-1 Received: by mail-qk1-f198.google.com with SMTP id a75so5895977qkg.16 for ; Thu, 28 Jan 2021 18:42:37 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=pY1PZuSBCnyka+ZAV8xjfnZ+umZqOhGr1AGr9iaeq20=; b=pvQNVKcInqe+dWAF7HCtdqSlfZk2YeRtoj+DvByQAqgxQPjvNNrG7Bx2n2f1w9GeFQ TpwkoiR8uuZaGksoYoYFo3d1TjU3jpic7rV/9sO2VZAZsGxX74Ho1+UECPn7V4p0ThQ2 jas3rm2Wxf33+zVaUaqmML9cs6sfOM0LutHJuFeA4csLD7/GZ2rBjGkANTBW2UXL2YR1 iwF/sW/DpZaGPuAKai3/SqEy0vxswZtzTr5bAJKwrGMvIp8BNKOLnQp09kTtUPXgeRpl xpIS+Mi5829iqyPcoHscxBQyU46VCRzbZzpskluh/teiCJ407rB/jmeJENOzCieD2A7Y Oh/A== X-Gm-Message-State: AOAM530B8LGzzRBcRMDnSL1hMyHYi36nKNGdiX/AmKb70WCpdaIdAKm7 PxxlLhy+ZOCrMeuR3INkRLSsR7M3zTeDfRTenwXmCBCAcz2jpbjhyneKxkmf2fKjbrAZ898fFi9 q9phaSBfwNLbgg4dKcyHI X-Received: by 2002:ad4:576a:: with SMTP id r10mr2190744qvx.29.1611888156376; Thu, 28 Jan 2021 18:42:36 -0800 (PST) X-Google-Smtp-Source: ABdhPJyFoT4cLSSazRgx9hP3wHiDOMt8m621Ha6ZR3mGQpIyGzRQEBkTCrMnUk0nR3KOuLQj4r5KhQ== X-Received: by 2002:ad4:576a:: with SMTP id r10mr2190731qvx.29.1611888156116; Thu, 28 Jan 2021 18:42:36 -0800 (PST) Received: from [192.168.1.16] (198-84-214-74.cpe.teksavvy.com. [198.84.214.74]) by smtp.gmail.com with ESMTPSA id z20sm4275317qki.93.2021.01.28.18.42.34 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 28 Jan 2021 18:42:35 -0800 (PST) Subject: Re: V2 [PATCH] x86: Properly set usable CET feature bits [BZ #26625] To: "H.J. Lu" , Adhemerval Zanella Cc: GNU C Library , Florian Weimer References: <20210127122702.3044784-1-hjl.tools@gmail.com> <17c201f1-fbab-a603-1c24-6bca65d5cf78@linaro.org> From: Carlos O'Donell Organization: Red Hat Message-ID: Date: Thu, 28 Jan 2021 21:42:34 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 Jan 2021 02:42:42 -0000 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 > From 65a9ca9c20db7ffd61f3defb4083b513589c7231 Mon Sep 17 00:00:00 2001 > From: "H.J. Lu" > 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 > 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 > + . */ > + > +#include > +#include > +#include > +#include > + > +/* 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 > 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 > + . */ > + > +#include > +#include > +#include > +#include > +#include > + > +/* 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 > 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.