PING^1 [PATCH] Call _dl_open_check after relocation is finished [BZ #24259]

  On Tue, Mar 5, 2019 at 6:00 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> >> If you report the error at this, doesn't this mean the object is still
> >> around and in a bad state?  This looks related to this bug:
> >
> > Yes.
> >
> >>   <https://sourceware.org/bugzilla/show_bug.cgi?id=20839>
> >>
> >> Would the CET bug go away if we got rid after the object without trace
> >> after a failure in _dl_open_check?
> >
> > Yes.
> >
> >> I can look into fixing the other bug, but I don't know how hard that's
> >> going to be.
>
> I did that now and the required infrastructure changes are fairly
> involved.  So I think we should add something that works today.
>
> A natural place for the CET compatibility check would be
> elf_machine_reject_phdr_p (currently used only on MIPS).  This way, we
> can continue searching for a CET-compatible library along the search
> path.

We only want to check it in dlopen path.  A legacy shared library can
loaded before main () and shadow stack will be disabled.

> >> > diff --git a/sysdeps/x86/tst-cet-legacy-5.c b/sysdeps/x86/tst-cet-legacy-5.c
> >> > new file mode 100644
> >> > index 0000000000..fbf640f664
> >> > --- /dev/null
> >> > +++ b/sysdeps/x86/tst-cet-legacy-5.c
> >>
> >> > +static void
> >> > +do_test_1 (const char *modname, bool fail)
> >> > +{
> >> > +  int (*fp) (void);
> >> > +  void *h;
> >> > +
> >> > +  h = dlopen (modname, RTLD_LAZY);
> >> > +  if (h == NULL)
> >> > +    {
> >> > +      if (fail)
> >> > +     {
> >> > +       const char *err = dlerror ();
> >> > +       if (strstr (err, "shadow stack isn't enabled") == NULL)
> >> > +         {
> >> > +           printf ("incorrect dlopen '%s' error: %s\n", modname,
> >> > +                   dlerror ());
> >> > +           exit (1);
> >> > +         }
> >> > +
> >> > +       return;
> >> > +     }
> >>
> >> Is the return supposed to be taken if running on non-CET hardware?  I'm
> >> looking for the UNSUPPORTED case.
> >
> > This path is taken only on CET hardware.  For non-CET hardware, 'h' shouldn't
> > be NULL.
>
> Please add logging to the test for the unsupported case, so that the aim
> of the test is clearer and when it fails to achieve its objective.
>

The tests should run on CET/non-CET kernel/processor.    Here is
the updated patch with FAIL_EXIT1.

OK for master?

Thanks.
  

On Tue, Apr 9, 2019 at 3:56 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Mar 5, 2019 at 6:00 AM Florian Weimer <fweimer@redhat.com> wrote:
> >
> > * H. J. Lu:
> >
> > >> If you report the error at this, doesn't this mean the object is still
> > >> around and in a bad state?  This looks related to this bug:
> > >
> > > Yes.
> > >
> > >>   <https://sourceware.org/bugzilla/show_bug.cgi?id=20839>
> > >>
> > >> Would the CET bug go away if we got rid after the object without trace
> > >> after a failure in _dl_open_check?
> > >
> > > Yes.
> > >
> > >> I can look into fixing the other bug, but I don't know how hard that's
> > >> going to be.
> >
> > I did that now and the required infrastructure changes are fairly
> > involved.  So I think we should add something that works today.
> >
> > A natural place for the CET compatibility check would be
> > elf_machine_reject_phdr_p (currently used only on MIPS).  This way, we
> > can continue searching for a CET-compatible library along the search
> > path.
>
> We only want to check it in dlopen path.  A legacy shared library can
> loaded before main () and shadow stack will be disabled.
>
> > >> > diff --git a/sysdeps/x86/tst-cet-legacy-5.c b/sysdeps/x86/tst-cet-legacy-5.c
> > >> > new file mode 100644
> > >> > index 0000000000..fbf640f664
> > >> > --- /dev/null
> > >> > +++ b/sysdeps/x86/tst-cet-legacy-5.c
> > >>
> > >> > +static void
> > >> > +do_test_1 (const char *modname, bool fail)
> > >> > +{
> > >> > +  int (*fp) (void);
> > >> > +  void *h;
> > >> > +
> > >> > +  h = dlopen (modname, RTLD_LAZY);
> > >> > +  if (h == NULL)
> > >> > +    {
> > >> > +      if (fail)
> > >> > +     {
> > >> > +       const char *err = dlerror ();
> > >> > +       if (strstr (err, "shadow stack isn't enabled") == NULL)
> > >> > +         {
> > >> > +           printf ("incorrect dlopen '%s' error: %s\n", modname,
> > >> > +                   dlerror ());
> > >> > +           exit (1);
> > >> > +         }
> > >> > +
> > >> > +       return;
> > >> > +     }
> > >>
> > >> Is the return supposed to be taken if running on non-CET hardware?  I'm
> > >> looking for the UNSUPPORTED case.
> > >
> > > This path is taken only on CET hardware.  For non-CET hardware, 'h' shouldn't
> > > be NULL.
> >
> > Please add logging to the test for the unsupported case, so that the aim
> > of the test is clearer and when it fails to achieve its objective.
> >
>
> The tests should run on CET/non-CET kernel/processor.    Here is
> the updated patch with FAIL_EXIT1.
>
> OK for master?
>

PING:

https://sourceware.org/ml/libc-alpha/2019-04/msg00228.html
  

* H. J. Lu:

> diff --git a/elf/dl-open.c b/elf/dl-open.c
> index 12a4f8b853..a144a40790 100644
> --- a/elf/dl-open.c
> +++ b/elf/dl-open.c
> @@ -292,8 +292,6 @@ dl_open_worker (void *a)
>    _dl_debug_state ();
>    LIBC_PROBE (map_complete, 3, args->nsid, r, new);
>  
> -  _dl_open_check (new);
> -
>    /* Print scope information.  */
>    if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_SCOPES))
>      _dl_show_scope (new, 0);
> @@ -366,6 +364,11 @@ dl_open_worker (void *a)
>  	_dl_relocate_object (l, l->l_scope, reloc_mode, 0);
>      }
>  
> +  /* NB: Since _dl_open_check may throw an exception, it must be called
> +     after relocation is finished.   Otherwise, a shared object may be
> +     mmapped without relocation.  */
> +  _dl_open_check (new);
> +
>    /* If the file is not loaded now as a dependency, add the search
>       list of the newly loaded object to the scope.  */
>    bool any_tls = false;

Presumably you test this using libpthread because it's NODELETE?

I'm not sure if this correct.  Won't this leave behind a relocated
NODELTE objects whose constructors have not run?  That would still leave
the process in a bad state.

Thanks,
Florian
  

On Wed, Jun 26, 2019 at 10:22 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > diff --git a/elf/dl-open.c b/elf/dl-open.c
> > index 12a4f8b853..a144a40790 100644
> > --- a/elf/dl-open.c
> > +++ b/elf/dl-open.c
> > @@ -292,8 +292,6 @@ dl_open_worker (void *a)
> >    _dl_debug_state ();
> >    LIBC_PROBE (map_complete, 3, args->nsid, r, new);
> >
> > -  _dl_open_check (new);
> > -
> >    /* Print scope information.  */
> >    if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_SCOPES))
> >      _dl_show_scope (new, 0);
> > @@ -366,6 +364,11 @@ dl_open_worker (void *a)
> >       _dl_relocate_object (l, l->l_scope, reloc_mode, 0);
> >      }
> >
> > +  /* NB: Since _dl_open_check may throw an exception, it must be called
> > +     after relocation is finished.   Otherwise, a shared object may be
> > +     mmapped without relocation.  */
> > +  _dl_open_check (new);
> > +
> >    /* If the file is not loaded now as a dependency, add the search
> >       list of the newly loaded object to the scope.  */
> >    bool any_tls = false;
>
> Presumably you test this using libpthread because it's NODELETE?

