Revert Intel CET changes to __jmp_buf_tag (Bug 22743)
Commit Message
On Thu, Jan 25, 2018 at 1:47 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 01/25/2018 06:33 AM, H.J. Lu wrote:
>
>> Please don't revert my patch. Please try this patch:
>>
>>
>> https://sourceware.org/git/?p=glibc.git;a=commit;h=4b7fc470a6740808b41502d7431f91805e272d26
>>
>> instead. I will clean it up and submit it tomorrow.
>
>
> I don't see how adding a symbol version to pthread_create helps to solve the
> general case. Callers of pthread_register_cancel and pthread_create are
> often compiled at different times. Not everyone does a mass rebuild each
> time they switch to a new glibc version.
True. We just need to use the older struct pthread_unwind_buf when shadow
stack is disabled.
> I still think you are over-engineering this. The pad array has still an
> unused member (the last one). Just change sigsetjmp to store the shadow
> pointer in that location, then the old and new setjmp will work with the
> current stack layout. As far as I can tell, there are only 64 signals, so
> you don't even have to change the location of the signal mask.
No, it doesn't work. struct pthread_unwind_buf is placed on caller's stack
and its address is passed from applications to libpthread. If the size of
caller's struct pthread_unwind_buf is smaller than what libpthread expects,
libpthread will override caller's stack.
> Furthermore, nothing in the toolchain prevents people from compiling
That is not true. From CET spec:
https://github.com/hjl-tools/x86-psABI/wiki/x86-64-psABI-cet.pdf
On a SHSTK capable processor, the following steps should be taken:
1. When loading an executable without interpreter, enable and lock SHSTK if
GNU_PROPERTY_X86_FEATURE_1_SHSTK is set on the executable.
2. When loading an executable with an interpreter, enable SHSTK
if GNU_PROPERTY_X86_FEATURE_1_SHSTK is set on
the interpreter. The interpreter should disable SHSTK if
GNU_PROPERTY_X86_FEATURE_1_SHSTK isn’t set or any shared
objects loaded via the DT_NEEDED tag, otherwise lock SHSTK.
3. After SHSTK is enabled, it is an error to load a shared object without
GNU_PROPERTY_X86_FEATURE_1_SHSTK.
> CET-marked binaries with older glibc headers, so you can't use CET markup to
> determine the size of the stack allocation anyway.
>
Yes, we can.
We enable shadow stack at run-time only if program and all used shared
objects, including dlopened ones, are shadow stack enabled, which means
that they must be compiled with GCC 8 or above and glibc 2.27 or above.
Since we need to save and restore shadow stack only if shadow stack is
enabled, we can safely assume that caller is compiled with smaller
struct pthread_unwind_buf on stack if shadow stack isn't enabled at
run-time. For callers with larger struct pthread_unwind_buf, but
shadow stack isn't enabled, we just have some unused space on caller's
stack.
Here is the patch to do that, Andrew, please work with Debian to verify
that fixes the crash.
BTW, my patch is on hjl/pr22743/master branch in glibc git repo.
Thanks.
Comments
On 01/25/2018 01:38 PM, H.J. Lu wrote:
>> I still think you are over-engineering this. The pad array has still an
>> unused member (the last one). Just change sigsetjmp to store the shadow
>> pointer in that location, then the old and new setjmp will work with the
>> current stack layout. As far as I can tell, there are only 64 signals, so
>> you don't even have to change the location of the signal mask.
> No, it doesn't work. struct pthread_unwind_buf is placed on caller's stack
> and its address is passed from applications to libpthread. If the size of
> caller's struct pthread_unwind_buf is smaller than what libpthread expects,
> libpthread will override caller's stack.
As far as I can see, ibuf->priv.pad[3] is currently unused. (sig)setjmp
could save the shadow stack pointer at the right offset in jmp_buf to
hit this place, then all these new conditionals wouldn't be necessary.
Of course it is still a hack, but your approach is not clearer IMHO.
The patch you posted is very different from the commit link you shared
earlier. I still need to review it in detail.
Thanks,
Florian
On Thu, Jan 25, 2018 at 4:50 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 01/25/2018 01:38 PM, H.J. Lu wrote:
>>>
>>> I still think you are over-engineering this. The pad array has still an
>>> unused member (the last one). Just change sigsetjmp to store the shadow
>>> pointer in that location, then the old and new setjmp will work with the
>>> current stack layout. As far as I can tell, there are only 64 signals,
>>> so
>>> you don't even have to change the location of the signal mask.
>>
>> No, it doesn't work. struct pthread_unwind_buf is placed on caller's
>> stack
>> and its address is passed from applications to libpthread. If the size
>> of
>> caller's struct pthread_unwind_buf is smaller than what libpthread
>> expects,
>> libpthread will override caller's stack.
>
>
> As far as I can see, ibuf->priv.pad[3] is currently unused. (sig)setjmp
> could save the shadow stack pointer at the right offset in jmp_buf to hit
> this place, then all these new conditionals wouldn't be necessary. Of course
> it is still a hack, but your approach is not clearer IMHO.
It is a mistake to use different jmpbuf sizes in libpthread and libc.
My patch is much more straightforward than what you suggested.
But I can live with it if everyone thinks it is the way to go.
> The patch you posted is very different from the commit link you shared
> earlier. I still need to review it in detail.
>
Yes, I reworked the legacy cleanup buffer detection. Now it simply checks
if shadow stack is enabled or not.
On Thu, Jan 25, 2018 at 8:00 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Jan 25, 2018 at 4:50 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 01/25/2018 01:38 PM, H.J. Lu wrote:
>>>>
>>>> I still think you are over-engineering this. The pad array has still an
>>>> unused member (the last one). Just change sigsetjmp to store the shadow
>>>> pointer in that location, then the old and new setjmp will work with the
>>>> current stack layout. As far as I can tell, there are only 64 signals,
>>>> so
>>>> you don't even have to change the location of the signal mask.
>>>
>>> No, it doesn't work. struct pthread_unwind_buf is placed on caller's
>>> stack
>>> and its address is passed from applications to libpthread. If the size
>>> of
>>> caller's struct pthread_unwind_buf is smaller than what libpthread
>>> expects,
>>> libpthread will override caller's stack.
>>
>>
>> As far as I can see, ibuf->priv.pad[3] is currently unused. (sig)setjmp
>> could save the shadow stack pointer at the right offset in jmp_buf to hit
>> this place, then all these new conditionals wouldn't be necessary. Of course
>> it is still a hack, but your approach is not clearer IMHO.
>
> It is a mistake to use different jmpbuf sizes in libpthread and libc.
> My patch is much more straightforward than what you suggested.
> But I can live with it if everyone thinks it is the way to go.
In my opinion, the fact that you two are having this argument
reinforces Carlos' position: the original patch should be reverted and
we should figure out what to do in 2.28 when we're not under time
pressure. HJ, do you have some concrete external reason why you must
have this new feature in 2.27? If so, please tell us what it is. To
me it doesn't seem urgent.
Also, this incident demonstrates that we need better testing of our
ABI backward compatibility guarantees. Automation for taking all of
the test binaries from glibc-2.x and running them against lib*.so from
glibc-2.y (y > x), or something like that. Probably Joseph is in the
best position to know how hard that would be.
zw
On Thu, Jan 25, 2018 at 6:55 AM, Zack Weinberg <zackw@panix.com> wrote:
> On Thu, Jan 25, 2018 at 8:00 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Thu, Jan 25, 2018 at 4:50 AM, Florian Weimer <fweimer@redhat.com> wrote:
>>> On 01/25/2018 01:38 PM, H.J. Lu wrote:
>>>>>
>>>>> I still think you are over-engineering this. The pad array has still an
>>>>> unused member (the last one). Just change sigsetjmp to store the shadow
>>>>> pointer in that location, then the old and new setjmp will work with the
>>>>> current stack layout. As far as I can tell, there are only 64 signals,
>>>>> so
>>>>> you don't even have to change the location of the signal mask.
>>>>
>>>> No, it doesn't work. struct pthread_unwind_buf is placed on caller's
>>>> stack
>>>> and its address is passed from applications to libpthread. If the size
>>>> of
>>>> caller's struct pthread_unwind_buf is smaller than what libpthread
>>>> expects,
>>>> libpthread will override caller's stack.
>>>
>>>
>>> As far as I can see, ibuf->priv.pad[3] is currently unused. (sig)setjmp
>>> could save the shadow stack pointer at the right offset in jmp_buf to hit
>>> this place, then all these new conditionals wouldn't be necessary. Of course
>>> it is still a hack, but your approach is not clearer IMHO.
>>
>> It is a mistake to use different jmpbuf sizes in libpthread and libc.
>> My patch is much more straightforward than what you suggested.
>> But I can live with it if everyone thinks it is the way to go.
>
> In my opinion, the fact that you two are having this argument
> reinforces Carlos' position: the original patch should be reverted and
> we should figure out what to do in 2.28 when we're not under time
> pressure. HJ, do you have some concrete external reason why you must
> have this new feature in 2.27? If so, please tell us what it is. To
> me it doesn't seem urgent.
My question is if we are going to fix it at all. If yes, why not 2.27.
Both approaches are opaque to users. They can't tell the difference.
On Thu, Jan 25, 2018 at 10:33 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Jan 25, 2018 at 6:55 AM, Zack Weinberg <zackw@panix.com> wrote:
>> In my opinion, the fact that you two are having this argument
>> reinforces Carlos' position: the original patch should be reverted and
>> we should figure out what to do in 2.28 when we're not under time
>> pressure. HJ, do you have some concrete external reason why you must
>> have this new feature in 2.27? If so, please tell us what it is. To
>> me it doesn't seem urgent.
>
> My question is if we are going to fix it at all. If yes, why not 2.27.
> Both approaches are opaque to users. They can't tell the difference.
My concerns are entirely based on timing: specifically, you seem to be
in a rush to squeak under the 2.27 deadline. Rushing leads to
mistakes.
This seems like the sort of thing that could reasonably be backported
to the release branch(es) ... *after* we have calmly, without rushing,
figured out the correct fix in mainline.
zw
On Thu, Jan 25, 2018 at 8:22 AM, Zack Weinberg <zackw@panix.com> wrote:
> On Thu, Jan 25, 2018 at 10:33 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Thu, Jan 25, 2018 at 6:55 AM, Zack Weinberg <zackw@panix.com> wrote:
>>> In my opinion, the fact that you two are having this argument
>>> reinforces Carlos' position: the original patch should be reverted and
>>> we should figure out what to do in 2.28 when we're not under time
>>> pressure. HJ, do you have some concrete external reason why you must
>>> have this new feature in 2.27? If so, please tell us what it is. To
>>> me it doesn't seem urgent.
>>
>> My question is if we are going to fix it at all. If yes, why not 2.27.
>> Both approaches are opaque to users. They can't tell the difference.
>
> My concerns are entirely based on timing: specifically, you seem to be
> in a rush to squeak under the 2.27 deadline. Rushing leads to
> mistakes.
The main issue for this one is testcase. Once a testcase is found, we
know how to avoid the issue.
> This seems like the sort of thing that could reasonably be backported
> to the release branch(es) ... *after* we have calmly, without rushing,
> figured out the correct fix in mainline.
>
I am fine with reverting my patch only on 2.27 branch, not on master.
On 01/25/2018 08:28 AM, H.J. Lu wrote:
> On Thu, Jan 25, 2018 at 8:22 AM, Zack Weinberg <zackw@panix.com> wrote:
>> On Thu, Jan 25, 2018 at 10:33 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Thu, Jan 25, 2018 at 6:55 AM, Zack Weinberg <zackw@panix.com> wrote:
>>>> In my opinion, the fact that you two are having this argument
>>>> reinforces Carlos' position: the original patch should be reverted and
>>>> we should figure out what to do in 2.28 when we're not under time
>>>> pressure. HJ, do you have some concrete external reason why you must
>>>> have this new feature in 2.27? If so, please tell us what it is. To
>>>> me it doesn't seem urgent.
>>>
>>> My question is if we are going to fix it at all. If yes, why not 2.27.
>>> Both approaches are opaque to users. They can't tell the difference.
>>
>> My concerns are entirely based on timing: specifically, you seem to be
>> in a rush to squeak under the 2.27 deadline. Rushing leads to
>> mistakes.
>
> The main issue for this one is testcase. Once a testcase is found, we
> know how to avoid the issue.
>
>> This seems like the sort of thing that could reasonably be backported
>> to the release branch(es) ... *after* we have calmly, without rushing,
>> figured out the correct fix in mainline.
>>
>
> I am fine with reverting my patch only on 2.27 branch, not on master.
This does not make sense. The revert on master would last for as long as
you have to come up with a patch that works and everyone accepts and has
consensus.
You checked these patches in without consensus, and instead of waiting
or pinging for review, you checked them in.
For x86_64 there is no machine maintainer, it requires community consensus,
the port is too important not to get serious community review.
They changes had negative ABI consequences, and now you have several
people interested in making sure future patches don't break ABI.
You have drawn attention to this work and now you have to reach consensus
on a solution for a primary architecture which is very important to all of
us in the downstream distributions. More time is required to make these
patches work.
I see no clear argument for why this needs to be in 2.27.
I will be reverting the patches in the next 8 hours.
On Thu, Jan 25, 2018 at 8:36 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 01/25/2018 08:28 AM, H.J. Lu wrote:
>> On Thu, Jan 25, 2018 at 8:22 AM, Zack Weinberg <zackw@panix.com> wrote:
>>> On Thu, Jan 25, 2018 at 10:33 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> On Thu, Jan 25, 2018 at 6:55 AM, Zack Weinberg <zackw@panix.com> wrote:
>>>>> In my opinion, the fact that you two are having this argument
>>>>> reinforces Carlos' position: the original patch should be reverted and
>>>>> we should figure out what to do in 2.28 when we're not under time
>>>>> pressure. HJ, do you have some concrete external reason why you must
>>>>> have this new feature in 2.27? If so, please tell us what it is. To
>>>>> me it doesn't seem urgent.
>>>>
>>>> My question is if we are going to fix it at all. If yes, why not 2.27.
>>>> Both approaches are opaque to users. They can't tell the difference.
>>>
>>> My concerns are entirely based on timing: specifically, you seem to be
>>> in a rush to squeak under the 2.27 deadline. Rushing leads to
>>> mistakes.
>>
>> The main issue for this one is testcase. Once a testcase is found, we
>> know how to avoid the issue.
>>
>>> This seems like the sort of thing that could reasonably be backported
>>> to the release branch(es) ... *after* we have calmly, without rushing,
>>> figured out the correct fix in mainline.
>>>
>>
>> I am fine with reverting my patch only on 2.27 branch, not on master.
>
> This does not make sense. The revert on master would last for as long as
> you have to come up with a patch that works and everyone accepts and has
> consensus.
We have 2 proposals, one with a patch and one without. How long
should it take to make a decision?
> You checked these patches in without consensus, and instead of waiting
> or pinging for review, you checked them in.
>
> For x86_64 there is no machine maintainer, it requires community consensus,
Do we need/want machine maintainers for x86-64 (i386)?
> the port is too important not to get serious community review.
>
> They changes had negative ABI consequences, and now you have several
> people interested in making sure future patches don't break ABI.
There are no arguments here.
> You have drawn attention to this work and now you have to reach consensus
> on a solution for a primary architecture which is very important to all of
> us in the downstream distributions. More time is required to make these
> patches work.
>
> I see no clear argument for why this needs to be in 2.27.
That is fine with me.
> I will be reverting the patches in the next 8 hours.
We need this on master so that we can work on CET support.
On 01/25/2018 08:40 AM, H.J. Lu wrote:
> On Thu, Jan 25, 2018 at 8:36 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 01/25/2018 08:28 AM, H.J. Lu wrote:
>>> On Thu, Jan 25, 2018 at 8:22 AM, Zack Weinberg <zackw@panix.com> wrote:
>>>> On Thu, Jan 25, 2018 at 10:33 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>> On Thu, Jan 25, 2018 at 6:55 AM, Zack Weinberg <zackw@panix.com> wrote:
>>>>>> In my opinion, the fact that you two are having this argument
>>>>>> reinforces Carlos' position: the original patch should be reverted and
>>>>>> we should figure out what to do in 2.28 when we're not under time
>>>>>> pressure. HJ, do you have some concrete external reason why you must
>>>>>> have this new feature in 2.27? If so, please tell us what it is. To
>>>>>> me it doesn't seem urgent.
>>>>>
>>>>> My question is if we are going to fix it at all. If yes, why not 2.27.
>>>>> Both approaches are opaque to users. They can't tell the difference.
>>>>
>>>> My concerns are entirely based on timing: specifically, you seem to be
>>>> in a rush to squeak under the 2.27 deadline. Rushing leads to
>>>> mistakes.
>>>
>>> The main issue for this one is testcase. Once a testcase is found, we
>>> know how to avoid the issue.
>>>
>>>> This seems like the sort of thing that could reasonably be backported
>>>> to the release branch(es) ... *after* we have calmly, without rushing,
>>>> figured out the correct fix in mainline.
>>>>
>>>
>>> I am fine with reverting my patch only on 2.27 branch, not on master.
>>
>> This does not make sense. The revert on master would last for as long as
>> you have to come up with a patch that works and everyone accepts and has
>> consensus.
>
> We have 2 proposals, one with a patch and one without. How long
> should it take to make a decision?
However long it takes.
Until then we revert the patches.
>> You checked these patches in without consensus, and instead of waiting
>> or pinging for review, you checked them in.
>>
>> For x86_64 there is no machine maintainer, it requires community consensus,
>
> Do we need/want machine maintainers for x86-64 (i386)?
No. We need people to follow consensus rules, ping patches, and ask for review.
Without review we are going to increase the risk of defects going in to the port.
The point of review is to lower defect rates and attain better solutions.
>> the port is too important not to get serious community review.
>>
>> They changes had negative ABI consequences, and now you have several
>> people interested in making sure future patches don't break ABI.
>
> There are no arguments here.
>
>> You have drawn attention to this work and now you have to reach consensus
>> on a solution for a primary architecture which is very important to all of
>> us in the downstream distributions. More time is required to make these
>> patches work.
>>
>> I see no clear argument for why this needs to be in 2.27.
>
> That is fine with me.
>
>> I will be reverting the patches in the next 8 hours.
>
> We need this on master so that we can work on CET support.
No. You do not *need* anything on master, you can work on your own branches
with all of your collaborators.
Master is for work that is complete, has consensus, and is ready to be
delivered into a public release for which we will promise ABI guarantees.
On 01/25/2018 05:22 PM, Zack Weinberg wrote:
> This seems like the sort of thing that could reasonably be backported
> to the release branch(es) ...*after* we have calmly, without rushing,
> figured out the correct fix in mainline.
H.J.'s approach requires that glibc 2.27 is fixed now because once
people build with CET, binaries will have the CET markup but still
follow the old ABI (assuming we make the ABI change subsequently).
(I don't understand why this doesn't already happen when glibc 2.26
headers are used to build programs with CET compiler flags.)
Thanks,
Florian
On Thu, Jan 25, 2018 at 8:47 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 01/25/2018 05:22 PM, Zack Weinberg wrote:
>>
>> This seems like the sort of thing that could reasonably be backported
>> to the release branch(es) ...*after* we have calmly, without rushing,
>> figured out the correct fix in mainline.
>
>
> H.J.'s approach requires that glibc 2.27 is fixed now because once people
> build with CET, binaries will have the CET markup but still follow the old
> ABI (assuming we make the ABI change subsequently).
No, they won't. We haven't checked in the critical patch to turn on
the CET markup yet. You can build glibc 2.27 with GCC 8. But you
won't get
[hjl@gnu-6 build-x86_64-linux]$ readelf -n csu/crt1.o
Displaying notes found in: .note.gnu.property
Owner Data size Description
GNU 0x00000010 NT_GNU_PROPERTY_TYPE_0
Properties: x86 feature: IBT, SHSTK
Displaying notes found in: .note.ABI-tag
Owner Data size Description
GNU 0x00000010 NT_GNU_ABI_TAG (ABI version tag)
OS: Linux, ABI: 3.2.0
[hjl@gnu-6 build-x86_64-linux]$
Use an used padding in pthread_unwind_buf to save and restore
shadow stack isn't a long term solution. What do we do if we need
to save and restore another register in jmp buf 5 years from now?
> (I don't understand why this doesn't already happen when glibc 2.26 headers
> are used to build programs with CET compiler flags.)
>
See above.
On Thu, Jan 25, 2018 at 8:46 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 01/25/2018 08:40 AM, H.J. Lu wrote:
>> On Thu, Jan 25, 2018 at 8:36 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>>> On 01/25/2018 08:28 AM, H.J. Lu wrote:
>>>> On Thu, Jan 25, 2018 at 8:22 AM, Zack Weinberg <zackw@panix.com> wrote:
>>>>> On Thu, Jan 25, 2018 at 10:33 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>> On Thu, Jan 25, 2018 at 6:55 AM, Zack Weinberg <zackw@panix.com> wrote:
>>>>>>> In my opinion, the fact that you two are having this argument
>>>>>>> reinforces Carlos' position: the original patch should be reverted and
>>>>>>> we should figure out what to do in 2.28 when we're not under time
>>>>>>> pressure. HJ, do you have some concrete external reason why you must
>>>>>>> have this new feature in 2.27? If so, please tell us what it is. To
>>>>>>> me it doesn't seem urgent.
>>>>>>
>>>>>> My question is if we are going to fix it at all. If yes, why not 2.27.
>>>>>> Both approaches are opaque to users. They can't tell the difference.
>>>>>
>>>>> My concerns are entirely based on timing: specifically, you seem to be
>>>>> in a rush to squeak under the 2.27 deadline. Rushing leads to
>>>>> mistakes.
>>>>
>>>> The main issue for this one is testcase. Once a testcase is found, we
>>>> know how to avoid the issue.
>>>>
>>>>> This seems like the sort of thing that could reasonably be backported
>>>>> to the release branch(es) ... *after* we have calmly, without rushing,
>>>>> figured out the correct fix in mainline.
>>>>>
>>>>
>>>> I am fine with reverting my patch only on 2.27 branch, not on master.
>>>
>>> This does not make sense. The revert on master would last for as long as
>>> you have to come up with a patch that works and everyone accepts and has
>>> consensus.
>>
>> We have 2 proposals, one with a patch and one without. How long
>> should it take to make a decision?
>
> However long it takes.
>
> Until then we revert the patches.
>
Sure. Please revert it now.
I will submit a patch to re-apply it + my fix after 2.27 branch
is taken.
On Thu, 25 Jan 2018, Zack Weinberg wrote:
> Also, this incident demonstrates that we need better testing of our
> ABI backward compatibility guarantees. Automation for taking all of
> the test binaries from glibc-2.x and running them against lib*.so from
> glibc-2.y (y > x), or something like that. Probably Joseph is in the
> best position to know how hard that would be.
I'd suggest, in the 2.x build tree,
make check rtld-prefix=SOMETHING run-program-env=SOMETHING
where the SOMETHINGs are appropriately defined to use the build or install
trees of 2.y. There are probably reasons that doesn't work for some or
all tests (apart from deliberate changes in glibc that required testsuite
changes but weren't considered to require new symbol versions) - but that
should be the basic idea of how to test binaries using a newer glibc
version at runtime.
(We can make changes to glibc to make that sort of thing easier if it
turns out such changes would help, but still want to be able to do it when
x, the old version, is a glibc version without those changes.)
On Thu, Jan 25, 2018 at 4:50 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 01/25/2018 01:38 PM, H.J. Lu wrote:
>>>
>>> I still think you are over-engineering this. The pad array has still an
>>> unused member (the last one). Just change sigsetjmp to store the shadow
>>> pointer in that location, then the old and new setjmp will work with the
>>> current stack layout. As far as I can tell, there are only 64 signals,
>>> so
>>> you don't even have to change the location of the signal mask.
>>
>> No, it doesn't work. struct pthread_unwind_buf is placed on caller's
>> stack
>> and its address is passed from applications to libpthread. If the size
>> of
>> caller's struct pthread_unwind_buf is smaller than what libpthread
>> expects,
>> libpthread will override caller's stack.
>
>
> As far as I can see, ibuf->priv.pad[3] is currently unused. (sig)setjmp
> could save the shadow stack pointer at the right offset in jmp_buf to hit
> this place, then all these new conditionals wouldn't be necessary. Of course
> it is still a hack, but your approach is not clearer IMHO.
FWIW, this approach doesn't work with x32 which is 64-bit process with
32-bit software pointer. Kernel may place shadow stack above 4GB.
We need to save and restore 64-bit shadow stack register for x32.
On 01/25/2018 09:01 AM, H.J. Lu wrote:
> On Thu, Jan 25, 2018 at 8:46 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 01/25/2018 08:40 AM, H.J. Lu wrote:
>>> On Thu, Jan 25, 2018 at 8:36 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>>>> On 01/25/2018 08:28 AM, H.J. Lu wrote:
>>>>> On Thu, Jan 25, 2018 at 8:22 AM, Zack Weinberg <zackw@panix.com> wrote:
>>>>>> On Thu, Jan 25, 2018 at 10:33 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>> On Thu, Jan 25, 2018 at 6:55 AM, Zack Weinberg <zackw@panix.com> wrote:
>>>>>>>> In my opinion, the fact that you two are having this argument
>>>>>>>> reinforces Carlos' position: the original patch should be reverted and
>>>>>>>> we should figure out what to do in 2.28 when we're not under time
>>>>>>>> pressure. HJ, do you have some concrete external reason why you must
>>>>>>>> have this new feature in 2.27? If so, please tell us what it is. To
>>>>>>>> me it doesn't seem urgent.
>>>>>>>
>>>>>>> My question is if we are going to fix it at all. If yes, why not 2.27.
>>>>>>> Both approaches are opaque to users. They can't tell the difference.
>>>>>>
>>>>>> My concerns are entirely based on timing: specifically, you seem to be
>>>>>> in a rush to squeak under the 2.27 deadline. Rushing leads to
>>>>>> mistakes.
>>>>>
>>>>> The main issue for this one is testcase. Once a testcase is found, we
>>>>> know how to avoid the issue.
>>>>>
>>>>>> This seems like the sort of thing that could reasonably be backported
>>>>>> to the release branch(es) ... *after* we have calmly, without rushing,
>>>>>> figured out the correct fix in mainline.
>>>>>>
>>>>>
>>>>> I am fine with reverting my patch only on 2.27 branch, not on master.
>>>>
>>>> This does not make sense. The revert on master would last for as long as
>>>> you have to come up with a patch that works and everyone accepts and has
>>>> consensus.
>>>
>>> We have 2 proposals, one with a patch and one without. How long
>>> should it take to make a decision?
>>
>> However long it takes.
>>
>> Until then we revert the patches.
>>
>
> Sure. Please revert it now.
>
> I will submit a patch to re-apply it + my fix after 2.27 branch
> is taken.
I have reverted the ABI breaking changes along with the matching
change which adds feature_1.
Thank you for working with everyone on this issue. I will spend the
day tomorrow reviewing these patches.
On Thu, Jan 25, 2018 at 11:45 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 01/25/2018 09:01 AM, H.J. Lu wrote:
>> On Thu, Jan 25, 2018 at 8:46 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>>> On 01/25/2018 08:40 AM, H.J. Lu wrote:
>>>> On Thu, Jan 25, 2018 at 8:36 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>>>>> On 01/25/2018 08:28 AM, H.J. Lu wrote:
>>>>>> On Thu, Jan 25, 2018 at 8:22 AM, Zack Weinberg <zackw@panix.com> wrote:
>>>>>>> On Thu, Jan 25, 2018 at 10:33 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>>> On Thu, Jan 25, 2018 at 6:55 AM, Zack Weinberg <zackw@panix.com> wrote:
>>>>>>>>> In my opinion, the fact that you two are having this argument
>>>>>>>>> reinforces Carlos' position: the original patch should be reverted and
>>>>>>>>> we should figure out what to do in 2.28 when we're not under time
>>>>>>>>> pressure. HJ, do you have some concrete external reason why you must
>>>>>>>>> have this new feature in 2.27? If so, please tell us what it is. To
>>>>>>>>> me it doesn't seem urgent.
>>>>>>>>
>>>>>>>> My question is if we are going to fix it at all. If yes, why not 2.27.
>>>>>>>> Both approaches are opaque to users. They can't tell the difference.
>>>>>>>
>>>>>>> My concerns are entirely based on timing: specifically, you seem to be
>>>>>>> in a rush to squeak under the 2.27 deadline. Rushing leads to
>>>>>>> mistakes.
>>>>>>
>>>>>> The main issue for this one is testcase. Once a testcase is found, we
>>>>>> know how to avoid the issue.
>>>>>>
>>>>>>> This seems like the sort of thing that could reasonably be backported
>>>>>>> to the release branch(es) ... *after* we have calmly, without rushing,
>>>>>>> figured out the correct fix in mainline.
>>>>>>>
>>>>>>
>>>>>> I am fine with reverting my patch only on 2.27 branch, not on master.
>>>>>
>>>>> This does not make sense. The revert on master would last for as long as
>>>>> you have to come up with a patch that works and everyone accepts and has
>>>>> consensus.
>>>>
>>>> We have 2 proposals, one with a patch and one without. How long
>>>> should it take to make a decision?
>>>
>>> However long it takes.
>>>
>>> Until then we revert the patches.
>>>
>>
>> Sure. Please revert it now.
>>
>> I will submit a patch to re-apply it + my fix after 2.27 branch
>> is taken.
>
> I have reverted the ABI breaking changes along with the matching
> change which adds feature_1.
>
> Thank you for working with everyone on this issue. I will spend the
> day tomorrow reviewing these patches.
>
FYI, my current patch is on hjl/pr22743/master branch at
https://github.com/hjl-tools/glibc/tree/hjl/pr22743/master
From 10bba58b7eece4eac0db07c100217b709efd4727 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Wed, 24 Jan 2018 15:27:49 -0800
Subject: [PATCH] nptl: Update struct pthread_unwind_buf [BZ #22743]
In glibc 2.27, the size of cancel_jmp_buf in struct pthread_unwind_buf
has been increased to match the size of __jmp_buf_tag on Linux/x86 in
order to save and restore shadow stack. Struct pthread_unwind_buf is
used in <pthread.h>, whose address is passed from applications to
libpthread. To access the private data in struct pthread_unwind_buf,
which is placed after cancel_jmp_buf, in libpthread, we must know which
struct pthread_unwind_buf, before glibc 27 and after glibc 2.27, is used
in caller. If the size of caller's struct pthread_unwind_buf is smaller
than what libpthread expects, libpthread will override caller's stack
since struct pthread_unwind_buf is placed on caller's stack.
We enable shadow stack at run-time only if program and all used shared
objects, including dlopened ones, are shadow stack enabled, which means
that they must be compiled with GCC 8 or above and glibc 2.27 or above.
Since we need to save and restore shadow stack only if shadow stack is
enabled, we can safely assume that caller is compiled with smaller
struct pthread_unwind_buf on stack if shadow stack isn't enabled at
run-time. For callers with larger struct pthread_unwind_buf, but
shadow stack isn't enabled, we just have some unused space on caller's
stack.
struct pthread_unwind_buf is changed to union of
1. struct cancel_jmp_buf[1], which contains the common fields of struct
updated_pthread_unwind_buf and struct compat_pthread_unwind_buf.
2. struct updated_pthread_unwind_buf, which is the updated layout of
the cleanup buffer.
3. struct compat_pthread_unwind_buf, which is the compatible layout of
the cleanup buffer.
A macro, UNWIND_BUF_PRIV, is added to get the pointer to the priv field.
By default, it uses the priv field of struct compat_pthread_unwind_buf.
If a target defines NEED_SAVED_MASK_IN_CANCEL_JMP_BUF, it must provide
its own version of UNEIND_BUF_PRIV to get the pointer to the priv field.
On Linux/x86, it uses the priv field of struct compat_pthread_unwind_buf
if shadow stack is disabled and struct updated_pthread_unwind_buf if
shadow stack is enabled.
[BZ #22743]
* csu/libc-start.c (LIBC_START_MAIN): Use the updated version
of the cleanup buffer.
* nptl/cleanup.c (__pthread_register_cancel): Use UNWIND_BUF_PRIV
to access the priv field in the cleanup buffer.
(__pthread_unregister_cancel): Likewise.
* nptl/cleanup_defer.c (__pthread_register_cancel_defer):
Likewise.
(__pthread_unregister_cancel_restore): Likewise.
* nptl/unwind.c (unwind_stop): Likewise.
(__pthread_unwind_next): Likewise.
* nptl/descr.h (pthread_unwind_buf_data): New.
(updated_pthread_unwind_buf): Likewise.
(compat_pthread_unwind_buf): Likewise.
(pthread_unwind_buf): Updated to use updated_pthread_unwind_buf
and compat_pthread_unwind_buf.
(UNWIND_BUF_PRIV): New. Macro to get pointer to the priv field
in the cleanup buffer.
* nptl/pthread_create.c (START_THREAD_DEFN): Use the updated
version of the cleanup buffer.
(__pthread_create_2_1): Use THREAD_COPY_ADDITONAL_INFO to copy
additonal info if defined.
* sysdeps/unix/sysv/linux/x86/nptl/pthreadP.h: Use the updated
version of the cleanup buffer to check cancel_jmp_buf size.
* sysdeps/unix/sysv/linux/x86/pthreaddef.h
(THREAD_COPY_ADDITONAL_INFO): New.
(UNWIND_BUF_PRIV): Likewise.
---
csu/libc-start.c | 6 ++-
nptl/cleanup.c | 10 ++--
nptl/cleanup_defer.c | 18 ++++---
nptl/descr.h | 75 ++++++++++++++++++++++-------
nptl/pthread_create.c | 9 +++-
nptl/unwind.c | 6 ++-
sysdeps/unix/sysv/linux/x86/nptl/pthreadP.h | 2 +-
sysdeps/unix/sysv/linux/x86/pthreaddef.h | 14 ++++++
8 files changed, 106 insertions(+), 34 deletions(-)
@@ -298,8 +298,10 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
struct pthread *self = THREAD_SELF;
/* Store old info. */
- unwind_buf.priv.data.prev = THREAD_GETMEM (self, cleanup_jmp_buf);
- unwind_buf.priv.data.cleanup = THREAD_GETMEM (self, cleanup);
+ unwind_buf.updated.priv.data.prev
+ = THREAD_GETMEM (self, cleanup_jmp_buf);
+ unwind_buf.updated.priv.data.cleanup
+ = THREAD_GETMEM (self, cleanup);
/* Store the new cleanup handler info. */
THREAD_SETMEM (self, cleanup_jmp_buf, &unwind_buf);
@@ -28,8 +28,10 @@ __pthread_register_cancel (__pthread_unwind_buf_t *buf)
struct pthread *self = THREAD_SELF;
/* Store old info. */
- ibuf->priv.data.prev = THREAD_GETMEM (self, cleanup_jmp_buf);
- ibuf->priv.data.cleanup = THREAD_GETMEM (self, cleanup);
+ UNWIND_BUF_PRIV (self, ibuf)->data.prev
+ = THREAD_GETMEM (self, cleanup_jmp_buf);
+ UNWIND_BUF_PRIV (self, ibuf)->data.cleanup
+ = THREAD_GETMEM (self, cleanup);
/* Store the new cleanup handler info. */
THREAD_SETMEM (self, cleanup_jmp_buf, (struct pthread_unwind_buf *) buf);
@@ -42,7 +44,9 @@ __cleanup_fct_attribute
__pthread_unregister_cancel (__pthread_unwind_buf_t *buf)
{
struct pthread_unwind_buf *ibuf = (struct pthread_unwind_buf *) buf;
+ struct pthread *self = THREAD_SELF;
- THREAD_SETMEM (THREAD_SELF, cleanup_jmp_buf, ibuf->priv.data.prev);
+ THREAD_SETMEM (self, cleanup_jmp_buf,
+ UNWIND_BUF_PRIV (self, ibuf)->data.prev);
}
hidden_def (__pthread_unregister_cancel)
@@ -28,8 +28,10 @@ __pthread_register_cancel_defer (__pthread_unwind_buf_t *buf)
struct pthread *self = THREAD_SELF;
/* Store old info. */
- ibuf->priv.data.prev = THREAD_GETMEM (self, cleanup_jmp_buf);
- ibuf->priv.data.cleanup = THREAD_GETMEM (self, cleanup);
+ UNWIND_BUF_PRIV (self, ibuf)->data.prev
+ = THREAD_GETMEM (self, cleanup_jmp_buf);
+ UNWIND_BUF_PRIV (self, ibuf)->data.cleanup
+ = THREAD_GETMEM (self, cleanup);
int cancelhandling = THREAD_GETMEM (self, cancelhandling);
@@ -49,9 +51,9 @@ __pthread_register_cancel_defer (__pthread_unwind_buf_t *buf)
cancelhandling = curval;
}
- ibuf->priv.data.canceltype = (cancelhandling & CANCELTYPE_BITMASK
- ? PTHREAD_CANCEL_ASYNCHRONOUS
- : PTHREAD_CANCEL_DEFERRED);
+ UNWIND_BUF_PRIV (self, ibuf)->data.canceltype
+ = (cancelhandling & CANCELTYPE_BITMASK
+ ? PTHREAD_CANCEL_ASYNCHRONOUS : PTHREAD_CANCEL_DEFERRED);
/* Store the new cleanup handler info. */
THREAD_SETMEM (self, cleanup_jmp_buf, (struct pthread_unwind_buf *) buf);
@@ -65,10 +67,12 @@ __pthread_unregister_cancel_restore (__pthread_unwind_buf_t *buf)
struct pthread *self = THREAD_SELF;
struct pthread_unwind_buf *ibuf = (struct pthread_unwind_buf *) buf;
- THREAD_SETMEM (self, cleanup_jmp_buf, ibuf->priv.data.prev);
+ THREAD_SETMEM (self, cleanup_jmp_buf,
+ UNWIND_BUF_PRIV (self, ibuf)->data.prev);
int cancelhandling;
- if (ibuf->priv.data.canceltype != PTHREAD_CANCEL_DEFERRED
+ if ((UNWIND_BUF_PRIV (self, ibuf)->data.canceltype
+ != PTHREAD_CANCEL_DEFERRED)
&& ((cancelhandling = THREAD_GETMEM (self, cancelhandling))
& CANCELTYPE_BITMASK) == 0)
{
@@ -55,11 +55,30 @@
/ PTHREAD_KEY_2NDLEVEL_SIZE)
+/* Private data in the cleanup buffer. */
+union pthread_unwind_buf_data
+{
+ /* This is the placeholder of the public version. */
+ void *pad[4];
+
+ struct
+ {
+ /* Pointer to the previous cleanup buffer. */
+ struct pthread_unwind_buf *prev;
+ /* Backward compatibility: state of the old-style cleanup
+ handler at the time of the previous new-style cleanup handler
+ installment. */
+ struct _pthread_cleanup_buffer *cleanup;
-/* Internal version of the buffer to store cancellation handler
+ /* Cancellation type before the push call. */
+ int canceltype;
+ } data;
+};
+
+/* Internal updated version of the buffer to store cancellation handler
information. */
-struct pthread_unwind_buf
+struct updated_pthread_unwind_buf
{
struct
{
@@ -70,27 +89,49 @@ struct pthread_unwind_buf
#endif
} cancel_jmp_buf[1];
- union
+ union pthread_unwind_buf_data priv;
+};
+
+/* Internal compatible version of the buffer to store cancellation
+ handler information. */
+struct compat_pthread_unwind_buf
+{
+ struct
{
- /* This is the placeholder of the public version. */
- void *pad[4];
+ __jmp_buf jmp_buf;
+ int mask_was_saved;
+ } cancel_jmp_buf[1];
+
+ union pthread_unwind_buf_data priv;
+};
+/* Internal version of the buffer to store cancellation handler
+ information. */
+struct pthread_unwind_buf
+{
+ union
+ {
+ /* The common fields of updated and compatible versions. */
struct
{
- /* Pointer to the previous cleanup buffer. */
- struct pthread_unwind_buf *prev;
-
- /* Backward compatibility: state of the old-style cleanup
- handler at the time of the previous new-style cleanup handler
- installment. */
- struct _pthread_cleanup_buffer *cleanup;
-
- /* Cancellation type before the push call. */
- int canceltype;
- } data;
- } priv;
+ __jmp_buf jmp_buf;
+ int mask_was_saved;
+ } cancel_jmp_buf[1];
+ struct updated_pthread_unwind_buf updated;
+ struct compat_pthread_unwind_buf compat;
+ };
};
+/* Get pointer to the priv field from THREAD_SELF, "self", and pointer
+ to the cleanup buffer, "p". By default, the compatible version is
+ used. If a target defines NEED_SAVED_MASK_IN_CANCEL_JMP_BUF, it
+ must provide its own version of UNEIND_BUF_PRIV. */
+#ifndef UNWIND_BUF_PRIV
+# ifdef NEED_SAVED_MASK_IN_CANCEL_JMP_BUF
+# error "UNWIND_BUF_PRIV is undefined!"
+# endif
+# define UNWIND_BUF_PRIV(self,p) (&((p)->compat.priv))
+#endif
/* Opcodes and data types for communication with the signal handler to
change user/group IDs. */
@@ -428,8 +428,8 @@ START_THREAD_DEFN
struct pthread_unwind_buf unwind_buf;
/* No previous handlers. */
- unwind_buf.priv.data.prev = NULL;
- unwind_buf.priv.data.cleanup = NULL;
+ unwind_buf.updated.priv.data.prev = NULL;
+ unwind_buf.updated.priv.data.cleanup = NULL;
int not_first_call;
not_first_call = setjmp ((struct __jmp_buf_tag *) unwind_buf.cancel_jmp_buf);
@@ -701,6 +701,11 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
THREAD_COPY_POINTER_GUARD (pd);
#endif
+ /* Copy additonal info. */
+#ifdef THREAD_COPY_ADDITONAL_INFO
+ THREAD_COPY_ADDITONAL_INFO (pd);
+#endif
+
/* Verify the sysinfo bits were copied in allocate_stack if needed. */
#ifdef NEED_DL_SYSINFO
CHECK_THREAD_SYSINFO (pd);
@@ -66,7 +66,8 @@ unwind_stop (int version, _Unwind_Action actions,
/* Handle the compatibility stuff. Execute all handlers
registered with the old method which would be unwound by this
step. */
- struct _pthread_cleanup_buffer *oldp = buf->priv.data.cleanup;
+ struct _pthread_cleanup_buffer *oldp
+ = UNWIND_BUF_PRIV (self, buf)->data.cleanup;
void *cfa = (void *) (_Unwind_Ptr) _Unwind_GetCFA (context);
if (curp != oldp && (do_longjump || FRAME_LEFT (cfa, curp, adj)))
@@ -133,6 +134,7 @@ __pthread_unwind_next (__pthread_unwind_buf_t *buf)
{
struct pthread_unwind_buf *ibuf = (struct pthread_unwind_buf *) buf;
- __pthread_unwind ((__pthread_unwind_buf_t *) ibuf->priv.data.prev);
+ __pthread_unwind ((__pthread_unwind_buf_t *)
+ UNWIND_BUF_PRIV (THREAD_SELF, ibuf)->data.prev);
}
hidden_def (__pthread_unwind_next)
@@ -23,7 +23,7 @@
extern struct pthread_unwind_buf ____pthread_unwind_buf_private;
-_Static_assert (sizeof (____pthread_unwind_buf_private.cancel_jmp_buf)
+_Static_assert (sizeof (____pthread_unwind_buf_private.updated.cancel_jmp_buf)
>= sizeof (struct __jmp_buf_tag),
"size of cancel_jmp_buf < sizeof __jmp_buf_tag");
@@ -20,3 +20,17 @@
/* Need saved_mask in cancel_jmp_buf. */
#define NEED_SAVED_MASK_IN_CANCEL_JMP_BUF 1
+
+/* Wee need to copy feature_1 in pthread_create. */
+#define THREAD_COPY_ADDITONAL_INFO(descr) \
+ ((descr)->header.feature_1 \
+ = THREAD_GETMEM (THREAD_SELF, header.feature_1))
+
+/* Use the compatible struct __cancel_jmp_buf_tag if shadow stack is
+ disabled. */
+#undef UNWIND_BUF_PRIV
+#define UNWIND_BUF_PRIV(self,p) \
+ (__extension__ ({ \
+ unsigned int feature_1 = THREAD_GETMEM (self, header.feature_1); \
+ (((feature_1 & (1 << 1)) == 0) \
+ ? &((p)->compat.priv) : &((p)->updated.priv));}))
--
2.14.3