[0/2] nptl: Update struct pthread_unwind_buf

  On Wed, Mar 7, 2018 at 12:14 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Mar 7, 2018 at 11:47 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Wed, Mar 7, 2018 at 9:34 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>>> On 03/07/2018 03:56 AM, H.J. Lu wrote:
>>>>>> 1. I have to add  __setjmp_cancel and __sigsetjmp_cancel which won't
>>>>>> save and restore shadow stack register.
>>>>
>>>> I have been testing this.  I ran into one issue.  GCC knows that setjmp will
>>>> return via longjmp and inserts ENDBR after it.  But it doesn't know
>>>>   __setjmp_cancel and __sigsetjmp_cancel.   We can either add them to GCC
>>>> or add NOTRACK prefix to the corresponding longjmps.
>>>
>>> I would rather GCC did not know about these implementation details.
>>>
>>> I have no objection to the NOTRACK prefix in the corresponding longjmps.
>>>
>>> What would be a downside to this choice?
>>>
>>
>> NOTRACK prefix is typically generated by compiler for switch table.  Compiler
>> knows each indirect jump target is valid and pointer load for indirect jump is
>> generated by compiler in read-only section.  This is pretty safe since there is
>> very little chance for malicious codes to temper the pointer value.  However,
>> in case of longjmp, the indirect jump target is in jmpbuf.   There is
>> a possilibty
>> for malicious codes to change the indirect jump target such that longjmp wil
>> jump to the wrong place.  Use NOTRACK prefix here defeats the purpose of
>> indirect branch tracking in CET.
>>
>
> Also GCC needs to know that __setjmp_cancel and __sigsetjmp_cancel may
> return twice, similar to setjmp.
>

Here is the GCC patch:
  

> -----Original Message-----

> From: H.J. Lu [mailto:hjl.tools@gmail.com]

> Sent: Wednesday, March 7, 2018 11:07 PM

> To: Carlos O'Donell <carlos@redhat.com>; Tsimbalist, Igor V

> <igor.v.tsimbalist@intel.com>

> Cc: Florian Weimer <fw@deneb.enyo.de>; GNU C Library <libc-

> alpha@sourceware.org>

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

> 

> On Wed, Mar 7, 2018 at 12:14 PM, H.J. Lu <hjl.tools@gmail.com> wrote:

> > On Wed, Mar 7, 2018 at 11:47 AM, H.J. Lu <hjl.tools@gmail.com> wrote:

> >> On Wed, Mar 7, 2018 at 9:34 AM, Carlos O'Donell <carlos@redhat.com>

> wrote:

> >>> On 03/07/2018 03:56 AM, H.J. Lu wrote:

> >>>>>> 1. I have to add  __setjmp_cancel and __sigsetjmp_cancel which

> won't

> >>>>>> save and restore shadow stack register.

> >>>>

> >>>> I have been testing this.  I ran into one issue.  GCC knows that setjmp

> will

> >>>> return via longjmp and inserts ENDBR after it.  But it doesn't know

> >>>>   __setjmp_cancel and __sigsetjmp_cancel.   We can either add them to

> GCC

> >>>> or add NOTRACK prefix to the corresponding longjmps.

> >>>

> >>> I would rather GCC did not know about these implementation details.

> >>>

> >>> I have no objection to the NOTRACK prefix in the corresponding

> longjmps.

> >>>

> >>> What would be a downside to this choice?

> >>>

> >>

> >> NOTRACK prefix is typically generated by compiler for switch table.

> Compiler

> >> knows each indirect jump target is valid and pointer load for indirect

> jump is

> >> generated by compiler in read-only section.  This is pretty safe since there

> is

> >> very little chance for malicious codes to temper the pointer value.

> However,

> >> in case of longjmp, the indirect jump target is in jmpbuf.   There is

> >> a possilibty

> >> for malicious codes to change the indirect jump target such that longjmp

> wil

> >> jump to the wrong place.  Use NOTRACK prefix here defeats the purpose

> of

> >> indirect branch tracking in CET.

> >>

> >

> > Also GCC needs to know that __setjmp_cancel and __sigsetjmp_cancel may

> > return twice, similar to setjmp.

> >

> 

> Here is the GCC patch:

> 

> 

> diff --git a/gcc/calls.c b/gcc/calls.c

> index 19c95b8455b..d1f436dfa91 100644

> --- a/gcc/calls.c

> +++ b/gcc/calls.c

> @@ -604,7 +604,7 @@ special_function_p (const_tree fndecl, int flags)

