Message ID | 20180713131908.GB2606@gmail.com |
---|---|
State | New |
Headers | show |
Series | V2: [PATCH 01/24] x86: Rename __glibc_reserved1 to feature_1 in tcbhead_t [BZ #22563] | expand |
On 07/13/2018 09:19 AM, H.J. Lu wrote: > On Wed, Jun 13, 2018 at 08:31:44AM -0700, H.J. Lu wrote: >> This will be used by CET run-time control. >> >> [BZ #22563] >> * nptl/pthread_create.c (__pthread_create_2_1): Use >> THREAD_COPY_ADDITONAL_INFO to copy additonal info if defined. >> * sysdeps/i386/nptl/tcb-offsets.sym (FEATURE_1_OFFSET): New. >> * sysdeps/x86_64/nptl/tcb-offsets.sym (FEATURE_1_OFFSET): >> Likewise. >> * sysdeps/i386/nptl/tls.h (tcbhead_t): Rename __glibc_reserved1 >> to feature_1. >> * sysdeps/x86_64/nptl/tls.h (tcbhead_t): Likewise. >> * sysdeps/unix/sysv/linux/x86/pthreaddef.h: New file. > > Here is the updated patch to add feature_1 to tcbhead_t and > introduce macros for CET enabling. OK for master? Fix the typo-prone macro API and post a v3 please. Thank you. > > H.J. > ---- > feature_1 has X86_FEATURE_1_IBT and X86_FEATURE_1_SHSTK bits for CET > run-time control. > > CET_ENABLED, IBT_ENABLED and SHSTK_ENABLED are defined to 1 or 0 to > indicate that if CET, IBT and SHSTK are enabled. OK. > > [BZ #22563] > * nptl/pthread_create.c (__pthread_create_2_1): Use > THREAD_COPY_ADDITONAL_INFO to copy additonal info if defined. > * sysdeps/i386/nptl/tcb-offsets.sym (FEATURE_1_OFFSET): New. > * sysdeps/x86_64/nptl/tcb-offsets.sym (FEATURE_1_OFFSET): > Likewise. > * sysdeps/i386/nptl/tls.h (tcbhead_t): Rename __glibc_reserved1 > to feature_1. > * sysdeps/x86_64/nptl/tls.h (tcbhead_t): Likewise. > * sysdeps/unix/sysv/linux/x86/pthreaddef.h: New file. > * sysdeps/x86/sysdep.h (X86_FEATURE_1_IBT): New. > (X86_FEATURE_1_SHSTK): Likewise. > (CET_ENABLED): Likewise. > (IBT_ENABLED): Likewise. > (SHSTK_ENABLED): Likewise. > --- > nptl/pthread_create.c | 5 +++++ > sysdeps/i386/nptl/tcb-offsets.sym | 1 + > sysdeps/i386/nptl/tls.h | 5 ++++- > sysdeps/unix/sysv/linux/x86/pthreaddef.h | 24 +++++++++++++++++++++ > sysdeps/x86/sysdep.h | 27 ++++++++++++++++++++++++ > sysdeps/x86_64/nptl/tcb-offsets.sym | 1 + > sysdeps/x86_64/nptl/tls.h | 5 ++++- > 7 files changed, 66 insertions(+), 2 deletions(-) > create mode 100644 sysdeps/unix/sysv/linux/x86/pthreaddef.h > > diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c > index 92c945b12b..16998f4bbd 100644 > --- a/nptl/pthread_create.c > +++ b/nptl/pthread_create.c > @@ -712,6 +712,11 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr, > THREAD_COPY_POINTER_GUARD (pd); > #endif > > + /* Copy additonal info. */ > +#ifdef THREAD_COPY_ADDITONAL_INFO > + THREAD_COPY_ADDITONAL_INFO (pd); > +#endif This is a typo-prone macro API. Please find a way to define this unconditionally. > + > /* Verify the sysinfo bits were copied in allocate_stack if needed. */ > #ifdef NEED_DL_SYSINFO > CHECK_THREAD_SYSINFO (pd); > diff --git a/sysdeps/i386/nptl/tcb-offsets.sym b/sysdeps/i386/nptl/tcb-offsets.sym > index 7d7fe5e71c..fbac241c45 100644 > --- a/sysdeps/i386/nptl/tcb-offsets.sym > +++ b/sysdeps/i386/nptl/tcb-offsets.sym > @@ -12,3 +12,4 @@ CLEANUP offsetof (struct pthread, cleanup) > CLEANUP_PREV offsetof (struct _pthread_cleanup_buffer, __prev) > MUTEX_FUTEX offsetof (pthread_mutex_t, __data.__lock) > POINTER_GUARD offsetof (tcbhead_t, pointer_guard) > +FEATURE_1_OFFSET offsetof (tcbhead_t, feature_1) > diff --git a/sysdeps/i386/nptl/tls.h b/sysdeps/i386/nptl/tls.h > index afb71ce431..21e23cd809 100644 > --- a/sysdeps/i386/nptl/tls.h > +++ b/sysdeps/i386/nptl/tls.h > @@ -41,7 +41,10 @@ typedef struct > uintptr_t stack_guard; > uintptr_t pointer_guard; > int gscope_flag; > - int __glibc_reserved1; > + /* Bit 0: X86_FEATURE_1_IBT. > + Bit 1: X86_FEATURE_1_SHSTK. > + */ > + unsigned int feature_1; OK. > /* Reservation of some values for the TM ABI. */ > void *__private_tm[3]; > /* GCC split stack support. */ > diff --git a/sysdeps/unix/sysv/linux/x86/pthreaddef.h b/sysdeps/unix/sysv/linux/x86/pthreaddef.h > new file mode 100644 > index 0000000000..539f6540d0 > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/x86/pthreaddef.h > @@ -0,0 +1,24 @@ > +/* Pthread macros. Linux/x86 version. > + Copyright (C) 2018 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 > + <http://www.gnu.org/licenses/>. */ > + > +#include_next <pthreaddef.h> > + > +/* Wee need to copy feature_1 in pthread_create. */ > +#define THREAD_COPY_ADDITONAL_INFO(descr) \ > + ((descr)->header.feature_1 \ > + = THREAD_GETMEM (THREAD_SELF, header.feature_1)) OK. > diff --git a/sysdeps/x86/sysdep.h b/sysdeps/x86/sysdep.h > index afcb7cfd76..976566aa37 100644 > --- a/sysdeps/x86/sysdep.h > +++ b/sysdeps/x86/sysdep.h > @@ -21,6 +21,33 @@ > > #include <sysdeps/generic/sysdep.h> > > +/* __CET__ is defined by GCC with Control-Flow Protection values: > + > +enum cf_protection_level > +{ > + CF_NONE = 0, > + CF_BRANCH = 1 << 0, > + CF_RETURN = 1 << 1, > + CF_FULL = CF_BRANCH | CF_RETURN, > + CF_SET = 1 << 2 > +}; > +*/ > + > +/* Set if CF_BRANCH (IBT) is enabled. */ > +#define X86_FEATURE_1_IBT (1U << 0) OK. Thanks for defining this, it makes the assembly easier to read. > +/* Set if CF_RETURN (SHSTK) is enabled. */ > +#define X86_FEATURE_1_SHSTK (1U << 1) OK. > + > +#ifdef __CET__ > +# define CET_ENABLED 1 > +# define IBT_ENABLED (__CET__ & X86_FEATURE_1_IBT) > +# define SHSTK_ENABLED (__CET__ & X86_FEATURE_1_SHSTK) > +#else > +# define CET_ENABLED 0 > +# define IBT_ENABLED 0 > +# define SHSTK_ENABLED 0 > +#endif > + > #ifdef __ASSEMBLER__ > > /* Syntactic details of assembler. */ > diff --git a/sysdeps/x86_64/nptl/tcb-offsets.sym b/sysdeps/x86_64/nptl/tcb-offsets.sym > index be63404a16..387621e88c 100644 > --- a/sysdeps/x86_64/nptl/tcb-offsets.sym > +++ b/sysdeps/x86_64/nptl/tcb-offsets.sym > @@ -12,6 +12,7 @@ MUTEX_FUTEX offsetof (pthread_mutex_t, __data.__lock) > MULTIPLE_THREADS_OFFSET offsetof (tcbhead_t, multiple_threads) > POINTER_GUARD offsetof (tcbhead_t, pointer_guard) > VGETCPU_CACHE_OFFSET offsetof (tcbhead_t, vgetcpu_cache) > +FEATURE_1_OFFSET offsetof (tcbhead_t, feature_1) OK. > > -- Not strictly offsets, but these values are also used in the TCB. > TCB_CANCELSTATE_BITMASK CANCELSTATE_BITMASK > diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h > index 65c0051dcf..f042a0250a 100644 > --- a/sysdeps/x86_64/nptl/tls.h > +++ b/sysdeps/x86_64/nptl/tls.h > @@ -51,7 +51,10 @@ typedef struct > uintptr_t stack_guard; > uintptr_t pointer_guard; > unsigned long int vgetcpu_cache[2]; > - int __glibc_reserved1; > + /* Bit 0: X86_FEATURE_1_IBT. > + Bit 1: X86_FEATURE_1_SHSTK. > + */ > + unsigned int feature_1; OK. > int __glibc_unused1; > /* Reservation of some values for the TM ABI. */ > void *__private_tm[4]; > Cheers, Carlos.
On 07/13/2018 12:51 PM, Carlos O'Donell wrote: > On 07/13/2018 09:19 AM, H.J. Lu wrote: >> On Wed, Jun 13, 2018 at 08:31:44AM -0700, H.J. Lu wrote: >>> This will be used by CET run-time control. >>> >>> [BZ #22563] >>> * nptl/pthread_create.c (__pthread_create_2_1): Use >>> THREAD_COPY_ADDITONAL_INFO to copy additonal info if defined. >>> * sysdeps/i386/nptl/tcb-offsets.sym (FEATURE_1_OFFSET): New. >>> * sysdeps/x86_64/nptl/tcb-offsets.sym (FEATURE_1_OFFSET): >>> Likewise. >>> * sysdeps/i386/nptl/tls.h (tcbhead_t): Rename __glibc_reserved1 >>> to feature_1. >>> * sysdeps/x86_64/nptl/tls.h (tcbhead_t): Likewise. >>> * sysdeps/unix/sysv/linux/x86/pthreaddef.h: New file. >> >> Here is the updated patch to add feature_1 to tcbhead_t and >> introduce macros for CET enabling. OK for master? > > Fix the typo-prone macro API and post a v3 please. > > Thank you. Umm, has this been tested with the sanitizers? I thought they used that field. jeff
On 07/13/2018 02:55 PM, Jeff Law wrote: > On 07/13/2018 12:51 PM, Carlos O'Donell wrote: >> On 07/13/2018 09:19 AM, H.J. Lu wrote: >>> On Wed, Jun 13, 2018 at 08:31:44AM -0700, H.J. Lu wrote: >>>> This will be used by CET run-time control. >>>> >>>> [BZ #22563] >>>> * nptl/pthread_create.c (__pthread_create_2_1): Use >>>> THREAD_COPY_ADDITONAL_INFO to copy additonal info if defined. >>>> * sysdeps/i386/nptl/tcb-offsets.sym (FEATURE_1_OFFSET): New. >>>> * sysdeps/x86_64/nptl/tcb-offsets.sym (FEATURE_1_OFFSET): >>>> Likewise. >>>> * sysdeps/i386/nptl/tls.h (tcbhead_t): Rename __glibc_reserved1 >>>> to feature_1. >>>> * sysdeps/x86_64/nptl/tls.h (tcbhead_t): Likewise. >>>> * sysdeps/unix/sysv/linux/x86/pthreaddef.h: New file. >>> >>> Here is the updated patch to add feature_1 to tcbhead_t and >>> introduce macros for CET enabling. OK for master? >> >> Fix the typo-prone macro API and post a v3 please. >> >> Thank you. > Umm, has this been tested with the sanitizers? I thought they used that > field. The size of tcbhead_t has not changed, which is something that might impact the santiziers. But now that you mention it, why do I vaguely remember a conversation about the santizers using the reserved bytes as storage for themselves? Is that what you are talking about? HJ, could you look into this please? I think the sanitizers are not within their rights to use any bytes in the tcbhead_t structure, particularly reserved bytes. We should coordinate with them, but that should not stop the acceptance of this patch in 2.28. Cheers, Carlos.
On 07/13/2018 01:05 PM, Carlos O'Donell wrote: > On 07/13/2018 02:55 PM, Jeff Law wrote: >> On 07/13/2018 12:51 PM, Carlos O'Donell wrote: >>> On 07/13/2018 09:19 AM, H.J. Lu wrote: >>>> On Wed, Jun 13, 2018 at 08:31:44AM -0700, H.J. Lu wrote: >>>>> This will be used by CET run-time control. >>>>> >>>>> [BZ #22563] >>>>> * nptl/pthread_create.c (__pthread_create_2_1): Use >>>>> THREAD_COPY_ADDITONAL_INFO to copy additonal info if defined. >>>>> * sysdeps/i386/nptl/tcb-offsets.sym (FEATURE_1_OFFSET): New. >>>>> * sysdeps/x86_64/nptl/tcb-offsets.sym (FEATURE_1_OFFSET): >>>>> Likewise. >>>>> * sysdeps/i386/nptl/tls.h (tcbhead_t): Rename __glibc_reserved1 >>>>> to feature_1. >>>>> * sysdeps/x86_64/nptl/tls.h (tcbhead_t): Likewise. >>>>> * sysdeps/unix/sysv/linux/x86/pthreaddef.h: New file. >>>> >>>> Here is the updated patch to add feature_1 to tcbhead_t and >>>> introduce macros for CET enabling. OK for master? >>> >>> Fix the typo-prone macro API and post a v3 please. >>> >>> Thank you. >> Umm, has this been tested with the sanitizers? I thought they used that >> field. > > The size of tcbhead_t has not changed, which is something that might impact > the santiziers. > > But now that you mention it, why do I vaguely remember a conversation about > the santizers using the reserved bytes as storage for themselves? > > Is that what you are talking about? > > HJ, could you look into this please? > > I think the sanitizers are not within their rights to use any bytes in the > tcbhead_t structure, particularly reserved bytes. We should coordinate with > them, but that should not stop the acceptance of this patch in 2.28. sanitizer_common/sanitizer_linux.cc: static bool Aarch64GetESR(ucontext_t *ucontext, u64 *esr) { static const u32 kEsrMagic = 0x45535201; u8 *aux = ucontext->uc_mcontext.__reserved; while (true) { _aarch64_ctx *ctx = (_aarch64_ctx *)aux; if (ctx->size == 0) break; if (ctx->magic == kEsrMagic) { *esr = ((__sanitizer_esr_context *)ctx)->esr; return true; } aux += ctx->size; } return false; } It's seriously lame that they're abusing the reserved field like that... I see that HJ is just changing x86, so we may be OK here since the sanitizer code in question is aarch64 specific. Jeff
* Jeff Law: > On 07/13/2018 01:05 PM, Carlos O'Donell wrote: >> I think the sanitizers are not within their rights to use any bytes in the >> tcbhead_t structure, particularly reserved bytes. We should coordinate with >> them, but that should not stop the acceptance of this patch in 2.28. > sanitizer_common/sanitizer_linux.cc: > > static bool Aarch64GetESR(ucontext_t *ucontext, u64 *esr) { > static const u32 kEsrMagic = 0x45535201; > u8 *aux = ucontext->uc_mcontext.__reserved; > while (true) { > _aarch64_ctx *ctx = (_aarch64_ctx *)aux; > if (ctx->size == 0) break; > if (ctx->magic == kEsrMagic) { > *esr = ((__sanitizer_esr_context *)ctx)->esr; > return true; > } > aux += ctx->size; > } > return false; > } > > It's seriously lame that they're abusing the reserved field like that... In this case, __reserved doesn't mean “reserved for future use”, but “reserved as an allocation area for the kernel for dynamically sized data structures”. This is part of the kernel/userspace API.
On 07/13/2018 03:16 PM, Florian Weimer wrote: > * Jeff Law: > >> On 07/13/2018 01:05 PM, Carlos O'Donell wrote: >>> I think the sanitizers are not within their rights to use any bytes in the >>> tcbhead_t structure, particularly reserved bytes. We should coordinate with >>> them, but that should not stop the acceptance of this patch in 2.28. >> sanitizer_common/sanitizer_linux.cc: >> >> static bool Aarch64GetESR(ucontext_t *ucontext, u64 *esr) { >> static const u32 kEsrMagic = 0x45535201; >> u8 *aux = ucontext->uc_mcontext.__reserved; >> while (true) { >> _aarch64_ctx *ctx = (_aarch64_ctx *)aux; >> if (ctx->size == 0) break; >> if (ctx->magic == kEsrMagic) { >> *esr = ((__sanitizer_esr_context *)ctx)->esr; >> return true; >> } >> aux += ctx->size; >> } >> return false; >> } >> >> It's seriously lame that they're abusing the reserved field like that... > > In this case, __reserved doesn't mean “reserved for future use”, but > “reserved as an allocation area for the kernel for dynamically sized > data structures”. This is part of the kernel/userspace API. Right, and this isn't in the tcbhead_t structure either, this is the on-stack uc_mcontext. Cheers, Carlos.
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c index 92c945b12b..16998f4bbd 100644 --- a/nptl/pthread_create.c +++ b/nptl/pthread_create.c @@ -712,6 +712,11 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr, THREAD_COPY_POINTER_GUARD (pd); #endif + /* Copy additonal info. */ +#ifdef THREAD_COPY_ADDITONAL_INFO + THREAD_COPY_ADDITONAL_INFO (pd); +#endif + /* Verify the sysinfo bits were copied in allocate_stack if needed. */ #ifdef NEED_DL_SYSINFO CHECK_THREAD_SYSINFO (pd); diff --git a/sysdeps/i386/nptl/tcb-offsets.sym b/sysdeps/i386/nptl/tcb-offsets.sym index 7d7fe5e71c..fbac241c45 100644 --- a/sysdeps/i386/nptl/tcb-offsets.sym +++ b/sysdeps/i386/nptl/tcb-offsets.sym @@ -12,3 +12,4 @@ CLEANUP offsetof (struct pthread, cleanup) CLEANUP_PREV offsetof (struct _pthread_cleanup_buffer, __prev) MUTEX_FUTEX offsetof (pthread_mutex_t, __data.__lock) POINTER_GUARD offsetof (tcbhead_t, pointer_guard) +FEATURE_1_OFFSET offsetof (tcbhead_t, feature_1) diff --git a/sysdeps/i386/nptl/tls.h b/sysdeps/i386/nptl/tls.h index afb71ce431..21e23cd809 100644 --- a/sysdeps/i386/nptl/tls.h +++ b/sysdeps/i386/nptl/tls.h @@ -41,7 +41,10 @@ typedef struct uintptr_t stack_guard; uintptr_t pointer_guard; int gscope_flag; - int __glibc_reserved1; + /* Bit 0: X86_FEATURE_1_IBT. + Bit 1: X86_FEATURE_1_SHSTK. + */ + unsigned int feature_1; /* Reservation of some values for the TM ABI. */ void *__private_tm[3]; /* GCC split stack support. */ diff --git a/sysdeps/unix/sysv/linux/x86/pthreaddef.h b/sysdeps/unix/sysv/linux/x86/pthreaddef.h new file mode 100644 index 0000000000..539f6540d0 --- /dev/null +++ b/sysdeps/unix/sysv/linux/x86/pthreaddef.h @@ -0,0 +1,24 @@ +/* Pthread macros. Linux/x86 version. + Copyright (C) 2018 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 + <http://www.gnu.org/licenses/>. */ + +#include_next <pthreaddef.h> + +/* Wee need to copy feature_1 in pthread_create. */ +#define THREAD_COPY_ADDITONAL_INFO(descr) \ + ((descr)->header.feature_1 \ + = THREAD_GETMEM (THREAD_SELF, header.feature_1)) diff --git a/sysdeps/x86/sysdep.h b/sysdeps/x86/sysdep.h index afcb7cfd76..976566aa37 100644 --- a/sysdeps/x86/sysdep.h +++ b/sysdeps/x86/sysdep.h @@ -21,6 +21,33 @@ #include <sysdeps/generic/sysdep.h> +/* __CET__ is defined by GCC with Control-Flow Protection values: + +enum cf_protection_level +{ + CF_NONE = 0, + CF_BRANCH = 1 << 0, + CF_RETURN = 1 << 1, + CF_FULL = CF_BRANCH | CF_RETURN, + CF_SET = 1 << 2 +}; +*/ + +/* Set if CF_BRANCH (IBT) is enabled. */ +#define X86_FEATURE_1_IBT (1U << 0) +/* Set if CF_RETURN (SHSTK) is enabled. */ +#define X86_FEATURE_1_SHSTK (1U << 1) + +#ifdef __CET__ +# define CET_ENABLED 1 +# define IBT_ENABLED (__CET__ & X86_FEATURE_1_IBT) +# define SHSTK_ENABLED (__CET__ & X86_FEATURE_1_SHSTK) +#else +# define CET_ENABLED 0 +# define IBT_ENABLED 0 +# define SHSTK_ENABLED 0 +#endif + #ifdef __ASSEMBLER__ /* Syntactic details of assembler. */ diff --git a/sysdeps/x86_64/nptl/tcb-offsets.sym b/sysdeps/x86_64/nptl/tcb-offsets.sym index be63404a16..387621e88c 100644 --- a/sysdeps/x86_64/nptl/tcb-offsets.sym +++ b/sysdeps/x86_64/nptl/tcb-offsets.sym @@ -12,6 +12,7 @@ MUTEX_FUTEX offsetof (pthread_mutex_t, __data.__lock) MULTIPLE_THREADS_OFFSET offsetof (tcbhead_t, multiple_threads) POINTER_GUARD offsetof (tcbhead_t, pointer_guard) VGETCPU_CACHE_OFFSET offsetof (tcbhead_t, vgetcpu_cache) +FEATURE_1_OFFSET offsetof (tcbhead_t, feature_1) -- Not strictly offsets, but these values are also used in the TCB. TCB_CANCELSTATE_BITMASK CANCELSTATE_BITMASK diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h index 65c0051dcf..f042a0250a 100644 --- a/sysdeps/x86_64/nptl/tls.h +++ b/sysdeps/x86_64/nptl/tls.h @@ -51,7 +51,10 @@ typedef struct uintptr_t stack_guard; uintptr_t pointer_guard; unsigned long int vgetcpu_cache[2]; - int __glibc_reserved1; + /* Bit 0: X86_FEATURE_1_IBT. + Bit 1: X86_FEATURE_1_SHSTK. + */ + unsigned int feature_1; int __glibc_unused1; /* Reservation of some values for the TM ABI. */ void *__private_tm[4];