Yes.

> I'm not sure if this correct.  Won't this leave behind a relocated
> NODELTE objects whose constructors have not run?  That would still leave
> the process in a bad state.

Such library is only mapped and relocated, but nothing else.   Next loading
will continue and finish the rest of loading steps, including calling
constructors.
  

* H. J. Lu:

> On Wed, Jun 26, 2019 at 10:22 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * H. J. Lu:
>>
>> > diff --git a/elf/dl-open.c b/elf/dl-open.c
>> > index 12a4f8b853..a144a40790 100644
>> > --- a/elf/dl-open.c
>> > +++ b/elf/dl-open.c
>> > @@ -292,8 +292,6 @@ dl_open_worker (void *a)
>> >    _dl_debug_state ();
>> >    LIBC_PROBE (map_complete, 3, args->nsid, r, new);
>> >
>> > -  _dl_open_check (new);
>> > -
>> >    /* Print scope information.  */
>> >    if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_SCOPES))
>> >      _dl_show_scope (new, 0);
>> > @@ -366,6 +364,11 @@ dl_open_worker (void *a)
>> >       _dl_relocate_object (l, l->l_scope, reloc_mode, 0);
>> >      }
>> >
>> > +  /* NB: Since _dl_open_check may throw an exception, it must be called
>> > +     after relocation is finished.   Otherwise, a shared object may be
>> > +     mmapped without relocation.  */
>> > +  _dl_open_check (new);
>> > +
>> >    /* If the file is not loaded now as a dependency, add the search
>> >       list of the newly loaded object to the scope.  */
>> >    bool any_tls = false;
>>
>> Presumably you test this using libpthread because it's NODELETE?
>
> Yes.

