| Message ID | 2dc14d06-a06c-1aeb-dca3-f97f4ee68415@redhat.com |
|---|---|
| State | Superseded |
| Headers |
Received: (qmail 118393 invoked by alias); 12 Apr 2018 21:36:34 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 118352 invoked by uid 89); 12 Apr 2018 21:36:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-23.4 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= X-HELO: mail-qk0-f169.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=Zq9Td6YQyn1SGWpTqVNA0Y2VHL04qt5ePbbRrWa/Jrk=; b=QgGg8ZNsJS038eWkgdtU0GuQVndosGEmUIRYJ858kcs+z74jx33uUN6urVh1p1K9Il okuMraxpkKzdftHtNJ2RDoJ9sh/J9OQqq/8ehCpjM6TWEZkZeXb5MAX4vVadS2HDCM4q Pz4W6MFzxyr0znKRhcdN3SRfEy4WBny+CCQY7AzcOBmci6TVLWyNkXFNWYr5GCww6T9S KVD/kQOQMSirOSwFwIS7IjGD6mMWpEA4skdEDxIGEn4syqOIPz+IGZic0XUmuf/6JmIx uaGqryr+mYyXpev0igShZVcgbMjMPU8kOeGsOfQ+pKbaq1iPHASnS3Jb5jVf5ZvwuTVo BpHA== X-Gm-Message-State: ALQs6tCa1ceykyg/ZzMRE4Jy335D/lG90lxeicQas5+xceXwJp6wIMBT vBo35lTMv+Lzu6kpP1rkcLmyItjgjkQ= X-Google-Smtp-Source: AIpwx4/SY5WF3jhq5G+Q4Lh6jzUfcSNLGGjjJhgm8dyOi0118H2erD74Dsjj5jP6n10Jld+nLCXIAQ== X-Received: by 10.55.129.69 with SMTP id c66mr2495414qkd.116.1523568985358; Thu, 12 Apr 2018 14:36:25 -0700 (PDT) Subject: Re: [PATCH] x86: Use pad in pthread_unwind_buf to preserve shadow stack register To: "H.J. Lu" <hjl.tools@gmail.com> Cc: Florian Weimer <fw@deneb.enyo.de>, Joseph Myers <joseph@codesourcery.com>, "Tsimbalist, Igor V" <igor.v.tsimbalist@intel.com>, GNU C Library <libc-alpha@sourceware.org> References: <CAMe9rOpj6nYdP33emmepZ+KWFG2JDVNsHn1bve3oo5WQyFmduA@mail.gmail.com> <61a5b452-e59e-dfef-4530-a94a60480961@redhat.com> <CAMe9rOr68P6Z=rX=xeFG1PbQE1fZ14iN3NmZEpChjP6QmUfS8g@mail.gmail.com> From: Carlos O'Donell <carlos@redhat.com> Message-ID: <2dc14d06-a06c-1aeb-dca3-f97f4ee68415@redhat.com> Date: Thu, 12 Apr 2018 16:36:21 -0500 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: <CAMe9rOr68P6Z=rX=xeFG1PbQE1fZ14iN3NmZEpChjP6QmUfS8g@mail.gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit |
Commit Message
Carlos O'Donell
April 12, 2018, 9:36 p.m. UTC
On 04/06/2018 07:59 AM, H.J. Lu wrote: > On Thu, Apr 5, 2018 at 9:46 PM, Carlos O'Donell <carlos@redhat.com> wrote: >> On 03/30/2018 12:41 PM, H.J. Lu wrote: >>> On Thu, Mar 29, 2018 at 1:20 PM, Florian Weimer <fw@deneb.enyo.de> wrote: >>>> * H. J. Lu: >>>> >>>>> On Thu, Mar 29, 2018 at 1:15 PM, Florian Weimer <fw@deneb.enyo.de> 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. > > Thanks for your feedbacks. > >> 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. > > I'd like to reserve 4 slots in ucontext for shadow stack: > > https://github.com/hjl-tools/glibc/commit/9bf6aefa8fb45f8df140d42ce9cf890bb24076e1 > > It should be binary backward compatible. I will investigate if there is a way > to support shadow stack with existing API. Otherwise, we need to add a new > API for ucontext functions with shadow stack. Could you explain in detail how this is binary backwards compatible? Is the assumption that if you extend ucontext_t, that the kernel will just write more to the stack, and users who accesss it via the void* in a signal handler setup via sigaction + SA_SIGINFO, will not read past the size they expect? How is the shadow stack information moved between a getcontext -> setcontext set of API calls? The user ucontext_t in existing binaries is too small to hold the shadow stack? Would we again have a "flag day" where CET enablement must be coordinated with the definition of a new ucontext_t? >> * prctl for CET. > > We have been experimenting different approaches to get the best implementation. > I am expecting that this patch may change as we collect more data. > OK. >> * 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" <hjl.tools@gmail.com> >>> 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. >> ~~~ > > Will fix it. > >>> 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 <bits/types/__sigset_t.h> >>> >>> /* 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. > > sysdeps/x86/nptl/pt-longjmp.c has > > _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)"); > > If shadow stack pointer is saved in the offset bigger than the size of > struct pthread_unwind_buf, assert will trigger at compile-time. > Thanks, I'll review this in the patch you posted. >> Also, from commit f33632ccd1dec3217583fcfdd965afb62954203c, >> where did this math come from? >> >> ((_JUMP_BUF_SIGSET_NSIG - 1 + 7) / (8 * sizeof (unsigned long int))) >> >> Why the +7? > > _JUMP_BUF_SIGSET_NSIG is the biggest signal number + 1. Agreed. > _JUMP_BUF_SIGSET_NSIG - 1 gives the biggest signal number. Agreed. > _JUMP_BUF_SIGSET_NSIG - 1 + 7 rounds up to the number of bytes > which are needed to store the biggest signal number. It does not. Result # of signals # of bits Current # Expected # FAIL 1 64 0 1 FAIL 2 64 0 1 FAIL 3 64 0 1 FAIL 4 64 0 1 FAIL 5 64 0 1 FAIL 6 64 0 1 FAIL 7 64 0 1 FAIL 8 64 0 1 FAIL 9 64 0 1 FAIL 10 64 0 1 FAIL 11 64 0 1 FAIL 12 64 0 1 FAIL 13 64 0 1 FAIL 14 64 0 1 FAIL 15 64 0 1 FAIL 16 64 0 1 FAIL 17 64 0 1 FAIL 18 64 0 1 FAIL 19 64 0 1 FAIL 20 64 0 1 FAIL 21 64 0 1 FAIL 22 64 0 1 FAIL 23 64 0 1 FAIL 24 64 0 1 FAIL 25 64 0 1 FAIL 26 64 0 1 FAIL 27 64 0 1 FAIL 28 64 0 1 FAIL 29 64 0 1 FAIL 30 64 0 1 FAIL 31 64 0 1 FAIL 32 64 0 1 FAIL 33 64 0 1 FAIL 34 64 0 1 FAIL 35 64 0 1 FAIL 36 64 0 1 FAIL 37 64 0 1 FAIL 38 64 0 1 FAIL 39 64 0 1 FAIL 40 64 0 1 FAIL 41 64 0 1 FAIL 42 64 0 1 FAIL 43 64 0 1 FAIL 44 64 0 1 FAIL 45 64 0 1 FAIL 46 64 0 1 FAIL 47 64 0 1 FAIL 48 64 0 1 FAIL 49 64 0 1 FAIL 50 64 0 1 FAIL 51 64 0 1 FAIL 52 64 0 1 FAIL 53 64 0 1 FAIL 54 64 0 1 FAIL 55 64 0 1 FAIL 56 64 0 1 FAIL 57 64 0 1 PASS 58 64 1 1 PASS 59 64 1 1 PASS 60 64 1 1 PASS 61 64 1 1 PASS 62 64 1 1 PASS 63 64 1 1 PASS 64 64 1 1 FAIL 65 128 1 2 FAIL 66 128 1 2 FAIL 67 128 1 2 FAIL 68 128 1 2 FAIL 69 128 1 2 FAIL 70 128 1 2 FAIL 71 128 1 2 FAIL 72 128 1 2 FAIL 73 128 1 2 FAIL 74 128 1 2 FAIL 75 128 1 2 FAIL 76 128 1 2 FAIL 77 128 1 2 FAIL 78 128 1 2 FAIL 79 128 1 2 FAIL 80 128 1 2 FAIL 81 128 1 2 FAIL 82 128 1 2 FAIL 83 128 1 2 FAIL 84 128 1 2 FAIL 85 128 1 2 FAIL 86 128 1 2 FAIL 87 128 1 2 FAIL 88 128 1 2 FAIL 89 128 1 2 FAIL 90 128 1 2 FAIL 91 128 1 2 FAIL 92 128 1 2 FAIL 93 128 1 2 FAIL 94 128 1 2 FAIL 95 128 1 2 FAIL 96 128 1 2 FAIL 97 128 1 2 FAIL 98 128 1 2 FAIL 99 128 1 2 FAIL 100 128 1 2 FAIL 101 128 1 2 FAIL 102 128 1 2 FAIL 103 128 1 2 FAIL 104 128 1 2 FAIL 105 128 1 2 FAIL 106 128 1 2 FAIL 107 128 1 2 FAIL 108 128 1 2 FAIL 109 128 1 2 FAIL 110 128 1 2 FAIL 111 128 1 2 FAIL 112 128 1 2 FAIL 113 128 1 2 FAIL 114 128 1 2 FAIL 115 128 1 2 FAIL 116 128 1 2 FAIL 117 128 1 2 FAIL 118 128 1 2 FAIL 119 128 1 2 FAIL 120 128 1 2 FAIL 121 128 1 2 PASS 122 128 2 2 PASS 123 128 2 2 PASS 124 128 2 2 PASS 125 128 2 2 PASS 126 128 2 2 PASS 127 128 2 2 PASS 128 128 2 2 FAIL 129 192 2 3 FAIL 130 192 2 3 FAIL 131 192 2 3 FAIL 132 192 2 3 FAIL 133 192 2 3 FAIL 134 192 2 3 FAIL 135 192 2 3 FAIL 136 192 2 3 FAIL 137 192 2 3 FAIL 138 192 2 3 FAIL 139 192 2 3 FAIL 140 192 2 3 FAIL 141 192 2 3 FAIL 142 192 2 3 FAIL 143 192 2 3 FAIL 144 192 2 3 FAIL 145 192 2 3 FAIL 146 192 2 3 FAIL 147 192 2 3 FAIL 148 192 2 3 FAIL 149 192 2 3 FAIL 150 192 2 3 FAIL 151 192 2 3 FAIL 152 192 2 3 FAIL 153 192 2 3 FAIL 154 192 2 3 FAIL 155 192 2 3 FAIL 156 192 2 3 FAIL 157 192 2 3 FAIL 158 192 2 3 FAIL 159 192 2 3 FAIL 160 192 2 3 FAIL 161 192 2 3 FAIL 162 192 2 3 FAIL 163 192 2 3 FAIL 164 192 2 3 FAIL 165 192 2 3 FAIL 166 192 2 3 FAIL 167 192 2 3 FAIL 168 192 2 3 FAIL 169 192 2 3 FAIL 170 192 2 3 FAIL 171 192 2 3 FAIL 172 192 2 3 FAIL 173 192 2 3 FAIL 174 192 2 3 FAIL 175 192 2 3 FAIL 176 192 2 3 FAIL 177 192 2 3 FAIL 178 192 2 3 FAIL 179 192 2 3 FAIL 180 192 2 3 FAIL 181 192 2 3 FAIL 182 192 2 3 FAIL 183 192 2 3 FAIL 184 192 2 3 FAIL 185 192 2 3 PASS 186 192 3 3 PASS 187 192 3 3 PASS 188 192 3 3 PASS 189 192 3 3 PASS 190 192 3 3 PASS 191 192 3 3 PASS 192 192 3 3 FAIL 193 256 3 4 FAIL 194 256 3 4 FAIL 195 256 3 4 FAIL 196 256 3 4 FAIL 197 256 3 4 FAIL 198 256 3 4 FAIL 199 256 3 4 FAIL 200 256 3 4 FAIL 201 256 3 4 FAIL 202 256 3 4 FAIL 203 256 3 4 FAIL 204 256 3 4 FAIL 205 256 3 4 FAIL 206 256 3 4 FAIL 207 256 3 4 FAIL 208 256 3 4 FAIL 209 256 3 4 FAIL 210 256 3 4 FAIL 211 256 3 4 FAIL 212 256 3 4 FAIL 213 256 3 4 FAIL 214 256 3 4 FAIL 215 256 3 4 FAIL 216 256 3 4 FAIL 217 256 3 4 FAIL 218 256 3 4 FAIL 219 256 3 4 FAIL 220 256 3 4 FAIL 221 256 3 4 FAIL 222 256 3 4 FAIL 223 256 3 4 FAIL 224 256 3 4 FAIL 225 256 3 4 FAIL 226 256 3 4 FAIL 227 256 3 4 FAIL 228 256 3 4 FAIL 229 256 3 4 FAIL 230 256 3 4 FAIL 231 256 3 4 FAIL 232 256 3 4 FAIL 233 256 3 4 FAIL 234 256 3 4 FAIL 235 256 3 4 FAIL 236 256 3 4 FAIL 237 256 3 4 FAIL 238 256 3 4 FAIL 239 256 3 4 FAIL 240 256 3 4 FAIL 241 256 3 4 FAIL 242 256 3 4 FAIL 243 256 3 4 FAIL 244 256 3 4 FAIL 245 256 3 4 FAIL 246 256 3 4 FAIL 247 256 3 4 FAIL 248 256 3 4 FAIL 249 256 3 4 PASS 250 256 4 4 PASS 251 256 4 4 PASS 252 256 4 4 PASS 253 256 4 4 PASS 254 256 4 4 PASS 255 256 4 4 PASS 256 256 4 4 FAIL 257 320 4 5 FAIL 258 320 4 5 FAIL 259 320 4 5 FAIL 260 320 4 5 FAIL 261 320 4 5 FAIL 262 320 4 5 FAIL 263 320 4 5 FAIL 264 320 4 5 FAIL 265 320 4 5 FAIL 266 320 4 5 FAIL 267 320 4 5 FAIL 268 320 4 5 FAIL 269 320 4 5 FAIL 270 320 4 5 FAIL 271 320 4 5 FAIL 272 320 4 5 FAIL 273 320 4 5 FAIL 274 320 4 5 FAIL 275 320 4 5 FAIL 276 320 4 5 FAIL 277 320 4 5 FAIL 278 320 4 5 FAIL 279 320 4 5 FAIL 280 320 4 5 FAIL 281 320 4 5 FAIL 282 320 4 5 FAIL 283 320 4 5 FAIL 284 320 4 5 FAIL 285 320 4 5 FAIL 286 320 4 5 FAIL 287 320 4 5 FAIL 288 320 4 5 FAIL 289 320 4 5 FAIL 290 320 4 5 FAIL 291 320 4 5 FAIL 292 320 4 5 FAIL 293 320 4 5 FAIL 294 320 4 5 FAIL 295 320 4 5 FAIL 296 320 4 5 FAIL 297 320 4 5 FAIL 298 320 4 5 FAIL 299 320 4 5 FAIL 300 320 4 5 FAIL 301 320 4 5 FAIL 302 320 4 5 FAIL 303 320 4 5 FAIL 304 320 4 5 FAIL 305 320 4 5 FAIL 306 320 4 5 FAIL 307 320 4 5 FAIL 308 320 4 5 FAIL 309 320 4 5 FAIL 310 320 4 5 FAIL 311 320 4 5 FAIL 312 320 4 5 FAIL 313 320 4 5 PASS 314 320 5 5 PASS 315 320 5 5 PASS 316 320 5 5 PASS 317 320 5 5 PASS 318 320 5 5 PASS 319 320 5 5 PASS 320 320 5 5 FAIL 321 384 5 6 FAIL 322 384 5 6 FAIL 323 384 5 6 FAIL 324 384 5 6 FAIL 325 384 5 6 FAIL 326 384 5 6 FAIL 327 384 5 6 FAIL 328 384 5 6 FAIL 329 384 5 6 FAIL 330 384 5 6 FAIL 331 384 5 6 FAIL 332 384 5 6 FAIL 333 384 5 6 FAIL 334 384 5 6 FAIL 335 384 5 6 FAIL 336 384 5 6 FAIL 337 384 5 6 FAIL 338 384 5 6 FAIL 339 384 5 6 FAIL 340 384 5 6 FAIL 341 384 5 6 FAIL 342 384 5 6 FAIL 343 384 5 6 FAIL 344 384 5 6 FAIL 345 384 5 6 FAIL 346 384 5 6 FAIL 347 384 5 6 FAIL 348 384 5 6 FAIL 349 384 5 6 FAIL 350 384 5 6 FAIL 351 384 5 6 FAIL 352 384 5 6 FAIL 353 384 5 6 FAIL 354 384 5 6 FAIL 355 384 5 6 FAIL 356 384 5 6 FAIL 357 384 5 6 FAIL 358 384 5 6 FAIL 359 384 5 6 FAIL 360 384 5 6 FAIL 361 384 5 6 FAIL 362 384 5 6 FAIL 363 384 5 6 FAIL 364 384 5 6 FAIL 365 384 5 6 FAIL 366 384 5 6 FAIL 367 384 5 6 FAIL 368 384 5 6 FAIL 369 384 5 6 FAIL 370 384 5 6 FAIL 371 384 5 6 FAIL 372 384 5 6 FAIL 373 384 5 6 FAIL 374 384 5 6 FAIL 375 384 5 6 FAIL 376 384 5 6 FAIL 377 384 5 6 PASS 378 384 6 6 PASS 379 384 6 6 PASS 380 384 6 6 PASS 381 384 6 6 PASS 382 384 6 6 PASS 383 384 6 6 PASS 384 384 6 6 FAIL 385 448 6 7 FAIL 386 448 6 7 FAIL 387 448 6 7 FAIL 388 448 6 7 FAIL 389 448 6 7 FAIL 390 448 6 7 FAIL 391 448 6 7 FAIL 392 448 6 7 FAIL 393 448 6 7 FAIL 394 448 6 7 FAIL 395 448 6 7 FAIL 396 448 6 7 FAIL 397 448 6 7 FAIL 398 448 6 7 FAIL 399 448 6 7 FAIL 400 448 6 7 FAIL 401 448 6 7 FAIL 402 448 6 7 FAIL 403 448 6 7 FAIL 404 448 6 7 FAIL 405 448 6 7 FAIL 406 448 6 7 FAIL 407 448 6 7 FAIL 408 448 6 7 FAIL 409 448 6 7 FAIL 410 448 6 7 FAIL 411 448 6 7 FAIL 412 448 6 7 FAIL 413 448 6 7 FAIL 414 448 6 7 FAIL 415 448 6 7 FAIL 416 448 6 7 FAIL 417 448 6 7 FAIL 418 448 6 7 FAIL 419 448 6 7 FAIL 420 448 6 7 FAIL 421 448 6 7 FAIL 422 448 6 7 FAIL 423 448 6 7 FAIL 424 448 6 7 FAIL 425 448 6 7 FAIL 426 448 6 7 FAIL 427 448 6 7 FAIL 428 448 6 7 FAIL 429 448 6 7 FAIL 430 448 6 7 FAIL 431 448 6 7 FAIL 432 448 6 7 FAIL 433 448 6 7 FAIL 434 448 6 7 FAIL 435 448 6 7 FAIL 436 448 6 7 FAIL 437 448 6 7 FAIL 438 448 6 7 FAIL 439 448 6 7 FAIL 440 448 6 7 FAIL 441 448 6 7 PASS 442 448 7 7 PASS 443 448 7 7 PASS 444 448 7 7 PASS 445 448 7 7 PASS 446 448 7 7 PASS 447 448 7 7 PASS 448 448 7 7 FAIL 449 512 7 8 FAIL 450 512 7 8 FAIL 451 512 7 8 FAIL 452 512 7 8 FAIL 453 512 7 8 FAIL 454 512 7 8 FAIL 455 512 7 8 FAIL 456 512 7 8 FAIL 457 512 7 8 FAIL 458 512 7 8 FAIL 459 512 7 8 FAIL 460 512 7 8 FAIL 461 512 7 8 FAIL 462 512 7 8 FAIL 463 512 7 8 FAIL 464 512 7 8 FAIL 465 512 7 8 FAIL 466 512 7 8 FAIL 467 512 7 8 FAIL 468 512 7 8 FAIL 469 512 7 8 FAIL 470 512 7 8 FAIL 471 512 7 8 FAIL 472 512 7 8 FAIL 473 512 7 8 FAIL 474 512 7 8 FAIL 475 512 7 8 FAIL 476 512 7 8 FAIL 477 512 7 8 FAIL 478 512 7 8 FAIL 479 512 7 8 FAIL 480 512 7 8 FAIL 481 512 7 8 FAIL 482 512 7 8 FAIL 483 512 7 8 FAIL 484 512 7 8 FAIL 485 512 7 8 FAIL 486 512 7 8 FAIL 487 512 7 8 FAIL 488 512 7 8 FAIL 489 512 7 8 FAIL 490 512 7 8 FAIL 491 512 7 8 FAIL 492 512 7 8 FAIL 493 512 7 8 FAIL 494 512 7 8 FAIL 495 512 7 8 FAIL 496 512 7 8 FAIL 497 512 7 8 FAIL 498 512 7 8 FAIL 499 512 7 8 FAIL 500 512 7 8 FAIL 501 512 7 8 FAIL 502 512 7 8 FAIL 503 512 7 8 FAIL 504 512 7 8 FAIL 505 512 7 8 PASS 506 512 8 8 PASS 507 512 8 8 PASS 508 512 8 8 PASS 509 512 8 8 PASS 510 512 8 8 PASS 511 512 8 8 PASS 512 512 8 8 Luckily for 512 signals (513) the math works out. For 96 signals it does not. (96 - 1 + 7) = 102 102 / 64 = 1 That's only a signal word, that only supports 64 signals, not 96. Why doesn't the static assert trigger? Because you a < the size of the pthread_unwind_buf, and too small actually, writing into other parts of the buffer! I would expect us to use something like this: --- ... but with the size you need and explain *why* it's 96. You need a comment like this: /* The layout looks like this: - N words for this. - N words for that. - N words for shadow stack. Total 96 signals. */ Align the number of signals up to multiple of signals you can store in a hardware word, and then figure out how many words that takes up. Please review my math. >>> /* 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 >>> + <http://www.gnu.org/licenses/>. */ >>> + >>> +#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 >>> + <http://www.gnu.org/licenses/>. */ >>> + >>> +#define __libc_longjmp __redirect___libc_longjmp >>> +#include <setjmp/longjmp.c> >>> +#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 >>> + <http://www.gnu.org/licenses/>. */ >>> + >>> +/* <nptl/descr.h> 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? > > sysdeps/unix/sysv/linux/x86/setjmpP.h has > > typedef struct > { > unsigned long int __val[_JUMP_BUF_SIGSET_NWORDS]; > } __jmp_buf_sigset_t; > > typedef union > { > __sigset_t __saved_mask_compat; > struct > { > __jmp_buf_sigset_t __saved_mask; > /* Used for shadow stack pointer. */ > unsigned long int __shadow_stack_pointer; > } __saved; > } __jmpbuf_arch_t; > > __shadow_stack_pointer is placed after __saved_mask, aka mask_was_saved. OK, I'll re-review. >>> + 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 <pthreadP.h> >>> +#include <jmp_buf-ssp.h> >>> + >>> +#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 <shlib-compat.h> >>> + >>> +/* 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. > > > I look forward to a v2 with correct rounding and a new comment.
diff --git a/sysdeps/unix/sysv/linux/x86/setjmpP.h b/sysdeps/unix/sysv/linux/x86/setjmpP.h index c0ed767a0d..6e1e8f901c 100644 --- a/sysdeps/unix/sysv/linux/x86/setjmpP.h +++ b/sysdeps/unix/sysv/linux/x86/setjmpP.h @@ -20,13 +20,15 @@ #define _SETJMPP_H 1 #include <bits/types/__sigset_t.h> +#include <libc-pointer-arith.h> -/* 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 +/* As of kernel 4.14, x86 _NSIG is 64. + Define it to 512 to leave some rooms for future use. */ +#define _JUMP_BUF_SIGSET_NSIG 512 /* Number of longs to hold all signals. */ #define _JUMP_BUF_SIGSET_NWORDS \ - ((_JUMP_BUF_SIGSET_NSIG - 1 + 7) / (8 * sizeof (unsigned long int))) + (ALIGN_UP(_JUMP_BUF_SIGSET_NSIG, (8 * sizeof (unsigned long int))) \ + / (8 * sizeof (unsigned long int))) typedef struct {