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

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

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.


-- 
H.J.