Ah, so it's related to bug 20839.

>> I'm not sure if this correct.  Won't this leave behind a relocated
>> NODELTE objects whose constructors have not run?  That would still leave
>> the process in a bad state.
>
> Such library is only mapped and relocated, but nothing else.   Next loading
> will continue and finish the rest of loading steps, including calling
> constructors.

But relocation can fail and unwind, too, as it happens in bug 20839.  So
at least the comment is misleading.

Clearly, moving around the check is not the proper fix, but maybe it's
the best option we have at present.

Have you investigate if we can remove again NODELETE mappings which have
been partially relocated?

Thanks,
Florian
  

On Thu, Jun 27, 2019 at 12:17 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Wed, Jun 26, 2019 at 10:22 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * H. J. Lu:
> >>
> >> > diff --git a/elf/dl-open.c b/elf/dl-open.c
> >> > index 12a4f8b853..a144a40790 100644
> >> > --- a/elf/dl-open.c
> >> > +++ b/elf/dl-open.c
> >> > @@ -292,8 +292,6 @@ dl_open_worker (void *a)
> >> >    _dl_debug_state ();
> >> >    LIBC_PROBE (map_complete, 3, args->nsid, r, new);
> >> >
> >> > -  _dl_open_check (new);
> >> > -
> >> >    /* Print scope information.  */
> >> >    if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_SCOPES))
> >> >      _dl_show_scope (new, 0);
> >> > @@ -366,6 +364,11 @@ dl_open_worker (void *a)
> >> >       _dl_relocate_object (l, l->l_scope, reloc_mode, 0);
> >> >      }
> >> >
> >> > +  /* NB: Since _dl_open_check may throw an exception, it must be called
> >> > +     after relocation is finished.   Otherwise, a shared object may be
> >> > +     mmapped without relocation.  */
> >> > +  _dl_open_check (new);
> >> > +
> >> >    /* If the file is not loaded now as a dependency, add the search
> >> >       list of the newly loaded object to the scope.  */
> >> >    bool any_tls = false;
> >>
> >> Presumably you test this using libpthread because it's NODELETE?
> >
> > Yes.
>
> Ah, so it's related to bug 20839.
>
> >> I'm not sure if this correct.  Won't this leave behind a relocated
> >> NODELTE objects whose constructors have not run?  That would still leave
> >> the process in a bad state.
> >
> > Such library is only mapped and relocated, but nothing else.   Next loading
> > will continue and finish the rest of loading steps, including calling
> > constructors.
>
> But relocation can fail and unwind, too, as it happens in bug 20839.  So
> at least the comment is misleading.

