From: "H.J. Lu" <hjl.tools@gmail.com>
To: "Carlos O'Donell" <carlos@redhat.com>
Cc: GNU C Library <libc-alpha@sourceware.org>
Subject: Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf
Date: Thu, 8 Feb 2018 05:27:43 -0800 [thread overview]
Message-ID: <CAMe9rOqewLkL97AjcRJpHxHjCEJW88=4TkrkGSv0CqLmsxfYgw@mail.gmail.com> (raw)
In-Reply-To: <4abf9786-1879-f16c-5a01-3261cd718d63@redhat.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.
next prev parent reply other threads:[~2018-02-08 13:25 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-01 20:57 [PATCH 0/2] nptl: Update struct pthread_unwind_buf H.J. Lu
2018-02-01 20:57 ` [PATCH 1/2] Revert "Revert Intel CET changes to __jmp_buf_tag (Bug 22743)" H.J. Lu
2018-02-01 20:57 ` [PATCH 2/2] nptl: Update struct pthread_unwind_buf [BZ #22743] H.J. Lu
2018-02-08 9:25 ` [PATCH 0/2] nptl: Update struct pthread_unwind_buf Carlos O'Donell
2018-02-08 11:55 ` Florian Weimer
2018-02-09 6:29 ` Carlos O'Donell
2018-02-09 10:48 ` Florian Weimer
2018-02-09 11:13 ` H.J. Lu
2018-02-09 12:11 ` Florian Weimer
2018-02-09 12:34 ` H.J. Lu
2018-02-09 14:13 ` H.J. Lu
2018-02-09 14:33 ` Florian Weimer
2018-02-09 15:24 ` H.J. Lu
2018-02-24 5:46 ` Carlos O'Donell
2018-02-24 15:19 ` H.J. Lu
2018-02-24 15:46 ` Florian Weimer
2018-02-25 2:04 ` H.J. Lu
2018-02-25 9:26 ` Florian Weimer
2018-02-25 11:37 ` H.J. Lu
2018-02-25 11:59 ` Florian Weimer
2018-02-25 12:53 ` H.J. Lu
2018-02-25 12:55 ` H.J. Lu
2018-02-25 12:58 ` Florian Weimer
2018-02-25 13:23 ` H.J. Lu
2018-02-25 13:31 ` Florian Weimer
2018-02-25 13:36 ` H.J. Lu
2018-02-25 13:49 ` H.J. Lu
2018-02-25 13:49 ` Florian Weimer
2018-02-25 14:00 ` H.J. Lu
2018-02-25 14:13 ` Florian Weimer
2018-02-26 3:55 ` H.J. Lu
2018-02-28 23:23 ` Carlos O'Donell
2018-03-07 11:56 ` H.J. Lu
2018-03-07 17:34 ` Carlos O'Donell
2018-03-07 19:47 ` H.J. Lu
2018-03-07 20:14 ` H.J. Lu
2018-03-07 22:06 ` H.J. Lu
2018-03-08 12:24 ` Tsimbalist, Igor V
2018-03-08 12:48 ` H.J. Lu
2018-03-09 0:47 ` Carlos O'Donell
2018-03-09 5:23 ` H.J. Lu
2018-03-15 4:20 ` Carlos O'Donell
2018-02-24 6:41 ` Carlos O'Donell
2018-02-08 13:27 ` H.J. Lu [this message]
2018-02-09 6:40 ` Carlos O'Donell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/libc/involved.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMe9rOqewLkL97AjcRJpHxHjCEJW88=4TkrkGSv0CqLmsxfYgw@mail.gmail.com' \
--to=hjl.tools@gmail.com \
--cc=carlos@redhat.com \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).