[v7,18/23] aarch64: Handle GCS marking

  From: Szabolcs Nagy <szabolcs.nagy@arm.com>

 - Handle GCS marking
 - Use l_searchlist.r_list for gcs (allows using the
   same function for static exe)
---
 sysdeps/aarch64/Makefile  |  4 ++-
 sysdeps/aarch64/dl-gcs.c  | 71 +++++++++++++++++++++++++++++++++++++++
 sysdeps/aarch64/dl-prop.h | 15 ++++++---
 sysdeps/aarch64/linkmap.h |  1 +
 4 files changed, 85 insertions(+), 6 deletions(-)
 create mode 100644 sysdeps/aarch64/dl-gcs.c
  

Hi Yury,
 
 > - Handle GCS marking
 > - Use l_searchlist.r_list for gcs (allows using the
 >  same function for static exe)

This is OK.

Reviewed-by: Wilco Dijkstra <Wilco.Dijkstra@arm.com>

Cheers,
Wilco


diff --git a/sysdeps/aarch64/Makefile b/sysdeps/aarch64/Makefile
index ca8b96f550..74479604f2 100644
--- a/sysdeps/aarch64/Makefile
+++ b/sysdeps/aarch64/Makefile
@@ -9,7 +9,9 @@ LDFLAGS-rtld += -Wl,-z,force-bti,--fatal-warnings
 endif
 
 ifeq ($(subdir),elf)
-sysdep-dl-routines += dl-bti
+sysdep-dl-routines += \
+  dl-bti \
+  dl-gcs
 
 tests += tst-audit26 \
          tst-audit27


OK

diff --git a/sysdeps/aarch64/dl-gcs.c b/sysdeps/aarch64/dl-gcs.c
new file mode 100644
index 0000000000..dc62908d13
--- /dev/null
+++ b/sysdeps/aarch64/dl-gcs.c
@@ -0,0 +1,71 @@
+/* AArch64 GCS functions.
+   Copyright (C) 2024-2025 Free Software Foundation, Inc.
+
+   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 <unistd.h>
+#include <errno.h>
+#include <libintl.h>
+#include <ldsodefs.h>
+
+static void
+fail (struct link_map *l, const char *program)
+{
+  if (program && program[0])
+    _dl_fatal_printf ("%s: %s: %s\n", program, l->l_name, N_("not GCS compatible"));
+  else if (program)
+    _dl_fatal_printf ("%s\n", N_("not GCS compatible"));
+  else
+    _dl_signal_error (0, l->l_name, "dlopen", N_("not GCS compatible"));
+}
+
+static void
+unsupported (void)
+{
+  _dl_fatal_printf ("%s\n", N_("unsupported GCS policy"));
+}
+
+static void
+check_gcs (struct link_map *l, const char *program)
+{
+  bool for_dlopen = program == NULL;
+  if (!l->l_mach.gcs)
+    {
+      if (GLRO(dl_aarch64_gcs_policy) == 2 || for_dlopen)
+       fail (l, program);
+      if (GLRO(dl_aarch64_gcs_policy) == 1)
+       GL(dl_aarch64_gcs) = 0;
+      else
+       unsupported ();
+    }
+}
+
+/* Apply GCS policy for L and its dependencies.
+   PROGRAM is NULL when this check is invoked for dl_open.  */
+
+void
+_dl_gcs_check (struct link_map *l, const char *program)
+{
+  /* GCS is disabled.  */
+  if (GL(dl_aarch64_gcs) == 0)
+    return;
+  /* GCS marking is ignored.  */
+  if (GLRO(dl_aarch64_gcs_policy) == 0)
+    return;
+
+  check_gcs (l, program);
+  for (unsigned int i = 0; i < l->l_searchlist.r_nlist; i++)
+    check_gcs (l->l_searchlist.r_list[i], program);
+}

OK

diff --git a/sysdeps/aarch64/dl-prop.h b/sysdeps/aarch64/dl-prop.h
index 361fc593da..abca2be7fa 100644
--- a/sysdeps/aarch64/dl-prop.h
+++ b/sysdeps/aarch64/dl-prop.h
@@ -24,16 +24,21 @@ extern void _dl_bti_protect (struct link_map *, int) attribute_hidden;
 extern void _dl_bti_check (struct link_map *, const char *)
     attribute_hidden;
 
