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

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

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.

-- 
H.J.

From bf2b26ea60c8131ad97a2606a112c1b9231f79b4 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Sat, 23 Feb 2019 10:43:17 -0800
Subject: [PATCH] Call _dl_open_check after relocation is finished [BZ #24259]

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      | 66 +++++++++++++++++++++
 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, 185 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

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;
diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile
index 7ec46ca100..308bba80e5 100644
--- a/sysdeps/x86/Makefile
+++ b/sysdeps/x86/Makefile
@@ -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
 
diff --git a/sysdeps/x86/tst-cet-legacy-5.c b/sysdeps/x86/tst-cet-legacy-5.c
new file mode 100644
index 0000000000..48bf997e38
--- /dev/null
+++ b/sysdeps/x86/tst-cet-legacy-5.c
@@ -0,0 +1,66 @@
+/* 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>
+#include <support/check.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)
+	    FAIL_EXIT1 ("incorrect dlopen '%s' error: %s\n", modname,
+			dlerror ());
+	  return;
+	}
+
+      FAIL_EXIT1 ("cannot open '%s': %s\n", modname, dlerror ());
+    }
+
+  fp = dlsym (h, "test");
+  if (fp == NULL)
+    FAIL_EXIT1 ("cannot get symbol 'test': %s\n", dlerror ());
+
+  if (fp () != 0)
+    FAIL_EXIT1 ("test () != 0");
+
+  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>
diff --git a/sysdeps/x86/tst-cet-legacy-5a.c b/sysdeps/x86/tst-cet-legacy-5a.c
new file mode 100644
index 0000000000..fc5a609dff
--- /dev/null
+++ b/sysdeps/x86/tst-cet-legacy-5a.c
@@ -0,0 +1 @@
+#include "tst-cet-legacy-5.c"
diff --git a/sysdeps/x86/tst-cet-legacy-5b.c b/sysdeps/x86/tst-cet-legacy-5b.c
new file mode 100644
index 0000000000..fc5a609dff
--- /dev/null
+++ b/sysdeps/x86/tst-cet-legacy-5b.c
@@ -0,0 +1 @@
+#include "tst-cet-legacy-5.c"
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 @@
+/* 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;
+}
diff --git a/sysdeps/x86/tst-cet-legacy-mod-5a.c b/sysdeps/x86/tst-cet-legacy-mod-5a.c
new file mode 100644
index 0000000000..daa43e4e8d
--- /dev/null
+++ b/sysdeps/x86/tst-cet-legacy-mod-5a.c
@@ -0,0 +1 @@
+#include "tst-cet-legacy-mod-5.c"
diff --git a/sysdeps/x86/tst-cet-legacy-mod-5b.c b/sysdeps/x86/tst-cet-legacy-mod-5b.c
new file mode 100644
index 0000000000..daa43e4e8d
--- /dev/null
+++ b/sysdeps/x86/tst-cet-legacy-mod-5b.c
@@ -0,0 +1 @@
+#include "tst-cet-legacy-mod-5.c"
-- 
2.20.1