Patchwork x86: Use pad in pthread_unwind_buf to preserve shadow stack register

login
register
mail settings
Submitter Carlos O'Donell
Date April 12, 2018, 9:36 p.m.
Message ID <2dc14d06-a06c-1aeb-dca3-f97f4ee68415@redhat.com>
Download mbox | patch
Permalink /patch/26705/
State New
Headers show

Comments

Carlos O'Donell - April 12, 2018, 9:36 p.m.
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.

Patch

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
   {