I updated comments.

> Clearly, moving around the check is not the proper fix, but maybe it's
> the best option we have at present.

That is true.

> Have you investigate if we can remove again NODELETE mappings which have
> been partially relocated?

No.

I added 2 testcase, NODELETE and non-NODELETE.  NODELETE fails without
my change.   non-NODELETE works with and without my change.
  

* H. J. Lu:

> diff --git a/elf/dl-open.c b/elf/dl-open.c
> index 12a4f8b853..e18ee398cb 100644
> --- a/elf/dl-open.c
> +++ b/elf/dl-open.c
> @@ -292,8 +292,6 @@ dl_open_worker (void *a)
>    _dl_debug_state ();
>    LIBC_PROBE (map_complete, 3, args->nsid, r, new);
>  
> -  _dl_open_check (new);
> -
>    /* Print scope information.  */
>    if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_SCOPES))
>      _dl_show_scope (new, 0);
> @@ -366,6 +364,12 @@ dl_open_worker (void *a)
>  	_dl_relocate_object (l, l->l_scope, reloc_mode, 0);
>      }
>  
> +  /* NB: Workaround for [BZ #20839] which doesn't remove the NODELETE
> +     object when _dl_open_check throws an exception.  Move it after
> +     relocation to avoid leaving the NODELETE object mapped without
> +     relocation.  */
> +  _dl_open_check (new);
> +
>    /* If the file is not loaded now as a dependency, add the search
>       list of the newly loaded object to the scope.  */
>    bool any_tls = false;

The downside is that an undefined symbol will now mask the CET failure.
Is this a problem?

Overall, I can't say I'm happy about this patch, but if it addresses
important breakage, I think this is okay for now.  If we fix the state
rollback properly, then hopefully we can move the check further up
again, where it probably belongs.

Thnaks,
Florian
  

On Mon, Jul 1, 2019 at 9:42 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > diff --git a/elf/dl-open.c b/elf/dl-open.c
> > index 12a4f8b853..e18ee398cb 100644
> > --- a/elf/dl-open.c
> > +++ b/elf/dl-open.c
> > @@ -292,8 +292,6 @@ dl_open_worker (void *a)
> >    _dl_debug_state ();
> >    LIBC_PROBE (map_complete, 3, args->nsid, r, new);
> >
> > -  _dl_open_check (new);
> > -
> >    /* Print scope information.  */
> >    if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_SCOPES))
> >      _dl_show_scope (new, 0);
> > @@ -366,6 +364,12 @@ dl_open_worker (void *a)
> >       _dl_relocate_object (l, l->l_scope, reloc_mode, 0);
> >      }
> >
> > +  /* NB: Workaround for [BZ #20839] which doesn't remove the NODELETE
> > +     object when _dl_open_check throws an exception.  Move it after
> > +     relocation to avoid leaving the NODELETE object mapped without
> > +     relocation.  */
> > +  _dl_open_check (new);
> > +
> >    /* If the file is not loaded now as a dependency, add the search
> >       list of the newly loaded object to the scope.  */
> >    bool any_tls = false;
>
> The downside is that an undefined symbol will now mask the CET failure.
> Is this a problem?

Since both problems will lead to load failure, there is no difference
in run-time
behavior except that the CET failure won't cause program crash now.

> Overall, I can't say I'm happy about this patch, but if it addresses
> important breakage, I think this is okay for now.  If we fix the state
> rollback properly, then hopefully we can move the check further up
> again, where it probably belongs.
>

Sure.  I am checking in my patch now.

Thanks.