+extern void _dl_gcs_check (struct link_map *, const char *)
+    attribute_hidden;
+
 static inline void __attribute__ ((always_inline))
 _rtld_main_check (struct link_map *m, const char *program)
 {
   _dl_bti_check (m, program);
+  _dl_gcs_check (m, program);
 }
 
 static inline void __attribute__ ((always_inline))
 _dl_open_check (struct link_map *m)
 {
   _dl_bti_check (m, NULL);
+  _dl_gcs_check (m, NULL);
 }
 
 static inline void __attribute__ ((always_inline))
@@ -45,10 +50,6 @@ static inline int
 _dl_process_gnu_property (struct link_map *l, int fd, uint32_t type,
                           uint32_t datasz, void *data)
 {
-  if (!GLRO(dl_aarch64_cpu_features).bti)
-    /* Skip note processing.  */
-    return 0;
-
   if (type == GNU_PROPERTY_AARCH64_FEATURE_1_AND)
     {
       /* Stop if the property note is ill-formed.  */
@@ -57,7 +58,11 @@ _dl_process_gnu_property (struct link_map *l, int fd, uint32_t type,
 
       unsigned int feature_1 = *(unsigned int *) data;
       if (feature_1 & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)
-       _dl_bti_protect (l, fd);
+       if (GLRO(dl_aarch64_cpu_features).bti)
+         _dl_bti_protect (l, fd);
+
+      if (feature_1 & GNU_PROPERTY_AARCH64_FEATURE_1_GCS)
+       l->l_mach.gcs = 1;
 
       /* Stop if we processed the property note.  */
       return 0;
diff --git a/sysdeps/aarch64/linkmap.h b/sysdeps/aarch64/linkmap.h
index df6d3c66e8..e56c890aea 100644
--- a/sysdeps/aarch64/linkmap.h
+++ b/sysdeps/aarch64/linkmap.h
@@ -23,4 +23,5 @@ struct link_map_machine
   ElfW(Addr) plt;        /* Address of .plt */
   void *tlsdesc_table;   /* Address of TLS descriptor hash table.  */
   bool bti_fail;         /* Failed to enable Branch Target Identification.  */
+  bool gcs;              /* Guarded Control Stack marking.  */
 };


OK
  

On 03/01/25 12:41, Yury Khrustalev wrote:
> From: Szabolcs Nagy <szabolcs.nagy@arm.com>
> 
>  - Handle GCS marking
>  - Use l_searchlist.r_list for gcs (allows using the
>    same function for static exe)
> ---
>  sysdeps/aarch64/Makefile  |  4 ++-
>  sysdeps/aarch64/dl-gcs.c  | 71 +++++++++++++++++++++++++++++++++++++++
>  sysdeps/aarch64/dl-prop.h | 15 ++++++---
>  sysdeps/aarch64/linkmap.h |  1 +
>  4 files changed, 85 insertions(+), 6 deletions(-)
>  create mode 100644 sysdeps/aarch64/dl-gcs.c
> 
> diff --git a/sysdeps/aarch64/Makefile b/sysdeps/aarch64/Makefile
> index ca8b96f550..74479604f2 100644
> --- a/sysdeps/aarch64/Makefile
> +++ b/sysdeps/aarch64/Makefile
> @@ -9,7 +9,9 @@ LDFLAGS-rtld += -Wl,-z,force-bti,--fatal-warnings
>  endif
>  
>  ifeq ($(subdir),elf)
> -sysdep-dl-routines += dl-bti
> +sysdep-dl-routines += \
> +  dl-bti \
> +  dl-gcs
>  
>  tests += tst-audit26 \
>  	 tst-audit27
> diff --git a/sysdeps/aarch64/dl-gcs.c b/sysdeps/aarch64/dl-gcs.c
> new file mode 100644
> index 0000000000..dc62908d13
> --- /dev/null
> +++ b/sysdeps/aarch64/dl-gcs.c
> @@ -0,0 +1,71 @@
> +/* AArch64 GCS functions.
> +   Copyright (C) 2024-2025 Free Software Foundation, Inc.
> +
> +   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 <unistd.h>
> +#include <errno.h>
> +#include <libintl.h>
> +#include <ldsodefs.h>
> +
> +static void
> +fail (struct link_map *l, const char *program)
> +{
> +  if (program && program[0])
> +    _dl_fatal_printf ("%s: %s: %s\n", program, l->l_name, N_("not GCS compatible"));
> +  else if (program)
> +    _dl_fatal_printf ("%s\n", N_("not GCS compatible"));
> +  else
> +    _dl_signal_error (0, l->l_name, "dlopen", N_("not GCS compatible"));
> +}

I don't think N_ would make a difference here, it is not support on loader
and adding on the translation database does not add much without revamping
how to translation during program loading.

> +
> +static void
> +unsupported (void)
> +{
> +  _dl_fatal_printf ("%s\n", N_("unsupported GCS policy"));
> +}
> +
> +static void
> +check_gcs (struct link_map *l, const char *program)
> +{
> +  bool for_dlopen = program == NULL;
> +  if (!l->l_mach.gcs)
> +    {
> +      if (GLRO(dl_aarch64_gcs_policy) == 2 || for_dlopen)
> +	fail (l, program);
> +      if (GLRO(dl_aarch64_gcs_policy) == 1)
> +	GL(dl_aarch64_gcs) = 0;
> +      else
> +	unsupported ();
> +    }
> +}
> +

This requires to update the glibc.cpu.aarch64_gcs_policy entry at 
manual/tunables.texi with the description of what each value represents.

> +/* Apply GCS policy for L and its dependencies.
> +   PROGRAM is NULL when this check is invoked for dl_open.  */
> +
> +void
> +_dl_gcs_check (struct link_map *l, const char *program)
> +{
> +  /* GCS is disabled.  */
> +  if (GL(dl_aarch64_gcs) == 0)
> +    return;
> +  /* GCS marking is ignored.  */
> +  if (GLRO(dl_aarch64_gcs_policy) == 0)
> +    return;
> +
> +  check_gcs (l, program);
> +  for (unsigned int i = 0; i < l->l_searchlist.r_nlist; i++)
> +    check_gcs (l->l_searchlist.r_list[i], program);
> +}
> diff --git a/sysdeps/aarch64/dl-prop.h b/sysdeps/aarch64/dl-prop.h
> index 361fc593da..abca2be7fa 100644
> --- a/sysdeps/aarch64/dl-prop.h
> +++ b/sysdeps/aarch64/dl-prop.h
> @@ -24,16 +24,21 @@ extern void _dl_bti_protect (struct link_map *, int) attribute_hidden;
>  extern void _dl_bti_check (struct link_map *, const char *)
>      attribute_hidden;
>  
> +extern void _dl_gcs_check (struct link_map *, const char *)
> +    attribute_hidden;
> +
>  static inline void __attribute__ ((always_inline))
>  _rtld_main_check (struct link_map *m, const char *program)
>  {
>    _dl_bti_check (m, program);
> +  _dl_gcs_check (m, program);
>  }
>  
>  static inline void __attribute__ ((always_inline))
>  _dl_open_check (struct link_map *m)
>  {
>    _dl_bti_check (m, NULL);
> +  _dl_gcs_check (m, NULL);
>  }
>  
>  static inline void __attribute__ ((always_inline))
> @@ -45,10 +50,6 @@ static inline int
>  _dl_process_gnu_property (struct link_map *l, int fd, uint32_t type,
>  			  uint32_t datasz, void *data)
>  {
> -  if (!GLRO(dl_aarch64_cpu_features).bti)
> -    /* Skip note processing.  */
> -    return 0;
> -
>    if (type == GNU_PROPERTY_AARCH64_FEATURE_1_AND)
>      {
>        /* Stop if the property note is ill-formed.  */
> @@ -57,7 +58,11 @@ _dl_process_gnu_property (struct link_map *l, int fd, uint32_t type,
>  
>        unsigned int feature_1 = *(unsigned int *) data;
>        if (feature_1 & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)
> -	_dl_bti_protect (l, fd);
> +	if (GLRO(dl_aarch64_cpu_features).bti)
> +	  _dl_bti_protect (l, fd);
> +
> +      if (feature_1 & GNU_PROPERTY_AARCH64_FEATURE_1_GCS)
> +	l->l_mach.gcs = 1;
>  
>        /* Stop if we processed the property note.  */
>        return 0;
> diff --git a/sysdeps/aarch64/linkmap.h b/sysdeps/aarch64/linkmap.h
> index df6d3c66e8..e56c890aea 100644
> --- a/sysdeps/aarch64/linkmap.h
> +++ b/sysdeps/aarch64/linkmap.h
> @@ -23,4 +23,5 @@ struct link_map_machine
>    ElfW(Addr) plt;	  /* Address of .plt */
>    void *tlsdesc_table;	  /* Address of TLS descriptor hash table.  */
>    bool bti_fail;	  /* Failed to enable Branch Target Identification.  */
> +  bool gcs;		  /* Guarded Control Stack marking.  */
>  };
  

On Tue, Jan 07, 2025 at 02:05:46PM -0300, Adhemerval Zanella Netto wrote:
> 
> > +static void
> > +fail (struct link_map *l, const char *program)
> > +{
> > +  if (program && program[0])
> > +    _dl_fatal_printf ("%s: %s: %s\n", program, l->l_name, N_("not GCS compatible"));
> > +  else if (program)
> > +    _dl_fatal_printf ("%s\n", N_("not GCS compatible"));
> > +  else
> > +    _dl_signal_error (0, l->l_name, "dlopen", N_("not GCS compatible"));
> > +}
> 
> I don't think N_ would make a difference here, it is not support on loader
> and adding on the translation database does not add much without revamping
> how to translation during program loading.

Makes sense, thanks. I'll remove "N_".

> > +static void
> > +check_gcs (struct link_map *l, const char *program)
> > +{
> > +  bool for_dlopen = program == NULL;
> > +  if (!l->l_mach.gcs)
> > +    {
> > +      if (GLRO(dl_aarch64_gcs_policy) == 2 || for_dlopen)
> > +	fail (l, program);
> > +      if (GLRO(dl_aarch64_gcs_policy) == 1)
> > +	GL(dl_aarch64_gcs) = 0;
> > +      else
> > +	unsupported ();
> > +    }
> > +}
> > +
> 
> This requires to update the glibc.cpu.aarch64_gcs_policy entry at 
> manual/tunables.texi with the description of what each value represents.

Right now GCS tunable names are reserved in the manual. When corresponding
Linux kernel functionality has been released, there will be another patch
for Glibc that will add the description of both GCS tunables. At this point
these tunables will become "public". This will likely be after the upcoming
Glibc release.

Thanks,
Yury
  

On 08/01/25 10:59, Yury Khrustalev wrote:
> On Tue, Jan 07, 2025 at 02:05:46PM -0300, Adhemerval Zanella Netto wrote:
>>
>>> +static void
>>> +fail (struct link_map *l, const char *program)
>>> +{
>>> +  if (program && program[0])
>>> +    _dl_fatal_printf ("%s: %s: %s\n", program, l->l_name, N_("not GCS compatible"));
>>> +  else if (program)
>>> +    _dl_fatal_printf ("%s\n", N_("not GCS compatible"));
>>> +  else
>>> +    _dl_signal_error (0, l->l_name, "dlopen", N_("not GCS compatible"));
>>> +}
>>
>> I don't think N_ would make a difference here, it is not support on loader
>> and adding on the translation database does not add much without revamping
>> how to translation during program loading.
> 
> Makes sense, thanks. I'll remove "N_".
> 
>>> +static void
>>> +check_gcs (struct link_map *l, const char *program)
>>> +{
>>> +  bool for_dlopen = program == NULL;
>>> +  if (!l->l_mach.gcs)
>>> +    {
>>> +      if (GLRO(dl_aarch64_gcs_policy) == 2 || for_dlopen)
>>> +	fail (l, program);
>>> +      if (GLRO(dl_aarch64_gcs_policy) == 1)
>>> +	GL(dl_aarch64_gcs) = 0;
>>> +      else
>>> +	unsupported ();
>>> +    }
>>> +}
>>> +
>>
>> This requires to update the glibc.cpu.aarch64_gcs_policy entry at 
>> manual/tunables.texi with the description of what each value represents.
> 
> Right now GCS tunable names are reserved in the manual. When corresponding
> Linux kernel functionality has been released, there will be another patch
> for Glibc that will add the description of both GCS tunables. At this point
> these tunables will become "public". This will likely be after the upcoming
> Glibc release.

Ok, but this code is already defining the logic of how aarch64_gcs_policy
works and when a user glibc on a GCS enabled kernel the tunable will be
enacted.  I still think we need to document the current semantic, the
reserved rationale would make sense if the tunable was not enacted.
  

On Wed, Jan 08, 2025 at 11:08:07AM -0300, Adhemerval Zanella Netto wrote:
> 
> >> This requires to update the glibc.cpu.aarch64_gcs_policy entry at 
> >> manual/tunables.texi with the description of what each value represents.
> > 
> > Right now GCS tunable names are reserved in the manual. When corresponding
> > Linux kernel functionality has been released, there will be another patch
> > for Glibc that will add the description of both GCS tunables. At this point
> > these tunables will become "public". This will likely be after the upcoming
> > Glibc release.
> 
> Ok, but this code is already defining the logic of how aarch64_gcs_policy
> works and when a user glibc on a GCS enabled kernel the tunable will be
> enacted.  I still think we need to document the current semantic, the
> reserved rationale would make sense if the tunable was not enacted.

Carlos, can I ask for your opinion? I thought we discussed this and the decision
was to reserve tunable names for now and document them properly later after the
corresponding Linux kernel functionality has been released.

Thanks,
Yury