This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] x86: Use pad in pthread_unwind_buf to preserve shadow stack register
- From: Carlos O'Donell <carlos at redhat dot com>
- To: "H.J. Lu" <hjl dot tools at gmail dot com>
- Cc: Florian Weimer <fw at deneb dot enyo dot de>, Joseph Myers <joseph at codesourcery dot com>, "Tsimbalist, Igor V" <igor dot v dot tsimbalist at intel dot com>, GNU C Library <libc-alpha at sourceware dot org>
- Date: Thu, 12 Apr 2018 16:36:21 -0500
- Subject: Re: [PATCH] x86: Use pad in pthread_unwind_buf to preserve shadow stack register
- References: <CAMe9rOpj6nYdP33emmepZ+KWFG2JDVNsHn1bve3oo5WQyFmduA@mail.gmail.com> <61a5b452-e59e-dfef-4530-a94a60480961@redhat.com> <CAMe9rOr68P6Z=rX=xeFG1PbQE1fZ14iN3NmZEpChjP6QmUfS8g@mail.gmail.com>
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:
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
{
---
... 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.
--
Cheers,
Carlos.