Call _dl_open_check after relocation is finished [BZ #24259]
Commit Message
When dlopening a shared object, _dl_open_check will throw an exception
if CET shadow stack is enabled and the shared object has no shadow stack
support. dl_open_worker must call _dl_open_check after relocation is
finished. Otherwise, the dependency shared objects may be mmapped
without relocation. This will lead to run-time failure later when they
are needed by another dlopened shared object which is shadow stack
enabled.
[BZ #24259]
* elf/dl-open.c (dl_open_worker): Call _dl_open_check after
relocation is finished.
* sysdeps/x86/Makefile (tests): Add tst-cet-legacy-5a and
tst-cet-legacy-5b.
(modules-names): Add tst-cet-legacy-mod-5a and
tst-cet-legacy-mod-5b.
(CFLAGS-tst-cet-legacy-5a.c): New.
(CFLAGS-tst-cet-legacy-5b.c): Likewise.
(CFLAGS-tst-cet-legacy-mod-5a.c): Likewise.
(CFLAGS-tst-cet-legacy-mod-5b.c): Likewise.
($(objpfx)tst-cet-legacy-5a): Likewise.
($(objpfx)tst-cet-legacy-5a.out): Likewise.
($(objpfx)tst-cet-legacy-mod-5a.so): Likewise.
($(objpfx)tst-cet-legacy-mod-5b.so): Likewise.
($(objpfx)tst-cet-legacy-5b): Likewise.
($(objpfx)tst-cet-legacy-5b.out): Likewise.
(tst-cet-legacy-5b-ENV): Likewise.
* sysdeps/x86/tst-cet-legacy-5.c: New file.
* sysdeps/x86/tst-cet-legacy-5a.c: Likewise.
* sysdeps/x86/tst-cet-legacy-5b.c: Likewise.
* sysdeps/x86/tst-cet-legacy-mod-5.c: Likewise.
* sysdeps/x86/tst-cet-legacy-mod-5a.c: Likewise.
* sysdeps/x86/tst-cet-legacy-mod-5b.c: Likewise.
---
elf/dl-open.c | 7 ++-
sysdeps/x86/Makefile | 22 ++++++-
sysdeps/x86/tst-cet-legacy-5.c | 76 ++++++++++++++++++++++++
sysdeps/x86/tst-cet-legacy-5a.c | 1 +
sysdeps/x86/tst-cet-legacy-5b.c | 1 +
sysdeps/x86/tst-cet-legacy-mod-5.c | 91 +++++++++++++++++++++++++++++
sysdeps/x86/tst-cet-legacy-mod-5a.c | 1 +
sysdeps/x86/tst-cet-legacy-mod-5b.c | 1 +
8 files changed, 195 insertions(+), 5 deletions(-)
create mode 100644 sysdeps/x86/tst-cet-legacy-5.c
create mode 100644 sysdeps/x86/tst-cet-legacy-5a.c
create mode 100644 sysdeps/x86/tst-cet-legacy-5b.c
create mode 100644 sysdeps/x86/tst-cet-legacy-mod-5.c
create mode 100644 sysdeps/x86/tst-cet-legacy-mod-5a.c
create mode 100644 sysdeps/x86/tst-cet-legacy-mod-5b.c
Comments
On Sun, Feb 24, 2019 at 8:02 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> When dlopening a shared object, _dl_open_check will throw an exception
> if CET shadow stack is enabled and the shared object has no shadow stack
> support. dl_open_worker must call _dl_open_check after relocation is
> finished. Otherwise, the dependency shared objects may be mmapped
> without relocation. This will lead to run-time failure later when they
> are needed by another dlopened shared object which is shadow stack
> enabled.
>
> [BZ #24259]
> * elf/dl-open.c (dl_open_worker): Call _dl_open_check after
> relocation is finished.
> * sysdeps/x86/Makefile (tests): Add tst-cet-legacy-5a and
> tst-cet-legacy-5b.
> (modules-names): Add tst-cet-legacy-mod-5a and
> tst-cet-legacy-mod-5b.
> (CFLAGS-tst-cet-legacy-5a.c): New.
> (CFLAGS-tst-cet-legacy-5b.c): Likewise.
> (CFLAGS-tst-cet-legacy-mod-5a.c): Likewise.
> (CFLAGS-tst-cet-legacy-mod-5b.c): Likewise.
> ($(objpfx)tst-cet-legacy-5a): Likewise.
> ($(objpfx)tst-cet-legacy-5a.out): Likewise.
> ($(objpfx)tst-cet-legacy-mod-5a.so): Likewise.
> ($(objpfx)tst-cet-legacy-mod-5b.so): Likewise.
> ($(objpfx)tst-cet-legacy-5b): Likewise.
> ($(objpfx)tst-cet-legacy-5b.out): Likewise.
> (tst-cet-legacy-5b-ENV): Likewise.
> * sysdeps/x86/tst-cet-legacy-5.c: New file.
> * sysdeps/x86/tst-cet-legacy-5a.c: Likewise.
> * sysdeps/x86/tst-cet-legacy-5b.c: Likewise.
> * sysdeps/x86/tst-cet-legacy-mod-5.c: Likewise.
> * sysdeps/x86/tst-cet-legacy-mod-5a.c: Likewise.
> * sysdeps/x86/tst-cet-legacy-mod-5b.c: Likewise.
PING:
https://sourceware.org/ml/libc-alpha/2019-02/msg00579.html
* H. J. Lu:
> diff --git a/elf/dl-open.c b/elf/dl-open.c
> index 12a4f8b853..a62cb91975 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
> + 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;
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:
<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?
I can look into fixing the other bug, but I don't know how hard that's
going to be.
> 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.
> diff --git a/sysdeps/x86/tst-cet-legacy-mod-5.c b/sysdeps/x86/tst-cet-legacy-mod-5.c
> new file mode 100644
> index 0000000000..38d0aaa727
> --- /dev/null
> +++ b/sysdeps/x86/tst-cet-legacy-mod-5.c
> @@ -0,0 +1,91 @@
> + /* This test will fail on spurious wake-ups, which are allowed; however,
> + the current implementation shouldn't produce spurious wake-ups in the
> + scenario we are testing here. */
> + err = pthread_cond_wait (&cond, &mut);
> + if (err != 0)
> + error (EXIT_FAILURE, err, "parent: cannot wait fir signal");
You can use xpthread_barrier_wait et al., which doesn't have spurious
wake-ups. The existing wrappers should make the switch pretty easy.
Thanks,
Florian
On Mon, Mar 4, 2019 at 4:52 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > diff --git a/elf/dl-open.c b/elf/dl-open.c
> > index 12a4f8b853..a62cb91975 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
> > + 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;
>
> 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.
>
> > 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.
> > diff --git a/sysdeps/x86/tst-cet-legacy-mod-5.c b/sysdeps/x86/tst-cet-legacy-mod-5.c
> > new file mode 100644
> > index 0000000000..38d0aaa727
> > --- /dev/null
> > +++ b/sysdeps/x86/tst-cet-legacy-mod-5.c
> > @@ -0,0 +1,91 @@
>
> > + /* This test will fail on spurious wake-ups, which are allowed; however,
> > + the current implementation shouldn't produce spurious wake-ups in the
> > + scenario we are testing here. */
> > + err = pthread_cond_wait (&cond, &mut);
> > + if (err != 0)
> > + error (EXIT_FAILURE, err, "parent: cannot wait fir signal");
>
> You can use xpthread_barrier_wait et al., which doesn't have spurious
> wake-ups. The existing wrappers should make the switch pretty easy.
>
I will update.
Thanks.
* 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.
>> > 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.
Thanks,
Florian
@@ -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
+ 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;
@@ -19,13 +19,16 @@ ifeq ($(subdir),elf)
sysdep-dl-routines += dl-cet
tests += tst-cet-legacy-1 tst-cet-legacy-1a tst-cet-legacy-2 \
- tst-cet-legacy-2a tst-cet-legacy-3 tst-cet-legacy-4
+ tst-cet-legacy-2a tst-cet-legacy-3 tst-cet-legacy-4 \
+ tst-cet-legacy-5a
tst-cet-legacy-1a-ARGS = -- $(host-test-program-cmd)
ifneq (no,$(have-tunables))
-tests += tst-cet-legacy-4a tst-cet-legacy-4b tst-cet-legacy-4c
+tests += tst-cet-legacy-4a tst-cet-legacy-4b tst-cet-legacy-4c \
+ tst-cet-legacy-5b
endif
modules-names += tst-cet-legacy-mod-1 tst-cet-legacy-mod-2 \
- tst-cet-legacy-mod-4
+ tst-cet-legacy-mod-4 tst-cet-legacy-mod-5a \
+ tst-cet-legacy-mod-5b
CFLAGS-tst-cet-legacy-2.c += -fcf-protection=branch
CFLAGS-tst-cet-legacy-2a.c += -fcf-protection
@@ -36,6 +39,10 @@ CFLAGS-tst-cet-legacy-4.c += -fcf-protection=branch
CFLAGS-tst-cet-legacy-4a.c += -fcf-protection
CFLAGS-tst-cet-legacy-4b.c += -fcf-protection
CFLAGS-tst-cet-legacy-mod-4.c += -fcf-protection=none
+CFLAGS-tst-cet-legacy-5a.c += -fcf-protection
+CFLAGS-tst-cet-legacy-5b.c += -fcf-protection
+CFLAGS-tst-cet-legacy-mod-5a.c += -fcf-protection=none
+CFLAGS-tst-cet-legacy-mod-5b.c += -fcf-protection
$(objpfx)tst-cet-legacy-1: $(objpfx)tst-cet-legacy-mod-1.so \
$(objpfx)tst-cet-legacy-mod-2.so
@@ -47,6 +54,11 @@ $(objpfx)tst-cet-legacy-2a: $(objpfx)tst-cet-legacy-mod-2.so $(libdl)
$(objpfx)tst-cet-legacy-2a.out: $(objpfx)tst-cet-legacy-mod-1.so
$(objpfx)tst-cet-legacy-4: $(libdl)
$(objpfx)tst-cet-legacy-4.out: $(objpfx)tst-cet-legacy-mod-4.so
+$(objpfx)tst-cet-legacy-5a: $(libdl)
+$(objpfx)tst-cet-legacy-5a.out: $(objpfx)tst-cet-legacy-mod-5a.so \
+ $(objpfx)tst-cet-legacy-mod-5b.so
+$(objpfx)tst-cet-legacy-mod-5a.so: $(common-objpfx)nptl/libpthread.so
+$(objpfx)tst-cet-legacy-mod-5b.so: $(common-objpfx)nptl/libpthread.so
ifneq (no,$(have-tunables))
$(objpfx)tst-cet-legacy-4a: $(libdl)
$(objpfx)tst-cet-legacy-4a.out: $(objpfx)tst-cet-legacy-mod-4.so
@@ -57,6 +69,10 @@ tst-cet-legacy-4b-ENV = GLIBC_TUNABLES=glibc.cpu.x86_shstk=on
$(objpfx)tst-cet-legacy-4c: $(libdl)
$(objpfx)tst-cet-legacy-4c.out: $(objpfx)tst-cet-legacy-mod-4.so
tst-cet-legacy-4c-ENV = GLIBC_TUNABLES=glibc.cpu.x86_shstk=off
+$(objpfx)tst-cet-legacy-5b: $(libdl)
+$(objpfx)tst-cet-legacy-5b.out: $(objpfx)tst-cet-legacy-mod-5a.so \
+ $(objpfx)tst-cet-legacy-mod-5b.so
+tst-cet-legacy-5b-ENV = GLIBC_TUNABLES=glibc.cpu.hwcaps=-IBT,-SHSTK
endif
endif
new file mode 100644
@@ -0,0 +1,76 @@
+/* Check compatibility of CET-enabled executable with dlopened legacy
+ shared object.
+ Copyright (C) 2019 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include <dlfcn.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <string.h>
+
+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;
+ }
+
+ printf ("cannot open '%s': %s\n", modname, dlerror ());
+ exit (1);
+ }
+
+ fp = dlsym (h, "test");
+ if (fp == NULL)
+ {
+ printf ("cannot get symbol 'test': %s\n", dlerror ());
+ exit (1);
+ }
+
+ if (fp () != 0)
+ {
+ puts ("test () != 0");
+ exit (1);
+ }
+
+ dlclose (h);
+}
+
+static int
+do_test (void)
+{
+ do_test_1 ("tst-cet-legacy-mod-5a.so", true);
+ do_test_1 ("tst-cet-legacy-mod-5b.so", false);
+ return 0;
+}
+
+#include <support/test-driver.c>
new file mode 100644
@@ -0,0 +1 @@
+#include "tst-cet-legacy-5.c"
new file mode 100644
@@ -0,0 +1 @@
+#include "tst-cet-legacy-5.c"
new file mode 100644
@@ -0,0 +1,91 @@
+/* Check compatibility of CET-enabled executable with dlopened legacy
+ shared object.
+ Copyright (C) 2019 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include <error.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+static pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
+static pthread_mutex_t mut = PTHREAD_MUTEX_INITIALIZER;
+
+
+static void *
+tf (void *p)
+{
+ int err;
+
+ err = pthread_mutex_lock (&mut);
+ if (err != 0)
+ error (EXIT_FAILURE, err, "child: cannot get mutex");
+
+ puts ("child: got mutex; signalling");
+
+ pthread_cond_signal (&cond);
+
+ puts ("child: unlock");
+
+ err = pthread_mutex_unlock (&mut);
+ if (err != 0)
+ error (EXIT_FAILURE, err, "child: cannot unlock");
+
+ puts ("child: done");
+
+ return NULL;
+}
+
+int
+test (void)
+{
+ pthread_t th;
+ int err;
+
+ printf ("&cond = %p\n&mut = %p\n", &cond, &mut);
+
+ puts ("parent: get mutex");
+
+ err = pthread_mutex_lock (&mut);
+ if (err != 0)
+ error (EXIT_FAILURE, err, "parent: cannot get mutex");
+
+ puts ("parent: create child");
+
+ err = pthread_create (&th, NULL, tf, NULL);
+ if (err != 0)
+ error (EXIT_FAILURE, err, "parent: cannot create thread");
+
+ puts ("parent: wait for condition");
+
+ /* This test will fail on spurious wake-ups, which are allowed; however,
+ the current implementation shouldn't produce spurious wake-ups in the
+ scenario we are testing here. */
+ err = pthread_cond_wait (&cond, &mut);
+ if (err != 0)
+ error (EXIT_FAILURE, err, "parent: cannot wait fir signal");
+
+ puts ("parent: got signal");
+
+ err = pthread_join (th, NULL);
+ if (err != 0)
+ error (EXIT_FAILURE, err, "parent: failed to join");
+
+ puts ("done");
+
+ return 0;
+}
new file mode 100644
@@ -0,0 +1 @@
+#include "tst-cet-legacy-mod-5.c"
new file mode 100644
@@ -0,0 +1 @@
+#include "tst-cet-legacy-mod-5.c"