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

["Tsimbalist, Igor V" <igor dot v dot tsimbalist at intel dot com>]

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