diff mbox series

x86: Move CET control to _dl_x86_feature_control [BZ #25887]

Message ID CAMe9rOo7vBxcj=veaHybF07pZZdeFEpiNXn7Uh-PYaMmu4yMfg@mail.gmail.com
State Superseded
Headers
Series x86: Move CET control to _dl_x86_feature_control [BZ #25887] |

Commit Message

H.J. Lu May 16, 2020, 11:44 p.m. UTC 
  On Sat, May 16, 2020 at 10:27 AM Florian Weimer <fw@deneb.enyo.de> wrote:
>
> * H. J. Lu via Libc-alpha:
>
> > diff --git a/sysdeps/x86/cet-tunables.h b/sysdeps/x86/cet-tunables.h
> > index 5e1e42df10..0088b89d3e 100644
> > --- a/sysdeps/x86/cet-tunables.h
> > +++ b/sysdeps/x86/cet-tunables.h
> > @@ -16,14 +16,32 @@
> >     License along with the GNU C Library; if not, see
> >     <https://www.gnu.org/licenses/>.  */
> >
> > -/* Valid control values:
> > +#ifndef _CET_TUNABLES_H
> > +#define _CET_TUNABLES_H
> > +
> > +/* For each CET feature, IBT and SHSTK, valid control values:
> >     0: Enable CET features based on ELF property note.
> >     1: Always disable CET features.
> >     2: Always enable CET features.
> >     3: Enable CET features permissively.
> > +
> > +   Bits 0-1: IBT
> > +   Bits 2-3: SHSTK
> >   */
> >  #define CET_ELF_PROPERTY     0
> >  #define CET_ALWAYS_OFF               1
> >  #define CET_ALWAYS_ON                2
> >  #define CET_PERMISSIVE               3
> > -#define CET_MAX                      CET_PERMISSIVE
> > +#define CET_CONTROL_MASK     3
> > +#define CET_IBT_SHIFT                0
> > +#define CET_SHSTK_SHIFT              2
> > +
> > +/* Get CET control value.  */
> > +
> > +static inline unsigned int
> > +get_cet_control_value (unsigned int shift)
> > +{
> > +  return (GL(dl_x86_feature_1)[1] >> shift) & CET_CONTROL_MASK;
> > +}
> > +
> > +#endif /* cet-tunables.h */
>
> Is there a reason why this has to be a single bitmask?  Maybe a
> bitfield would better document the intent?

Here is the updated patch to use bitfields.

OK for master branch?

Comments

Florian Weimer May 18, 2020, 7:19 a.m. UTC | #1 
* H. J. Lu via Libc-alpha:

> Here is the updated patch to use bitfields.

Thanks, I like this better.

> diff --git a/sysdeps/x86/cet-tunables.h b/sysdeps/x86/cet-control.h
> similarity index 61%
> rename from sysdeps/x86/cet-tunables.h
> rename to sysdeps/x86/cet-control.h
> index 5e1e42df10..3a314f9609 100644
> --- a/sysdeps/x86/cet-tunables.h
> +++ b/sysdeps/x86/cet-control.h
> @@ -16,14 +16,26 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -/* Valid control values:
> -   0: Enable CET features based on ELF property note.
> -   1: Always disable CET features.
> -   2: Always enable CET features.
> -   3: Enable CET features permissively.
> - */
> -#define CET_ELF_PROPERTY	0
> -#define CET_ALWAYS_OFF		1
> -#define CET_ALWAYS_ON		2
> -#define CET_PERMISSIVE		3
> -#define CET_MAX			CET_PERMISSIVE
> +#ifndef _CET_CONTROL_H
> +#define _CET_CONTROL_H
> +
> +/* For each CET feature, IBT and SHSTK, valid control values  */

Missing ”.” at end of comment.

> +enum dl_x86_cet_control
> +{
> +  /* Enable CET features based on ELF property note.  */
> +  elf_property = 0,
> +  /* Always enable CET features.  */
> +  always_on,
> +  /* Always disable CET features. */
> +  always_off,
> +  /* Enable CET features permissively. */

Missing double space after “.”.

> +  permissive
> +};

Given enum constantse are not not scoped and this is a widely-included
header, I think you should include at least a “cet_” prefix in these
enum constants.

It's still not clear to me why CET control variables have to be in
_rtld_global.  Regular global variables would likely lead to clearer
code, I think.  _rtld_global is needed for read-write data that is
shared with libc/libdl, and this does not seem to apply to CET control
settings.

Thanks,
Florian
diff mbox series

Patch

From cd9ccac3dbcb286de61f4123ead91bf893321c13 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Tue, 28 Apr 2020 07:46:17 -0700
Subject: [PATCH] x86: Move CET control to _dl_x86_feature_control [BZ #25887]

1. Change _dl_x86_feature_1[2] to _dl_x86_feature_1.
2. Add _dl_x86_feature_control after _dl_x86_feature_1, which is a
struct of 2 bitfields for IBT and SHSTK control

This fixes [BZ #25887].
---
 sysdeps/i386/dl-machine.h                     |  2 +-
 sysdeps/unix/sysv/linux/x86/cpu-features.c    |  2 +-
 sysdeps/x86/{cet-tunables.h => cet-control.h} | 34 ++++++++++-----
 sysdeps/x86/cpu-features.c                    | 12 +++---
 sysdeps/x86/cpu-tunables.c                    | 31 +++-----------
 sysdeps/x86/dl-cet.c                          | 42 +++++++++----------
 sysdeps/x86/dl-procruntime.c                  | 19 ++++++++-
 sysdeps/x86/ldsodefs.h                        |  1 +
 8 files changed, 74 insertions(+), 69 deletions(-)
 rename sysdeps/x86/{cet-tunables.h => cet-control.h} (61%)

diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
index 8af0789a9c..0f08079e48 100644
--- a/sysdeps/i386/dl-machine.h
+++ b/sysdeps/i386/dl-machine.h
@@ -71,7 +71,7 @@  elf_machine_runtime_setup (struct link_map *l, int lazy, int profile)
   extern void _dl_runtime_profile_shstk (Elf32_Word) attribute_hidden;
   /* Check if SHSTK is enabled by kernel.  */
   bool shstk_enabled
-    = (GL(dl_x86_feature_1)[0] & GNU_PROPERTY_X86_FEATURE_1_SHSTK) != 0;
+    = (GL(dl_x86_feature_1) & GNU_PROPERTY_X86_FEATURE_1_SHSTK) != 0;
 
   if (l->l_info[DT_JMPREL] && lazy)
     {
diff --git a/sysdeps/unix/sysv/linux/x86/cpu-features.c b/sysdeps/unix/sysv/linux/x86/cpu-features.c
index d67b300595..fc0e32827d 100644
--- a/sysdeps/unix/sysv/linux/x86/cpu-features.c
+++ b/sysdeps/unix/sysv/linux/x86/cpu-features.c
@@ -34,7 +34,7 @@  static inline void
 x86_setup_tls (void)
 {
   __libc_setup_tls ();
-  THREAD_SETMEM (THREAD_SELF, header.feature_1, GL(dl_x86_feature_1)[0]);
+  THREAD_SETMEM (THREAD_SELF, header.feature_1, GL(dl_x86_feature_1));
 }
 
 #  define ARCH_SETUP_TLS() x86_setup_tls ()
diff --git a/sysdeps/x86/cet-tunables.h b/sysdeps/x86/cet-control.h
similarity index 61%
rename from sysdeps/x86/cet-tunables.h
rename to sysdeps/x86/cet-control.h
index 5e1e42df10..3a314f9609 100644
--- a/sysdeps/x86/cet-tunables.h
+++ b/sysdeps/x86/cet-control.h
@@ -16,14 +16,26 @@ 
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-/* Valid control values:
-   0: Enable CET features based on ELF property note.
-   1: Always disable CET features.
-   2: Always enable CET features.
-   3: Enable CET features permissively.
- */
-#define CET_ELF_PROPERTY	0
-#define CET_ALWAYS_OFF		1
-#define CET_ALWAYS_ON		2
-#define CET_PERMISSIVE		3
-#define CET_MAX			CET_PERMISSIVE
+#ifndef _CET_CONTROL_H
+#define _CET_CONTROL_H
+
+/* For each CET feature, IBT and SHSTK, valid control values  */
+enum dl_x86_cet_control
+{
+  /* Enable CET features based on ELF property note.  */
+  elf_property = 0,
+  /* Always enable CET features.  */
+  always_on,
+  /* Always disable CET features. */
+  always_off,
+  /* Enable CET features permissively. */
+  permissive
+};
+
+struct dl_x86_feature_control
+{
+  enum dl_x86_cet_control ibt : 2;
+  enum dl_x86_cet_control shstk : 2;
+};
+
+#endif /* cet-control.h */
diff --git a/sysdeps/x86/cpu-features.c b/sysdeps/x86/cpu-features.c
index bfb415f05a..cf1d84f2af 100644
--- a/sysdeps/x86/cpu-features.c
+++ b/sysdeps/x86/cpu-features.c
@@ -39,7 +39,6 @@  extern void TUNABLE_CALLBACK (set_x86_shstk) (tunable_val_t *)
 
 #if CET_ENABLED
 # include <dl-cet.h>
-# include <cet-tunables.h>
 #endif
 
 static void
@@ -620,7 +619,7 @@  no_cpuid:
 
   if (cet_status)
     {
-      GL(dl_x86_feature_1)[0] = cet_status;
+      GL(dl_x86_feature_1) = cet_status;
 
 # ifndef SHARED
       /* Check if IBT and SHSTK are enabled by kernel.  */
@@ -644,14 +643,13 @@  no_cpuid:
 
 	      /* Clear the disabled bits in dl_x86_feature_1.  */
 	      if (res == 0)
-		GL(dl_x86_feature_1)[0] &= ~cet_feature;
+		GL(dl_x86_feature_1) &= ~cet_feature;
 	    }
 
 	  /* Lock CET if IBT or SHSTK is enabled in executable.  Don't
-	     lock CET if SHSTK is enabled permissively.  */
-	  if (((GL(dl_x86_feature_1)[1] >> CET_MAX)
-	       & ((1 << CET_MAX) - 1))
-	       != CET_PERMISSIVE)
+	     lock CET if IBT or SHSTK is enabled permissively.  */
+	  if (GL(dl_x86_feature_control).ibt != permissive
+	      && GL(dl_x86_feature_control).shstk != permissive)
 	    dl_cet_lock_cet ();
 	}
 # endif
diff --git a/sysdeps/x86/cpu-tunables.c b/sysdeps/x86/cpu-tunables.c
index 861bd7bcaa..58abc1ecfc 100644
--- a/sysdeps/x86/cpu-tunables.c
+++ b/sysdeps/x86/cpu-tunables.c
@@ -336,28 +336,18 @@  TUNABLE_CALLBACK (set_hwcaps) (tunable_val_t *valp)
 }
 
 # if CET_ENABLED
-#  include <cet-tunables.h>
 
 attribute_hidden
 void
 TUNABLE_CALLBACK (set_x86_ibt) (tunable_val_t *valp)
 {
   if (DEFAULT_MEMCMP (valp->strval, "on", sizeof ("on")) == 0)
-    {
-      GL(dl_x86_feature_1)[1] &= ~((1 << CET_MAX) - 1);
-      GL(dl_x86_feature_1)[1] |= CET_ALWAYS_ON;
-    }
+    GL(dl_x86_feature_control).ibt = always_on;
   else if (DEFAULT_MEMCMP (valp->strval, "off", sizeof ("off")) == 0)
-    {
-      GL(dl_x86_feature_1)[1] &= ~((1 << CET_MAX) - 1);
-      GL(dl_x86_feature_1)[1] |= CET_ALWAYS_OFF;
-    }
+    GL(dl_x86_feature_control).ibt = always_off;
   else if (DEFAULT_MEMCMP (valp->strval, "permissive",
 			   sizeof ("permissive")) == 0)
-    {
-      GL(dl_x86_feature_1)[1] &= ~((1 << CET_MAX) - 1);
-      GL(dl_x86_feature_1)[1] |= CET_PERMISSIVE;
-    }
+    GL(dl_x86_feature_control).ibt = permissive;
 }
 
 attribute_hidden
@@ -365,21 +355,12 @@  void
 TUNABLE_CALLBACK (set_x86_shstk) (tunable_val_t *valp)
 {
   if (DEFAULT_MEMCMP (valp->strval, "on", sizeof ("on")) == 0)
-    {
-      GL(dl_x86_feature_1)[1] &= ~(((1 << CET_MAX) - 1) << CET_MAX);
-      GL(dl_x86_feature_1)[1] |= (CET_ALWAYS_ON << CET_MAX);
-    }
+    GL(dl_x86_feature_control).shstk = always_on;
   else if (DEFAULT_MEMCMP (valp->strval, "off", sizeof ("off")) == 0)
-    {
-      GL(dl_x86_feature_1)[1] &= ~(((1 << CET_MAX) - 1) << CET_MAX);
-      GL(dl_x86_feature_1)[1] |= (CET_ALWAYS_OFF << CET_MAX);
-    }
+    GL(dl_x86_feature_control).shstk = always_off;
   else if (DEFAULT_MEMCMP (valp->strval, "permissive",
 			   sizeof ("permissive")) == 0)
-    {
-      GL(dl_x86_feature_1)[1] &= ~(((1 << CET_MAX) - 1) << CET_MAX);
-      GL(dl_x86_feature_1)[1] |= (CET_PERMISSIVE << CET_MAX);
-    }
+    GL(dl_x86_feature_control).shstk = permissive;
 }
 # endif
 #endif
diff --git a/sysdeps/x86/dl-cet.c b/sysdeps/x86/dl-cet.c
index c7029f1b51..82880c1999 100644
--- a/sysdeps/x86/dl-cet.c
+++ b/sysdeps/x86/dl-cet.c
@@ -20,7 +20,6 @@ 
 #include <libintl.h>
 #include <ldsodefs.h>
 #include <dl-cet.h>
-#include <cet-tunables.h>
 
 /* GNU_PROPERTY_X86_FEATURE_1_IBT and GNU_PROPERTY_X86_FEATURE_1_SHSTK
    are defined in <elf.h>, which are only available for C sources.
@@ -39,23 +38,22 @@  static void
 dl_cet_check (struct link_map *m, const char *program)
 {
   /* Check how IBT should be enabled.  */
-  unsigned int enable_ibt_type
-    = GL(dl_x86_feature_1)[1] & ((1 << CET_MAX) - 1);
+  enum dl_x86_cet_control enable_ibt_type
+    = GL(dl_x86_feature_control).ibt;
   /* Check how SHSTK should be enabled.  */
-  unsigned int enable_shstk_type
-    = ((GL(dl_x86_feature_1)[1] >> CET_MAX) & ((1 << CET_MAX) - 1));
+  enum dl_x86_cet_control enable_shstk_type
+    = GL(dl_x86_feature_control).shstk;
 
   /* No legacy object check if both IBT and SHSTK are always on.  */
-  if (enable_ibt_type == CET_ALWAYS_ON
-      && enable_shstk_type == CET_ALWAYS_ON)
+  if (enable_ibt_type == always_on && enable_shstk_type == always_on)
     return;
 
   /* Check if IBT is enabled by kernel.  */
   bool ibt_enabled
-    = (GL(dl_x86_feature_1)[0] & GNU_PROPERTY_X86_FEATURE_1_IBT) != 0;
+    = (GL(dl_x86_feature_1) & GNU_PROPERTY_X86_FEATURE_1_IBT) != 0;
   /* Check if SHSTK is enabled by kernel.  */
   bool shstk_enabled
-    = (GL(dl_x86_feature_1)[0] & GNU_PROPERTY_X86_FEATURE_1_SHSTK) != 0;
+    = (GL(dl_x86_feature_1) & GNU_PROPERTY_X86_FEATURE_1_SHSTK) != 0;
 
   if (ibt_enabled || shstk_enabled)
     {
@@ -65,9 +63,9 @@  dl_cet_check (struct link_map *m, const char *program)
 
       /* Check if IBT and SHSTK are enabled in object.  */
       bool enable_ibt = (ibt_enabled
-			 && enable_ibt_type != CET_ALWAYS_OFF);
+			 && enable_ibt_type != always_off);
       bool enable_shstk = (shstk_enabled
-			   && enable_shstk_type != CET_ALWAYS_OFF);
+			   && enable_shstk_type != always_off);
       if (program)
 	{
 	  /* Enable IBT and SHSTK only if they are enabled in executable.
@@ -76,10 +74,10 @@  dl_cet_check (struct link_map *m, const char *program)
 	     GLIBC_TUNABLES=glibc.cpu.hwcaps=-IBT,-SHSTK
 	   */
 	  enable_ibt &= (HAS_CPU_FEATURE (IBT)
-			 && (enable_ibt_type == CET_ALWAYS_ON
+			 && (enable_ibt_type == always_on
 			     || (m->l_cet & lc_ibt) != 0));
 	  enable_shstk &= (HAS_CPU_FEATURE (SHSTK)
-			   && (enable_shstk_type == CET_ALWAYS_ON
+			   && (enable_shstk_type == always_on
 			       || (m->l_cet & lc_shstk) != 0));
 	}
 
@@ -111,7 +109,7 @@  dl_cet_check (struct link_map *m, const char *program)
 
 	      /* IBT is enabled only if it is enabled in executable as
 		 well as all shared objects.  */
-	      enable_ibt &= (enable_ibt_type == CET_ALWAYS_ON
+	      enable_ibt &= (enable_ibt_type == always_on
 			     || (l->l_cet & lc_ibt) != 0);
 	      if (!found_ibt_legacy && enable_ibt != ibt_enabled)
 		{
@@ -121,7 +119,7 @@  dl_cet_check (struct link_map *m, const char *program)
 
 	      /* SHSTK is enabled only if it is enabled in executable as
 		 well as all shared objects.  */
-	      enable_shstk &= (enable_shstk_type == CET_ALWAYS_ON
+	      enable_shstk &= (enable_shstk_type == always_on
 			       || (l->l_cet & lc_shstk) != 0);
 	      if (enable_shstk != shstk_enabled)
 		{
@@ -137,7 +135,7 @@  dl_cet_check (struct link_map *m, const char *program)
 	{
 	  if (!program)
 	    {
-	      if (enable_ibt_type != CET_PERMISSIVE)
+	      if (enable_ibt_type != permissive)
 		{
 		  /* When IBT is enabled, we cannot dlopen a shared
 		     object without IBT.  */
@@ -148,7 +146,7 @@  dl_cet_check (struct link_map *m, const char *program)
 				      N_("rebuild shared object with IBT support enabled"));
 		}
 
-	      if (enable_shstk_type != CET_PERMISSIVE)
+	      if (enable_shstk_type != permissive)
 		{
 		  /* When SHSTK is enabled, we cannot dlopen a shared
 		     object without SHSTK.  */
@@ -159,8 +157,8 @@  dl_cet_check (struct link_map *m, const char *program)
 				      N_("rebuild shared object with SHSTK support enabled"));
 		}
 
-	      if (enable_ibt_type != CET_PERMISSIVE
-		  && enable_shstk_type != CET_PERMISSIVE)
+	      if (enable_ibt_type != permissive
+		  && enable_shstk_type != permissive)
 		return;
 	    }
 
@@ -190,7 +188,7 @@  dl_cet_check (struct link_map *m, const char *program)
 	    }
 
 	  /* Clear the disabled bits in dl_x86_feature_1.  */
-	  GL(dl_x86_feature_1)[0] &= ~cet_feature;
+	  GL(dl_x86_feature_1) &= ~cet_feature;
 
 	  cet_feature_changed = true;
 	}
@@ -199,9 +197,9 @@  dl_cet_check (struct link_map *m, const char *program)
       if (program && (ibt_enabled || shstk_enabled))
 	{
 	  if ((!ibt_enabled
-	       || enable_ibt_type != CET_PERMISSIVE)
+	       || enable_ibt_type != permissive)
 	      && (!shstk_enabled
-		  || enable_shstk_type != CET_PERMISSIVE))
+		  || enable_shstk_type != permissive))
 	    {
 	      /* Lock CET if IBT or SHSTK is enabled in executable unless
 	         IBT or SHSTK is enabled permissively.  */
diff --git a/sysdeps/x86/dl-procruntime.c b/sysdeps/x86/dl-procruntime.c
index 88a7cac646..129f0dd338 100644
--- a/sysdeps/x86/dl-procruntime.c
+++ b/sysdeps/x86/dl-procruntime.c
@@ -47,11 +47,26 @@ 
 # if !defined PROCINFO_DECL && defined SHARED
   ._dl_x86_feature_1
 # else
-PROCINFO_CLASS unsigned int _dl_x86_feature_1[2]
+PROCINFO_CLASS unsigned int _dl_x86_feature_1
+# endif
+# ifndef PROCINFO_DECL
+= 0
+# endif
+# if !defined SHARED || defined PROCINFO_DECL
+;
+# else
+,
+# endif
+
+# if !defined PROCINFO_DECL && defined SHARED
+  ._dl_x86_feature_control
+# else
+PROCINFO_CLASS struct dl_x86_feature_control _dl_x86_feature_control
 # endif
 # ifndef PROCINFO_DECL
 = {
-    0,
+    .ibt = elf_property,
+    .shstk = elf_property
   }
 # endif
 # if !defined SHARED || defined PROCINFO_DECL
diff --git a/sysdeps/x86/ldsodefs.h b/sysdeps/x86/ldsodefs.h
index 072f826666..5b1b09dbf8 100644
--- a/sysdeps/x86/ldsodefs.h
+++ b/sysdeps/x86/ldsodefs.h
@@ -61,6 +61,7 @@  struct La_x32_retval;
 				     struct La_x86_64_retval *,		\
 				     const char *)
 
+#include <cet-control.h>
 #include_next <ldsodefs.h>
 
 #endif
-- 
2.26.2