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

[Carlos O'Donell <carlos at redhat dot com>]

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.

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
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

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.

-- 
Cheers,
Carlos.