From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 117357 invoked by alias); 6 Apr 2018 04:46:54 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 117345 invoked by uid 89); 6 Apr 2018 04:46:53 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-22.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE,UNSUBSCRIBE_BODY autolearn=ham version=3.3.2 spammy=pain, filling, priv, Overall X-HELO: mail-qk0-f171.google.com 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=LKVVSnnKjpVfNmZaNjasUAGnpUycilgH7M4TeUI+oRw=; b=clQSoAhUtLsWpC67UOMZ/9LZIL9O/0KruRWo0zmcFIUS7rtUtARPfhCfjrEMNDuTgB skmPNh+T1Ev+6aZWNwJcFPPg+v0nubzmWRu/CQnO2OHX2OE1XCSg75VqJxAsfmLeaZw9 xIdc9HMKkDy1p8dYuRXRlvH60T56k3inCo7/vaBr7CZuaQg4syUyXdXJtBlFtnDgIjdr drtMHWeh64rVFdEMyiBUfj4VW6p2ySce3gfXe5RICkizUGB4jDBv8rIBDjdWEPLC9pw9 de9ABhb9/pEcOdYDmuy2fjE1J45S518ZGOcLk5jesrKC8fcNH/04oclnMtxJxtKYMaen yERg== X-Gm-Message-State: ALQs6tACGGmevHV8GQnFvndnrhPGK9V9wM1RhLd27+HqohnCXEe1TrNo GXUwHM1wSKaz4/9i9y/sJf6pm6OkqJQ= X-Google-Smtp-Source: AIpwx484dmATWgxxRZMZXT7O6yr3ZMpBwIt8/xYlu0c8p0HbazG02O0MIb0P6v9Tm8LhRgpBsf2r+g== X-Received: by 10.55.121.133 with SMTP id u127mr34243767qkc.130.1522990007672; Thu, 05 Apr 2018 21:46:47 -0700 (PDT) Subject: Re: [PATCH] x86: Use pad in pthread_unwind_buf to preserve shadow stack register To: "H.J. Lu" , Florian Weimer Cc: Joseph Myers , "Tsimbalist, Igor V" , GNU C Library References: From: Carlos O'Donell Message-ID: <61a5b452-e59e-dfef-4530-a94a60480961@redhat.com> Date: Fri, 06 Apr 2018 04:46:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-04/txt/msg00150.txt.bz2 On 03/30/2018 12:41 PM, H.J. Lu wrote: > On Thu, Mar 29, 2018 at 1:20 PM, Florian Weimer wrote: >> * H. J. Lu: >> >>> On Thu, Mar 29, 2018 at 1:15 PM, Florian Weimer wrote: >>>> * H. J. Lu: >>>> >>>>> You need to make a choice. You either don't introduce a new symbol >>>>> version or don't save shadow stack for thread cancellation. You >>>>> can't have both. >>>> I don't understand. We have room to save the shadow stack pointer in >>>> the existing struct. >>> No, we don't have room in struct pthread_unwind_buf: >>> >>> Note: There is an unused pointer space in pthread_unwind_buf_data. But >>> it isn't suitable for saving and restoring shadow stack register since >>> x32 is a 64-bit process with 32-bit software pointer and kernel may >>> place x32 shadow stack above 4GB. We need to save and restore 64-bit >>> shadow stack register for x32. >> We have for void * fields. They are subsequently overwritten by >> __pthread_register_cancel. But __sigsetjmp can write to them first >> without causing any harm. We just need a private __longjmp_cancel >> that doesn't restore the shadow stack pointer. > Here is the patch which does that. Any comments? OK, so I have reviewed https://github.com/hjl-tools/glibc/ hjl/cet/master, please confirm that this is the correct branch of the required implementation for CET. It really helps to review the rest of the patch set, you should be preparing this as a patch set instead of having it reviewed one-at-a-time. This issue was already raised in the thread with Zack. Architecture: * We avoid a "flag day" with feature_1 TCB flag to switch to a new ABI, which we have discussed is a fragile process which should be avoided if a supportable alternative solution exists. * We avoid versioned symbols, this makes CET backportable, and this has a bigger benefit for long-term stable distributions. * A key design problem has been that cancellation jump buffers within glibc are truncated to optimize on-stack size, and this means that setjmp could write beyond the structure because setjmp now tries to save the shadowstack pointer into space that the cancellation jump buffer did not allocate. For the record this optimization seems premature and I'm sad we did it, this is a lesson we should learn from. * We have all agreed to the following concepts: * The cancellation process, in particular the unwinder, never returns from any of the functions we call, it just keeps calling into the unwinder to jump to the next unwound cancellation function all the way to the thread start routine. Therefore because we never return from one of these functions we never need to restore the shadow stack, and consequently wherever it is stored in the cancellation jump buffer can be overwritten if we need the space (it's a dead store). * The corollary to this is that function calls made from cancellation handlers will continue to advance the shadowstack from the deepest point at which cancellation is initiated from. This means that the depth of the shadowstack doesn't match the depth of the real stack while we are unwinding. I don't know if this will have consequences on analysis tooling or not, or debug tools during unwinding. It's a fairly advanced situation and corner case, and restoring the shadowstack is not useful becuase we don't need it and simplifies the implementation. * The cancellation jump buffer has private data used for chaining the cancel jump buffers together such that the custom unwinder can follow them and call them in sequence. This space constitutes 4 void *'s which is space that setjmp can write to, because we will just overwrite it when we register the cancel handler. * If the new shadowstack-enabled setjmp stores the shadowstack pointer into the space taken by the 4 void*'s then we won't overflow the stack, and we don't need to change the layout of the cancellation jump buffer. The 4 void*'s are sufficient, even for x32 to write a 64-bit shadow stack address. * After fixing the cancellation jump buffers the following work needs to be reviewed: * Add feature_1 in tcb head to track CET status and make it easily available to runtime for checking. * Save and restore shadowstack in setjmp/longjmp. * Add CET support to ld.so et. al. and track runtime status. * Adjust vfork for shadow stack usage. * Add ENDBR or NOTRACK where required in assembly. * CET and makecontext incompatible. - Probably need to discuss which default is appropriate. - Should the user get CET automatically disabled in makecontext() et. al. silently? - Should your current solution, which is to error out during the build, and require flag changes, be the default? This forces the user to review the security for their application. * prctl for CET. * The work to review after this patch appears to be less contentious in terms of the kinds of changes that are required. Most of the changes are internal details of enabling CET and not ABI details, with the exception of the possible pain we might cause with makecontext() being unsupported and what default position to take there. Design: * Overall the implementation looks exactly how I might expect it to look, but some of the math that places the shadowstack pointer appears to need either commenting or fixing because I don't understand it. You need to make it easy for me to see that we have placed the shadowstack pointer into the 4 pad words. Details: * One comment needs filling out a bit more, noted below. > 0001-x86-Use-pad-in-pthread_unwind_buf-to-preserve-shadow.patch > > > From f222537447f5ec879427f318b7c0396362b7453a Mon Sep 17 00:00:00 2001 > From: "H.J. Lu" > Date: Sat, 24 Feb 2018 17:23:54 -0800 > Subject: [PATCH] x86: Use pad in pthread_unwind_buf to preserve shadow stack > register > > The pad array in struct pthread_unwind_buf is used by setjmp to save > shadow stack register. We assert that size of struct pthread_unwind_buf > is no less than offset of shadow stack pointer + shadow stack pointer > size. > OK. > Since functions, like LIBC_START_MAIN, START_THREAD_DEFN as well as > these with thread cancellation, call setjmp, but never return after > __libc_unwind_longjmp, __libc_unwind_longjmp, which is defined as > __libc_longjmp on x86, doesn't need to restore shadow stack register. OK. > __libc_longjmp, which is a private interface for thread cancellation > implementation in libpthread, is changed to call __longjmp_cancel, > instead of __longjmp. __longjmp_cancel is a new internal function > in libc, which is similar to __longjmp, but doesn't restore shadow > stack register. OK. Good. I like the use of a __longjmp_cancel name to call out what's going on in the API (clear semantics). > > The compatibility longjmp and siglongjmp in libpthread.so are changed > to call __libc_siglongjmp, instead of __libc_longjmp, so that they will > restore shadow stack register. OK. > > Tested with build-many-glibcs.py. > > * nptl/pthread_create.c (START_THREAD_DEFN): Clear previous > handlers after setjmp. > * setjmp/longjmp.c (__libc_longjmp): Don't define alias if > defined. > * sysdeps/unix/sysv/linux/x86/setjmpP.h (_JUMP_BUF_SIGSET_NSIG): > Changed to 97. > * sysdeps/x86/Makefile (sysdep_routines): Add __longjmp_cancel. > * sysdeps/x86/__longjmp_cancel.S: New file. > * sysdeps/x86/longjmp.c: Likewise. > * sysdeps/x86/nptl/pt-longjmp.c: Likewise. This looks much better. > --- > nptl/pthread_create.c | 9 ++-- > setjmp/longjmp.c | 2 + > sysdeps/unix/sysv/linux/x86/setjmpP.h | 4 +- > sysdeps/x86/Makefile | 4 ++ > sysdeps/x86/__longjmp_cancel.S | 20 ++++++++ > sysdeps/x86/longjmp.c | 45 ++++++++++++++++ > sysdeps/x86/nptl/pt-longjmp.c | 97 +++++++++++++++++++++++++++++++++++ > 7 files changed, 176 insertions(+), 5 deletions(-) > create mode 100644 sysdeps/x86/__longjmp_cancel.S > create mode 100644 sysdeps/x86/longjmp.c > create mode 100644 sysdeps/x86/nptl/pt-longjmp.c > > diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c > index caaf07c134..1c5b3780c6 100644 > --- a/nptl/pthread_create.c > +++ b/nptl/pthread_create.c > @@ -427,12 +427,15 @@ START_THREAD_DEFN > compilers without that support we do use setjmp. */ > struct pthread_unwind_buf unwind_buf; > > - /* No previous handlers. */ > + int not_first_call; > + not_first_call = setjmp ((struct __jmp_buf_tag *) unwind_buf.cancel_jmp_buf); > + > + /* No previous handlers. NB: This must be done after setjmp since > + the same space may be used by setjmp to store extra data which > + should never be used by __libc_unwind_longjmp. */ Suggest: ~~~ No previous handlers. NB: This must be done after setjmp since the private space in the unwind jump buffer may overlap space used by setjmp to store extra architecture-specific information which is never be used by the cancellation-specific __libc_unwind_longjmp. The private space is allowed to overlap because the unwinder never has to return through any of the jumped-to call frames, and thus only a minimum amount of saved data need be stored, and for example, need not include the process signal mask information. This is all an optimization to reduce stack usage when pushing cancellation handlers. ~~~ > unwind_buf.priv.data.prev = NULL; > unwind_buf.priv.data.cleanup = NULL; > > - int not_first_call; > - not_first_call = setjmp ((struct __jmp_buf_tag *) unwind_buf.cancel_jmp_buf); OK. > if (__glibc_likely (! not_first_call)) > { > /* Store the new cleanup handler info. */ > diff --git a/setjmp/longjmp.c b/setjmp/longjmp.c > index a2a7065a85..453889e103 100644 > --- a/setjmp/longjmp.c > +++ b/setjmp/longjmp.c > @@ -40,9 +40,11 @@ __libc_siglongjmp (sigjmp_buf env, int val) > } > > #ifndef __libc_siglongjmp > +# ifndef __libc_longjmp > /* __libc_longjmp is a private interface for cancellation implementation > in libpthread. */ > strong_alias (__libc_siglongjmp, __libc_longjmp) > +# endif OK. > weak_alias (__libc_siglongjmp, _longjmp) > weak_alias (__libc_siglongjmp, longjmp) > weak_alias (__libc_siglongjmp, siglongjmp) > diff --git a/sysdeps/unix/sysv/linux/x86/setjmpP.h b/sysdeps/unix/sysv/linux/x86/setjmpP.h > index c0ed767a0d..90a6bbcf32 100644 > --- a/sysdeps/unix/sysv/linux/x86/setjmpP.h > +++ b/sysdeps/unix/sysv/linux/x86/setjmpP.h > @@ -22,8 +22,8 @@ > #include > > /* The biggest signal number + 1. As of kernel 4.14, x86 _NSIG is 64. > - Define it to 513 to leave some rooms for future use. */ > -#define _JUMP_BUF_SIGSET_NSIG 513 > + Define it to 97 to leave some rooms for future use. */ OK. > +#define _JUMP_BUF_SIGSET_NSIG 97 Please provide proof in the way of a comment or rewriting this constant to show that it places the shadow stack pointer on both x86_64 and x32 into the range of the private pad. Also, from commit f33632ccd1dec3217583fcfdd965afb62954203c, where did this math come from? ((_JUMP_BUF_SIGSET_NSIG - 1 + 7) / (8 * sizeof (unsigned long int))) Why the +7? > /* Number of longs to hold all signals. */ > #define _JUMP_BUF_SIGSET_NWORDS \ > ((_JUMP_BUF_SIGSET_NSIG - 1 + 7) / (8 * sizeof (unsigned long int))) > diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile > index 0d0326c21a..d25d6f0ae4 100644 > --- a/sysdeps/x86/Makefile > +++ b/sysdeps/x86/Makefile > @@ -8,3 +8,7 @@ sysdep-dl-routines += dl-get-cpu-features > tests += tst-get-cpu-features > tests-static += tst-get-cpu-features-static > endif > + > +ifeq ($(subdir),setjmp) > +sysdep_routines += __longjmp_cancel > +endif OK. > diff --git a/sysdeps/x86/__longjmp_cancel.S b/sysdeps/x86/__longjmp_cancel.S > new file mode 100644 > index 0000000000..b57dbfa376 > --- /dev/null > +++ b/sysdeps/x86/__longjmp_cancel.S > @@ -0,0 +1,20 @@ > +/* __longjmp_cancel for x86. > + 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 > + . */ > + > +#define __longjmp __longjmp_cancel > +#include <__longjmp.S> OK. > diff --git a/sysdeps/x86/longjmp.c b/sysdeps/x86/longjmp.c > new file mode 100644 > index 0000000000..a53f31e1dd > --- /dev/null > +++ b/sysdeps/x86/longjmp.c > @@ -0,0 +1,45 @@ > +/* __libc_siglongjmp for x86. > + 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 > + . */ > + > +#define __libc_longjmp __redirect___libc_longjmp > +#include > +#undef __libc_longjmp > + > +extern void __longjmp_cancel (__jmp_buf __env, int __val) > + __attribute__ ((__noreturn__)) attribute_hidden; > + > +/* Since __libc_longjmp is a private interface for cancellation > + implementation in libpthread, there is no need to restore shadow > + stack register. */ > + > +void > +__libc_longjmp (sigjmp_buf env, int val) > +{ > + /* Perform any cleanups needed by the frames being unwound. */ > + _longjmp_unwind (env, val); OK. > + > + if (env[0].__mask_was_saved) > + /* Restore the saved signal mask. */ > + (void) __sigprocmask (SIG_SETMASK, > + (sigset_t *) &env[0].__saved_mask, > + (sigset_t *) NULL); OK. > + > + /* Call the machine-dependent function to restore machine state > + without shadow stack. */ > + __longjmp_cancel (env[0].__jmpbuf, val ?: 1); OK. > +} > diff --git a/sysdeps/x86/nptl/pt-longjmp.c b/sysdeps/x86/nptl/pt-longjmp.c > new file mode 100644 > index 0000000000..7eb8651cfe > --- /dev/null > +++ b/sysdeps/x86/nptl/pt-longjmp.c > @@ -0,0 +1,97 @@ > +/* ABI compatibility for 'longjmp' and 'siglongjmp' symbols in libpthread ABI. > + X86 version. > + Copyright (C) 18 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 > + . */ > + > +/* has > + > +struct pthread_unwind_buf > +{ > + struct > + { > + __jmp_buf jmp_buf; > + int mask_was_saved; > + } cancel_jmp_buf[1]; > + > + union > + { > + void *pad[4]; > + struct > + { > + struct pthread_unwind_buf *prev; > + struct _pthread_cleanup_buffer *cleanup; > + int canceltype; > + } data; > + } priv; > +}; > + > + The pad array in struct pthread_unwind_buf is used by setjmp to save This appears to be SHADOW_STACK_POINTER_OFFSET in subsequent patches. However on your hjl/cet/master branch it appears that this offset is not defined to be *just after* the mask_was_saved? > + shadow stack register. Assert that size of struct pthread_unwind_buf > + is no less than offset of shadow stack pointer plus shadow stack > + pointer size. > + > + NB: setjmp is called in libpthread to save shadow stack register. But > + __libc_unwind_longjmp doesn't restore shadow stack register since they > + never return after longjmp. */ > + > +#include > +#include > + > +#ifdef __x86_64__ > +# define SHADOW_STACK_POINTER_SIZE 8 > +#else > +# define SHADOW_STACK_POINTER_SIZE 4 > +#endif > + > +_Static_assert ((sizeof (struct pthread_unwind_buf) > + >= (SHADOW_STACK_POINTER_OFFSET > + + SHADOW_STACK_POINTER_SIZE)), > + "size of struct pthread_unwind_buf < " > + "(SHADOW_STACK_POINTER_OFFSET + SHADOW_STACK_POINTER_SIZE)"); OK. > + > +#include > + > +/* libpthread once had its own longjmp (and siglongjmp alias), though there > + was no apparent reason for it. There is no use in having a separate > + symbol in libpthread, but the historical ABI requires it. For static > + linking, there is no need to provide anything here--the libc version > + will be linked in. For shared library ABI compatibility, there must be > + longjmp and siglongjmp symbols in libpthread.so. > + > + With an IFUNC resolver, it would be possible to avoid the indirection, > + but the IFUNC resolver might run before the __libc_longjmp symbol has > + been relocated, in which case the IFUNC resolver would not be able to > + provide the correct address. */ > + > +#if SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_22) > + > +static void __attribute__ ((noreturn, used)) > +longjmp_compat (jmp_buf env, int val) > +{ > + /* NB: We call __libc_siglongjmp, instead of __libc_longjmp, since > + __libc_longjmp is a private interface for cancellation which > + doesn't restore shadow stack register. */ > + __libc_siglongjmp (env, val); OK. > +} > + > +strong_alias (longjmp_compat, longjmp_alias) > +compat_symbol (libpthread, longjmp_alias, longjmp, GLIBC_2_0); > + > +strong_alias (longjmp_alias, siglongjmp_alias) > +compat_symbol (libpthread, siglongjmp_alias, siglongjmp, GLIBC_2_0); > + > +#endif > -- 2.14.3 -- Cheers, Carlos.