PING^2 [PATCH/v3] x86: Use pad in pthread_unwind_buf to preserve shadow stack register

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Thu, Apr 26, 2018 at 2:31 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Sat, Apr 21, 2018 at 11:37 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Fri, Apr 20, 2018 at 10:28:24PM -0500, Carlos O'Donell wrote:
>>> Lets call this v2. Subject adjusted. Please keep incrementing the version
>>> number on your patches to make the review easier by myself and others.
>>>
>>
>> Done.
>>
>>> > +
>>> > +  /* 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
>>>
>>> s/be//g
>>
>>  Done
>>
>>> > +/* Number of bits per long.  */
>>> > +#define _JUMP_BUF_SIGSET_BITS_PER_WORD (8 * sizeof (unsigned long int))
>>> > +/* The biggest signal number.  As of kernel 4.14, x86 _NSIG is 64.
>>> > +   Define it to 96 to leave some rooms for future use.  */
>>>
>>> Why 96?
>>>
>>> Sure on x86_64 the cancel jump buf has 4 x void*'s and each is 64-bits,
>>> so you have 128 signals worth of space and then the shadow stack pointer.
>>>
>>> Does this work on x32?
>>>
>>> On x32 you have only 4 void*'s in the private pad.
>>>
>>> Your presently structured sigset_t looks like this:
>>>
>>> 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;
>>>
>>> Which means you have a sigset_t *before* the __shadow_stack_pointer.
>>>
>>> On x32 to save a 64-bit shadow stack pointer, you'll only have 2 x 32-bit
>>> words left? That's only space for 64 signals?
>>>
>>> Are you counting on one additional 32-bit word of padding between the
>>> __mask_was_saved and the rest of the long arguments?
>>>
>>> If so, then this still needs spelling out in an a comment why 96 signals
>>> works on both i686, x32, and x86_64.
>>>
>>> Also it should be explained if 96 is the *maximum* we can do with the current
>>> layout, or if more is possible. In which case picking 96 is *not* arbitrary.
>>
>>    The pad array in struct pthread_unwind_buf is used by setjmp to save
>>    shadow stack register.  The usable space in __saved_mask for sigset
>>    and shadow stack pointer:
>>    1. i386: The 4x4 byte pad array which can be used for 4 byte shadow
>>    stack pointer and maximum 12 byte sigset.
>>    2. x32: 4 byte padding + the 4x4 byte pad array which can be used
>>    for 8 byte shadow stack pointer and maximum 12 byte sigset.
>>    3. x86-64: The 4x8 byte pad array which can be used for 8 byte
>>    shadow stack pointer and maximum 24 byte sigset.
>>
>> Please see comments in sysdeps/unix/sysv/linux/x86/setjmpP.h for
>> details.
>>
>>> Suggest:
>>> ~~~
>>> The pad array in struct pthread_unwind_buf is used by setjmp to save
>>> the shadow stack register.  Assert that the size of struct pthread_unwind_buf
>>> is no less than the offset of the shadow stack pointer plus the shadow stack
>>> pointer size.
>>>
>>> NB: We use setjmp in thread cancellation and this saves the shadow stack
>>> register, but __libc_unwind_longjmp doesn't restore the shadow stack register
>>> since cancellation never returns after longjmp.
>>> ~~~
>>>
>>
>> Done.
>>
>>>
>>> This assertion is too loose.
>>>
>>> The assertion you need is that the shadow stack pointer itself falls within
>>> the range of the private padding. This would have caught the previous bug
>>> with the rounded up size.
>>>
>>> Please adjust the assertion to be as tight as possible or add new assertions.
>>>
>>> Completely untested, but just to show what I'm thinking:
>>>
>>> _Static_assert ((offsetof (struct pthread_unwind_buf, cancel_jump_buf[0].mask_was_saved)
>>>                + sizeof (int) < SHADOW_STACK_POINTER_OFFSET
>>>                && (offsetof (struct pthread_unwind_buf, priv.pad[4])
>>>                   > (SHADOW_STACK_POINTER_OFFSET + SHADOW_STACK_POINTER_SIZE),
>>>               "Shadow stack pointer is not within private storage of pthread_unwind_buf.");
>>>
>>
>> I used a slighly different assert.
>>
>>>
>>> Looking forward to a v3.
>>>
>>
>> Here it is.
> .14.3
>
> PING:
>
> https://sourceware.org/ml/libc-alpha/2018-04/msg00467.html
>
> BTW, after enabling shadow stack in context functions, I am ready
> to submit all CET patches.
>

PING.


-- 
H.J.