PING [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 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.


-- 
H.J.