Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf

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

On Thu, Feb 8, 2018 at 1:25 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 02/01/2018 12:57 PM, H.J. Lu wrote:
>> struct pthread_unwind_buf is updated to save and restore shadow stack
>> register with backward binary compatibility.
>>
>> H.J. Lu (2):
>>   Revert "Revert Intel CET changes to __jmp_buf_tag (Bug 22743)"
>>   nptl: Update struct pthread_unwind_buf [BZ #22743]
>>
>
> High Level:
>
> To enable Intel CET we need to make sure that all places that change
> execution context abruptly also adjust the shadow stack such that the
> subsequent return matches what is presently on the stack. The current
> setjmp/longjmp are just such functions that change execution context
> abruptly, and to use longjmp to return to a setjmp location we must
> also restore the shadow stack on the return jump.
>
> The cancellation push/pop sequences also use a jump buffer, and at a
> high-level your solution is to expand those buffers with enough space
> (an ABI change) to store the shadow stack pointer. The ABI transition
> is managed using the CET note markup, and safety is ensured in that way
> as a all-or-nothing ABI change.
>
> Note that it depends heavily CET markup rolling out smoothly with glibc
> 2.28. If any distro enables CET markup without glibc 2.28 then it might
> be possible to have objects that use the old-ABI jump buffers in a CET
> enabled application and crash. Similarly if users build and use their
> own runtime.
>
> Why not version __pthread_register_cancel instead? The versioned symbol
> would seem to me a more robust indicator of the larger jump buffer than
> the requirement on coordinating markup + glibc 2.28 in all distros and
> for all users?
>
> I would like to see an argument made for CET markup against versioning
> __pthread_register_cleanup.
>
> Design:
>
> Commit f33632ccd1dec3217583fcfdd965afb62954203c finds space in the
> existing sigset_t by reducing the number of possible signals down to
> 513, the kernel is only using 64, and the remaining space is used to
> store the 64-bit shadow stack pointer.
>
> However, this isn't the only place that jump buffers like those used
> in setjmp/longjmp are used. Internally in pthread_cleanup_push and
> pthread_clenaup_pop the cancellation is handled by a special
> structure that is almost identical to the jump buffers used by
> setjmp/longjmp. However, because they are not identical, and were
> designed to avoid needing to save/restore the process signal mask
> during cancellation (as an optimization), these buffers also do
> not have enough space to store the shadow stack pointer.
>
> Therefore there needs to be an expansion in the size of the jump
> buffer used for cancellation to accomodate the shadow stack pointer.
>
> Your patches present the most logical next step here, which is to
> make the cancellation jump buffer the same size as the normal jump
> buffer used for setjmp/longjmp.
>
> Changing the cancellation jump buffer is an ABI change though since
> the jump buffer is allocated in the application stack via the the
> pthread_cleanup_push macro. Some way must be found to determine which
> size of jump buffer the caller is using to know if the newer larger
> version can be used.
>
> It is not sufficient to version pthread_create to make this distinction
> because an application may have been compiled with a new glibc, get
> the new version of pthread_create, and then call a DSO which used the
> old pthread_cleanup_push, and so expecte the old-ABI jump buffer.
>
> It is not sufficient to use priv.pad[3] because on ABIs with 32-bit
> pointers like x32 there is insufficient space to store the 64-bit
> shadow stack pointer.
>
> It *is* sufficient to version __pthread_register_cleanup in theory
> because on a caller-by-caller basis this would indicate if the caller
> had been compiled with the old or new-ABI jump buffer and read
> accordingly.
>
> Your patches take another approach, which is to use the binutils CET
> markup to indicate an all-or-nothing ABI transition to the new larger
> sized jump buffer. If CET is enabled, then the application would have
> been compiled with a CET enabled gcc, CET enabled glibc, and CET
> enabled binutils, and the result is a CET enabled application which
> is then assumed to have the newer larger sized cancellation jump buffer.
> Your solution avoids adding another symbol version and uses the existing
> CET markup as the deciding factor.
>
> Why might we reject versioning __pthread_register_cleanup? It would seem
> that since glibc internally controls the pthread_unwind_buf it will never
> be passed by pointer to another TU to be used in ways we don't control
> (the problem s390x faced when trying to increase the size of jmpbuf
> for setjmp/longjmp) e.g. loaded by a newer version of __pthread_register_cleanup
> that expects a different sized object. The internals and their use
> as expected from pthread_cleanup_push/pop are all in the same translation
> unit.
>
> Again, I would like to see an argument made for CET markup against versioning
> __pthread_register_cleanup.

Symbol versioning works well when only opaque data pointers are used.
A pointer of struct pthread_unwind_buf is passed to libpthread from user
programs.  Within libpthread, we need to know the exact layout of struct
pthread_unwind_buf.  It is next to impossible to use symbol versioning here.

> Details:
>
> The name NEED_SAVED_MASK_IN_CANCEL_JMP_BUF while true is slightly
> misleading IMO. While it is true that you need the saved mask there, the
> actual logical goal is to make the structure match in layout with the
> non-cancel jmp_buf. I would rename this to NEED_SETJMP_LAYOUT or something

Will do.

> like that to indicate the intent is to make the structure match the layout
> used by setjmp jump buffers.
>
> Both UNWIND_BUF_PRIV and THREAD_COPY_ADDITONAL_INFO are exmaples of typo-prone
> macro APIs. We should work hard to define defaults and have unconditional
> users of these macros such that -Wundef can warn us if we make mistakes
> e.g. https://sourceware.org/glibc/wiki/Wundef

Will do.

> Notes:
> - This current patch set passes the ABI tests I ran last time so it is good
>   to note that your patches do not appear to introduce any other ABI regressions.
>

Thanks.


-- 
H.J.