>      name_decl = DECL_NAME (cgraph_node::get (fndecl)->orig_decl);

> 

>    if (fndecl && name_decl

> -      && IDENTIFIER_LENGTH (name_decl) <= 11

> +      && IDENTIFIER_LENGTH (name_decl) <= 18

>        /* Exclude functions not at the file scope, or not `extern',

>     since they are not the magic functions we would otherwise

>     think they are.

> @@ -637,8 +637,8 @@ special_function_p (const_tree fndecl, int flags)

>    }

> 

>        /* ECF_RETURNS_TWICE is safe even for -ffreestanding.  */

> -      if (! strcmp (tname, "setjmp")

> -    || ! strcmp (tname, "sigsetjmp")

> +      if (! strncmp (tname, "setjmp", 6)

> +    || ! strncmp (tname, "sigsetjmp", 9)

>      || ! strcmp (name, "savectx")

>      || ! strcmp (name, "vfork")

>      || ! strcmp (name, "getcontext"))


Should it be compared with the whole function name (__setjmp_cancel and
__sigsetjmp_cancel) as something like setjmp_my_func will be also detected?

Also there was an error in detection of __getcontext, which is 12 bytes, but it
will be fixed by this patch.

Igor

> --

> H.J.
  

On Thu, Mar 8, 2018 at 4:24 AM, Tsimbalist, Igor V
<igor.v.tsimbalist@intel.com> wrote:
>> -----Original Message-----
>> From: H.J. Lu [mailto:hjl.tools@gmail.com]
>> Sent: Wednesday, March 7, 2018 11:07 PM
>> To: Carlos O'Donell <carlos@redhat.com>; Tsimbalist, Igor V
>> <igor.v.tsimbalist@intel.com>
>> Cc: Florian Weimer <fw@deneb.enyo.de>; GNU C Library <libc-
>> alpha@sourceware.org>
>> Subject: Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf
>>
>> On Wed, Mar 7, 2018 at 12:14 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> > On Wed, Mar 7, 2018 at 11:47 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> >> On Wed, Mar 7, 2018 at 9:34 AM, Carlos O'Donell <carlos@redhat.com>
>> wrote:
>> >>> On 03/07/2018 03:56 AM, H.J. Lu wrote:
>> >>>>>> 1. I have to add  __setjmp_cancel and __sigsetjmp_cancel which
>> won't
>> >>>>>> save and restore shadow stack register.
>> >>>>
>> >>>> I have been testing this.  I ran into one issue.  GCC knows that setjmp
>> will
>> >>>> return via longjmp and inserts ENDBR after it.  But it doesn't know
>> >>>>   __setjmp_cancel and __sigsetjmp_cancel.   We can either add them to
>> GCC
>> >>>> or add NOTRACK prefix to the corresponding longjmps.
>> >>>
>> >>> I would rather GCC did not know about these implementation details.
>> >>>
>> >>> I have no objection to the NOTRACK prefix in the corresponding
>> longjmps.
>> >>>
>> >>> What would be a downside to this choice?
>> >>>
>> >>
>> >> NOTRACK prefix is typically generated by compiler for switch table.
>> Compiler
>> >> knows each indirect jump target is valid and pointer load for indirect
>> jump is
>> >> generated by compiler in read-only section.  This is pretty safe since there
>> is
>> >> very little chance for malicious codes to temper the pointer value.
>> However,
>> >> in case of longjmp, the indirect jump target is in jmpbuf.   There is
>> >> a possilibty
>> >> for malicious codes to change the indirect jump target such that longjmp
>> wil
>> >> jump to the wrong place.  Use NOTRACK prefix here defeats the purpose
>> of
>> >> indirect branch tracking in CET.
>> >>
>> >
>> > Also GCC needs to know that __setjmp_cancel and __sigsetjmp_cancel may
>> > return twice, similar to setjmp.
>> >
>>
>> Here is the GCC patch:
>>
>>
>> diff --git a/gcc/calls.c b/gcc/calls.c
>> index 19c95b8455b..d1f436dfa91 100644
>> --- a/gcc/calls.c
>> +++ b/gcc/calls.c
>> @@ -604,7 +604,7 @@ special_function_p (const_tree fndecl, int flags)
>>      name_decl = DECL_NAME (cgraph_node::get (fndecl)->orig_decl);
>>
>>    if (fndecl && name_decl
>> -      && IDENTIFIER_LENGTH (name_decl) <= 11
>> +      && IDENTIFIER_LENGTH (name_decl) <= 18
>>        /* Exclude functions not at the file scope, or not `extern',
>>     since they are not the magic functions we would otherwise
>>     think they are.
>> @@ -637,8 +637,8 @@ special_function_p (const_tree fndecl, int flags)
>>    }
>>
>>        /* ECF_RETURNS_TWICE is safe even for -ffreestanding.  */
>> -      if (! strcmp (tname, "setjmp")
>> -    || ! strcmp (tname, "sigsetjmp")
>> +      if (! strncmp (tname, "setjmp", 6)
>> +    || ! strncmp (tname, "sigsetjmp", 9)
>>      || ! strcmp (name, "savectx")
>>      || ! strcmp (name, "vfork")
>>      || ! strcmp (name, "getcontext"))
>
> Should it be compared with the whole function name (__setjmp_cancel and
> __sigsetjmp_cancel) as something like setjmp_my_func will be also detected?

True.  This is the patch I have tested:

https://github.com/hjl-tools/gcc/commit/e98087865405f051e93d5f35588789ef9686db4a

> Also there was an error in detection of __getcontext, which is 12 bytes, but it
> will be fixed by this patch.
  

On 03/08/2018 04:48 AM, H.J. Lu wrote:
> True.  This is the patch I have tested:
> 
> https://github.com/hjl-tools/gcc/commit/e98087865405f051e93d5f35588789ef9686db4a

I assume NOTRACK prefix is what Intel calls 'NO_TRACK_EN' in the CET
documentation since binutils uses the 0x3E prefix for it and that matches
the Intel CET docs. Please correct me if I'm wrong.

In which case I agree, using NOTRACK is going to prevent a useful use
of CET against writes to the cancellation jump buffer.

This patch looks good to me, but is not a *correctness* issue, it is
simply that we want to extend coverage to the private cancellation
setjmp/longjmp buffers.

Is there any way we can do this with source markup instead of via
a fragile list in the compiler?

Presumably users may want to markup their own code like this also
if they have custom implementations of functions that behave like
setjmp/longjmp?
  

On Thu, Mar 8, 2018 at 4:47 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 03/08/2018 04:48 AM, H.J. Lu wrote:
>> True.  This is the patch I have tested:
>>
>> https://github.com/hjl-tools/gcc/commit/e98087865405f051e93d5f35588789ef9686db4a
>
> I assume NOTRACK prefix is what Intel calls 'NO_TRACK_EN' in the CET
> documentation since binutils uses the 0x3E prefix for it and that matches
> the Intel CET docs. Please correct me if I'm wrong.

That is correct.

> In which case I agree, using NOTRACK is going to prevent a useful use
> of CET against writes to the cancellation jump buffer.

True.

> This patch looks good to me, but is not a *correctness* issue, it is
> simply that we want to extend coverage to the private cancellation
> setjmp/longjmp buffers.
>
> Is there any way we can do this with source markup instead of via
> a fragile list in the compiler?
>
> Presumably users may want to markup their own code like this also
> if they have custom implementations of functions that behave like
> setjmp/longjmp?
>

Yes, we can use __attribute__((__returns_twice__)).  I updated
hjl/setjmp/cancel branch to do that.  No GCC changes are needed.
  

On 03/08/2018 10:23 PM, H.J. Lu wrote:
> On Thu, Mar 8, 2018 at 4:47 PM, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 03/08/2018 04:48 AM, H.J. Lu wrote:
>>> True.  This is the patch I have tested:
>>>
>>> https://github.com/hjl-tools/gcc/commit/e98087865405f051e93d5f35588789ef9686db4a
>>
>> I assume NOTRACK prefix is what Intel calls 'NO_TRACK_EN' in the CET
>> documentation since binutils uses the 0x3E prefix for it and that matches
>> the Intel CET docs. Please correct me if I'm wrong.
> 
> That is correct.
> 
>> In which case I agree, using NOTRACK is going to prevent a useful use
>> of CET against writes to the cancellation jump buffer.
> 
> True.
> 
>> This patch looks good to me, but is not a *correctness* issue, it is
>> simply that we want to extend coverage to the private cancellation
>> setjmp/longjmp buffers.
>>
>> Is there any way we can do this with source markup instead of via
>> a fragile list in the compiler?
>>
>> Presumably users may want to markup their own code like this also
>> if they have custom implementations of functions that behave like
>> setjmp/longjmp?
>>
> 
> Yes, we can use __attribute__((__returns_twice__)).  I updated
> hjl/setjmp/cancel branch to do that.  No GCC changes are needed.

Is the next step for me to do another round of review on this branch?

Cheers,
Carlos.