diff mbox series

V3 [PATCH] x86: Remove ARCH_CET_LEGACY_BITMAP [BZ #25397]

Message ID 20200310133654.GA244749@gmail.com
State Dropped
Headers show
Series V3 [PATCH] x86: Remove ARCH_CET_LEGACY_BITMAP [BZ #25397] | expand

Commit Message

H.J. Lu March 10, 2020, 1:36 p.m. UTC
On Tue, Mar 10, 2020 at 02:13:37PM +0100, Florian Weimer wrote:
> * H. J. Lu:
> 
> >> I think the error message should say *where* indirect branch tracking
> >> is not enabled.  I assume this is a property of the loaded object.
> >
> > Fixed.   Now I got
> >
> > .... libvirtd[1048]: internal error: Failed to load module
> > '/usr/lib64/libvirt/storage-backend/libvirt_storage_backend_rbd.so':
> > /usr/lib64/ceph/libceph-common.so.0: indirect branch tracking isn't
> > enabled: Invalid argument
> 
> What I meant is that the error message should say that the object
> needs to be rebuilt with IBT/SHSTK support.  In the above, it's
> unclear whether the process or the object has to enable IBT.
> 
> (The Invalid Argument part should also be suppressed.)

Like this?  The diff against V2 patch is:
diff mbox series

Patch

diff --git a/sysdeps/x86/dl-cet.c b/sysdeps/x86/dl-cet.c
index f527a1414c..b2843488be 100644
--- a/sysdeps/x86/dl-cet.c
+++ b/sysdeps/x86/dl-cet.c
@@ -142,10 +142,10 @@  dl_cet_check (struct link_map *m, const char *program)
 		  /* When IBT is enabled, we cannot dlopen a shared
 		     object without IBT.  */
 		  if (found_ibt_legacy)
-		    _dl_signal_error (EINVAL,
+		    _dl_signal_error (0,
 				      m->l_initfini[ibt_legacy]->l_name,
 				      "dlopen",
-				      N_("indirect branch tracking isn't enabled"));
+				      N_("rebuilt with IBT support needed"));
 		}
 
 	      if (enable_shstk_type != CET_PERMISSIVE)
@@ -153,10 +153,10 @@  dl_cet_check (struct link_map *m, const char *program)
 		  /* When SHSTK is enabled, we cannot dlopen a shared
 		     object without SHSTK.  */
 		  if (found_shstk_legacy)
-		    _dl_signal_error (EINVAL,
+		    _dl_signal_error (0,
 				      m->l_initfini[shstk_legacy]->l_name,
 				      "dlopen",
-				      N_("shadow stack isn't enabled"));
+				      N_("rebuilt with SHSTK support needed"));
 		}
 
 	      if (enable_ibt_type != CET_PERMISSIVE

H.J.
---
Since legacy bitmap doesn't cover jitted code generated by legacy JIT
engine, it isn't very useful.  This patch removes ARCH_CET_LEGACY_BITMAP
and treats indirect branch tracking similar to shadow stack by removing
legacy bitmap support.

	* sysdeps/unix/sysv/linux/x86/dl-cet.h
	(dl_cet_allocate_legacy_bitmap): Removed.
	* sysdeps/unix/sysv/linux/x86/include/asm/prctl.h
	(ARCH_CET_LEGACY_BITMAP): Removed.
	* sysdeps/x86/Makefile (tests): Add tst-cet-legacy-7.
	(CFLAGS-tst-cet-legacy-mod-5a.c): Changed to
	-fcf-protection=branch.
	(CFLAGS-tst-cet-legacy-mod-6a.c): Likewise.
	(CFLAGS-tst-cet-legacy-7.c): New.
	* sysdeps/x86/dl-cet.c (dl_cet_mark_legacy_region): Removed.
	(dl_cet_check): Remove legacy bitmap support.  Don't allow
	dlopening legacy shared library when IBT is enabled.  Set
	feature_1 if IBT or SHSTK is enabled in executable.
	* sysdeps/x86/dl-procruntime.c (_dl_x86_legacy_bitmap): Removed.
	* sysdeps/x86/tst-cet-legacy-4.c: Include <string.h> and
	<support/check.h>.
	(do_test): Check indirect branch tracking error.  Use
	FAIL_EXIT1.
	* sysdeps/x86/tst-cet-legacy-7.c: New file.
---
 sysdeps/unix/sysv/linux/x86/dl-cet.h          |  20 --
 .../unix/sysv/linux/x86/include/asm/prctl.h   |   5 -
 sysdeps/x86/Makefile                          |   7 +-
 sysdeps/x86/dl-cet.c                          | 217 +++++-------------
 sysdeps/x86/dl-procruntime.c                  |  11 -
 sysdeps/x86/tst-cet-legacy-4.c                |  19 +-
 sysdeps/x86/tst-cet-legacy-7.c                |  36 +++
 7 files changed, 109 insertions(+), 206 deletions(-)
 create mode 100644 sysdeps/x86/tst-cet-legacy-7.c

diff --git a/sysdeps/unix/sysv/linux/x86/dl-cet.h b/sysdeps/unix/sysv/linux/x86/dl-cet.h
index 9b2aaa238c..ae97a433a2 100644
--- a/sysdeps/unix/sysv/linux/x86/dl-cet.h
+++ b/sysdeps/unix/sysv/linux/x86/dl-cet.h
@@ -18,26 +18,6 @@ 
 #include <sys/prctl.h>
 #include <asm/prctl.h>
 
-static inline int __attribute__ ((always_inline))
-dl_cet_allocate_legacy_bitmap (unsigned long *legacy_bitmap)
-{
-  /* Allocate legacy bitmap.  */
-#ifdef __LP64__
-  return (int) INTERNAL_SYSCALL_CALL (arch_prctl,
-				      ARCH_CET_LEGACY_BITMAP, legacy_bitmap);
-#else
-  unsigned long long legacy_bitmap_u64[2];
-  int res = INTERNAL_SYSCALL_CALL (arch_prctl,
-				   ARCH_CET_LEGACY_BITMAP, legacy_bitmap_u64);
-  if (res == 0)
-    {
-      legacy_bitmap[0] = legacy_bitmap_u64[0];
-      legacy_bitmap[1] = legacy_bitmap_u64[1];
-    }
-  return res;
-#endif
-}
-
 static inline int __attribute__ ((always_inline))
 dl_cet_disable_cet (unsigned int cet_feature)
 {
diff --git a/sysdeps/unix/sysv/linux/x86/include/asm/prctl.h b/sysdeps/unix/sysv/linux/x86/include/asm/prctl.h
index f67f3299b9..45ad0b052f 100644
--- a/sysdeps/unix/sysv/linux/x86/include/asm/prctl.h
+++ b/sysdeps/unix/sysv/linux/x86/include/asm/prctl.h
@@ -24,9 +24,4 @@ 
      OUT: allocated shadow stack address: *addr.
  */
 # define ARCH_CET_ALLOC_SHSTK	0x3004
-/* Return legacy region bitmap info in unsigned long long *addr:
-     address: addr[0].
-     size: addr[1].
- */
-# define ARCH_CET_LEGACY_BITMAP	0x3005
 #endif /* ARCH_CET_STATUS */
diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile
index 95182a508c..ec96b59a78 100644
--- a/sysdeps/x86/Makefile
+++ b/sysdeps/x86/Makefile
@@ -20,7 +20,7 @@  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-5a tst-cet-legacy-6a
+	 tst-cet-legacy-5a tst-cet-legacy-6a tst-cet-legacy-7
 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 \
@@ -43,14 +43,15 @@  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-5a.c += -fcf-protection=branch
 CFLAGS-tst-cet-legacy-mod-5b.c += -fcf-protection
 CFLAGS-tst-cet-legacy-mod-5c.c += -fcf-protection
 CFLAGS-tst-cet-legacy-6a.c += -fcf-protection
 CFLAGS-tst-cet-legacy-6b.c += -fcf-protection
-CFLAGS-tst-cet-legacy-mod-6a.c += -fcf-protection=none
+CFLAGS-tst-cet-legacy-mod-6a.c += -fcf-protection=branch
 CFLAGS-tst-cet-legacy-mod-6b.c += -fcf-protection
 CFLAGS-tst-cet-legacy-mod-6c.c += -fcf-protection
+CFLAGS-tst-cet-legacy-7.c += -fcf-protection=none
 
 $(objpfx)tst-cet-legacy-1: $(objpfx)tst-cet-legacy-mod-1.so \
 		       $(objpfx)tst-cet-legacy-mod-2.so
diff --git a/sysdeps/x86/dl-cet.c b/sysdeps/x86/dl-cet.c
index ca3b5849bc..b2843488be 100644
--- a/sysdeps/x86/dl-cet.c
+++ b/sysdeps/x86/dl-cet.c
@@ -33,63 +33,6 @@ 
 # error GNU_PROPERTY_X86_FEATURE_1_SHSTK != X86_FEATURE_1_SHSTK
 #endif
 
-static int
-dl_cet_mark_legacy_region (struct link_map *l)
-{
-  /* Mark PT_LOAD segments with PF_X in legacy code page bitmap.  */
-  size_t i, phnum = l->l_phnum;
-  const ElfW(Phdr) *phdr = l->l_phdr;
-#ifdef __x86_64__
-  typedef unsigned long long word_t;
-#else
-  typedef unsigned long word_t;
-#endif
-  unsigned int bits_to_set;
-  word_t mask_to_set;
-#define BITS_PER_WORD (sizeof (word_t) * 8)
-#define BITMAP_FIRST_WORD_MASK(start) \
-  (~((word_t) 0) << ((start) & (BITS_PER_WORD - 1)))
-#define BITMAP_LAST_WORD_MASK(nbits) \
-  (~((word_t) 0) >> (-(nbits) & (BITS_PER_WORD - 1)))
-
-  word_t *bitmap = (word_t *) GL(dl_x86_legacy_bitmap)[0];
-  word_t bitmap_size = GL(dl_x86_legacy_bitmap)[1];
-  word_t *p;
-  size_t page_size = GLRO(dl_pagesize);
-
-  for (i = 0; i < phnum; i++)
-    if (phdr[i].p_type == PT_LOAD && (phdr[i].p_flags & PF_X))
-      {
-	/* One bit in legacy bitmap represents a page.  */
-	ElfW(Addr) start = (phdr[i].p_vaddr + l->l_addr) / page_size;
-	ElfW(Addr) len = (phdr[i].p_memsz + page_size - 1) / page_size;
-	ElfW(Addr) end = start + len;
-
-	if ((end / 8) > bitmap_size)
-	  return -EINVAL;
-
-	p = bitmap + (start / BITS_PER_WORD);
-	bits_to_set = BITS_PER_WORD - (start % BITS_PER_WORD);
-	mask_to_set = BITMAP_FIRST_WORD_MASK (start);
-
-	while (len >= bits_to_set)
-	  {
-	    *p |= mask_to_set;
-	    len -= bits_to_set;
-	    bits_to_set = BITS_PER_WORD;
-	    mask_to_set = ~((word_t) 0);
-	    p++;
-	  }
-	if (len)
-	  {
-	    mask_to_set &= BITMAP_LAST_WORD_MASK (end);
-	    *p |= mask_to_set;
-	  }
-      }
-
-  return 0;
-}
-
 /* Check if object M is compatible with CET.  */
 
 static void
@@ -117,6 +60,8 @@  dl_cet_check (struct link_map *m, const char *program)
   if (ibt_enabled || shstk_enabled)
     {
       struct link_map *l = NULL;
+      unsigned int ibt_legacy = 0, shstk_legacy = 0;
+      bool found_ibt_legacy = false, found_shstk_legacy = false;
 
       /* Check if IBT and SHSTK are enabled in object.  */
       bool enable_ibt = (ibt_enabled
@@ -142,10 +87,7 @@  dl_cet_check (struct link_map *m, const char *program)
 	 support IBT nor SHSTK.  */
       if (enable_ibt || enable_shstk)
 	{
-	  int res;
 	  unsigned int i;
-	  unsigned int first_legacy, last_legacy;
-	  bool need_legacy_bitmap = false;
 
 	  i = m->l_searchlist.r_nlist;
 	  while (i-- > 0)
@@ -167,91 +109,25 @@  dl_cet_check (struct link_map *m, const char *program)
 		continue;
 #endif
 
-	      if (enable_ibt
-		  && enable_ibt_type != CET_ALWAYS_ON
-		  && !(l->l_cet & lc_ibt))
+	      /* IBT is enabled only if it is enabled in executable as
+		 well as all shared objects.  */
+	      enable_ibt &= (enable_ibt_type == CET_ALWAYS_ON
+			     || (l->l_cet & lc_ibt) != 0);
+	      if (!found_ibt_legacy && enable_ibt != ibt_enabled)
 		{
-		  /* Remember the first and last legacy objects.  */
-		  if (!need_legacy_bitmap)
-		    last_legacy = i;
-		  first_legacy = i;
-		  need_legacy_bitmap = true;
+		  found_ibt_legacy = true;
+		  ibt_legacy = i;
 		}
 
 	      /* SHSTK is enabled only if it is enabled in executable as
 		 well as all shared objects.  */
 	      enable_shstk &= (enable_shstk_type == CET_ALWAYS_ON
 			       || (l->l_cet & lc_shstk) != 0);
-	    }
-
-	  if (need_legacy_bitmap)
-	    {
-	      if (GL(dl_x86_legacy_bitmap)[0])
-		{
-		  /* Change legacy bitmap to writable.  */
-		  if (__mprotect ((void *) GL(dl_x86_legacy_bitmap)[0],
-				  GL(dl_x86_legacy_bitmap)[1],
-				  PROT_READ | PROT_WRITE) < 0)
-		    {
-mprotect_failure:
-		      if (program)
-			_dl_fatal_printf ("%s: mprotect legacy bitmap failed\n",
-					  l->l_name);
-		      else
-			_dl_signal_error (EINVAL, l->l_name, "dlopen",
-					  N_("mprotect legacy bitmap failed"));
-		    }
-		}
-	      else
+	      if (enable_shstk != shstk_enabled)
 		{
-		  /* Allocate legacy bitmap.  */
-		  int res = dl_cet_allocate_legacy_bitmap
-		    (GL(dl_x86_legacy_bitmap));
-		  if (res != 0)
-		    {
-		      if (program)
-			_dl_fatal_printf ("%s: legacy bitmap isn't available\n",
-					  l->l_name);
-		      else
-			_dl_signal_error (EINVAL, l->l_name, "dlopen",
-					  N_("legacy bitmap isn't available"));
-		    }
+		  found_shstk_legacy = true;
+		  shstk_legacy = i;
 		}
-
-	      /* Put legacy shared objects in legacy bitmap.  */
-	      for (i = first_legacy; i <= last_legacy; i++)
-		{
-		  l = m->l_initfini[i];
-
-		  if (l->l_init_called || (l->l_cet & lc_ibt))
-		    continue;
-
-#ifdef SHARED
-		  if (l == &GL(dl_rtld_map)
-		      ||  l->l_real == &GL(dl_rtld_map)
-		      || (program && l == m))
-		    continue;
-#endif
-
-		  /* If IBT is enabled in executable and IBT isn't enabled
-		     in this shard object, mark PT_LOAD segments with PF_X
-		     in legacy code page bitmap.  */
-		  res = dl_cet_mark_legacy_region (l);
-		  if (res != 0)
-		    {
-		      if (program)
-			_dl_fatal_printf ("%s: failed to mark legacy code region\n",
-					  l->l_name);
-		      else
-			_dl_signal_error (-res, l->l_name, "dlopen",
-					  N_("failed to mark legacy code region"));
-		    }
-		}
-
-	      /* Change legacy bitmap to read-only.  */
-	      if (__mprotect ((void *) GL(dl_x86_legacy_bitmap)[0],
-			      GL(dl_x86_legacy_bitmap)[1], PROT_READ) < 0)
-		goto mprotect_failure;
 	    }
 	}
 
@@ -259,23 +135,40 @@  mprotect_failure:
 
       if (enable_ibt != ibt_enabled || enable_shstk != shstk_enabled)
 	{
-	  if (!program
-	      && enable_shstk_type != CET_PERMISSIVE)
+	  if (!program)
 	    {
-	      /* When SHSTK is enabled, we can't dlopening a shared
-		 object without SHSTK.  */
-	      if (enable_shstk != shstk_enabled)
-		_dl_signal_error (EINVAL, l->l_name, "dlopen",
-				  N_("shadow stack isn't enabled"));
-	      return;
+	      if (enable_ibt_type != CET_PERMISSIVE)
+		{
+		  /* When IBT is enabled, we cannot dlopen a shared
+		     object without IBT.  */
+		  if (found_ibt_legacy)
+		    _dl_signal_error (0,
+				      m->l_initfini[ibt_legacy]->l_name,
+				      "dlopen",
+				      N_("rebuilt with IBT support needed"));
+		}
+
+	      if (enable_shstk_type != CET_PERMISSIVE)
+		{
+		  /* When SHSTK is enabled, we cannot dlopen a shared
+		     object without SHSTK.  */
+		  if (found_shstk_legacy)
+		    _dl_signal_error (0,
+				      m->l_initfini[shstk_legacy]->l_name,
+				      "dlopen",
+				      N_("rebuilt with SHSTK support needed"));
+		}
+
+	      if (enable_ibt_type != CET_PERMISSIVE
+		  && enable_shstk_type != CET_PERMISSIVE)
+		return;
 	    }
 
 	  /* Disable IBT and/or SHSTK if they are enabled by kernel, but
 	     disabled in executable or shared objects.  */
 	  unsigned int cet_feature = 0;
 
-	  /* Disable IBT only during program startup.  */
-	  if (program && !enable_ibt)
+	  if (!enable_ibt)
 	    cet_feature |= GNU_PROPERTY_X86_FEATURE_1_IBT;
 	  if (!enable_shstk)
 	    cet_feature |= GNU_PROPERTY_X86_FEATURE_1_SHSTK;
@@ -286,8 +179,14 @@  mprotect_failure:
 	      if (program)
 		_dl_fatal_printf ("%s: can't disable CET\n", program);
 	      else
-		_dl_signal_error (-res, l->l_name, "dlopen",
-				  N_("can't disable CET"));
+		{
+		  if (found_ibt_legacy)
+		    l = m->l_initfini[ibt_legacy];
+		  else
+		    l = m->l_initfini[shstk_legacy];
+		  _dl_signal_error (-res, l->l_name, "dlopen",
+				    N_("can't disable CET"));
+		}
 	    }
 
 	  /* Clear the disabled bits in dl_x86_feature_1.  */
@@ -297,17 +196,21 @@  mprotect_failure:
 	}
 
 #ifdef SHARED
-      if (program
-	  && (!shstk_enabled
-	      || enable_shstk_type != CET_PERMISSIVE)
-	  && (ibt_enabled || shstk_enabled))
+      if (program && (ibt_enabled || shstk_enabled))
 	{
-	  /* Lock CET if IBT or SHSTK is enabled in executable.  Don't
-	     lock CET if SHSTK is enabled permissively.  */
-	  int res = dl_cet_lock_cet ();
-	  if (res != 0)
-	    _dl_fatal_printf ("%s: can't lock CET\n", program);
+	  if ((!ibt_enabled
+	       || enable_ibt_type != CET_PERMISSIVE)
+	      && (!shstk_enabled
+		  || enable_shstk_type != CET_PERMISSIVE))
+	    {
+	      /* Lock CET if IBT or SHSTK is enabled in executable unless
+	         IBT or SHSTK is enabled permissively.  */
+	      int res = dl_cet_lock_cet ();
+	      if (res != 0)
+		_dl_fatal_printf ("%s: can't lock CET\n", program);
+	    }
 
+	  /* Set feature_1 if IBT or SHSTK is enabled in executable.  */
 	  cet_feature_changed = true;
 	}
 #endif
diff --git a/sysdeps/x86/dl-procruntime.c b/sysdeps/x86/dl-procruntime.c
index fb36801f3e..5e39a38133 100644
--- a/sysdeps/x86/dl-procruntime.c
+++ b/sysdeps/x86/dl-procruntime.c
@@ -54,15 +54,4 @@  PROCINFO_CLASS unsigned int _dl_x86_feature_1[2]
 # else
 ,
 # endif
-
-# if !defined PROCINFO_DECL && defined SHARED
-  ._dl_x86_legacy_bitmap
-# else
-PROCINFO_CLASS unsigned long _dl_x86_legacy_bitmap[2]
-# endif
-# if !defined SHARED || defined PROCINFO_DECL
-;
-# else
-,
-# endif
 #endif
diff --git a/sysdeps/x86/tst-cet-legacy-4.c b/sysdeps/x86/tst-cet-legacy-4.c
index a77078afc9..da1f3d1e0d 100644
--- a/sysdeps/x86/tst-cet-legacy-4.c
+++ b/sysdeps/x86/tst-cet-legacy-4.c
@@ -20,6 +20,9 @@ 
 #include <dlfcn.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>
+
+#include <support/check.h>
 
 static int
 do_test (void)
@@ -31,22 +34,18 @@  do_test (void)
   h = dlopen (modname, RTLD_LAZY);
   if (h == NULL)
     {
-      printf ("cannot open '%s': %s\n", modname, dlerror ());
-      exit (1);
+      const char *err = dlerror ();
+      if (!strstr (err, "indirect branch tracking isn't enabled"))
+	FAIL_EXIT1 ("incorrect dlopen '%s' error: %s\n", modname, err);
+      return 0;
     }
 
   fp = dlsym (h, "test");
   if (fp == NULL)
-    {
-      printf ("cannot get symbol 'test': %s\n", dlerror ());
-      exit (1);
-    }
+    FAIL_EXIT1 ("cannot get symbol 'test': %s\n", dlerror ());
 
   if (fp () != 0)
-    {
-      puts ("test () != 0");
-      exit (1);
-    }
+    FAIL_EXIT1 ("test () != 0");
 
   dlclose (h);
 
diff --git a/sysdeps/x86/tst-cet-legacy-7.c b/sysdeps/x86/tst-cet-legacy-7.c
new file mode 100644
index 0000000000..cf4c2779ce
--- /dev/null
+++ b/sysdeps/x86/tst-cet-legacy-7.c
@@ -0,0 +1,36 @@ 
+/* Check compatibility of legacy executable with a JIT engine.
+   Copyright (C) 2020 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
+   <https://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+#include <sys/mman.h>
+#include <support/xunistd.h>
+
+static int
+do_test (void)
+{
+  void (*funcp) (void);
+  funcp = xmmap (NULL, 0x1000, PROT_EXEC | PROT_READ | PROT_WRITE,
+		 MAP_ANONYMOUS | MAP_PRIVATE, -1);
+  printf ("mmap = %p\n", funcp);
+  /* Write RET instruction.  */
+  *(char *) funcp = 0xc3;
+  funcp ();
+  return 0;
+}
+
+#include <support/test-driver.c>