[PATCH] nptl: fix potential merge of __rseq_* relro symbols

[PATCH] nptl: fix potential merge of __rseq_* relro symbols

[Michael Jeanson]

While working on a patch to add support for the extensible rseq ABI, we
came across an issue where a new 'const' variable would be merged with
the existing '__rseq_size' variable. We tracked this to the use of
'-fmerge-all-constants' which allows the compiler to merge identical
constant variables. This means that all 'const' variables in a compile
unit that are of the same size and are initialized to the same value can
be merged.

In this specific case, on 32 bit systems 'unsigned int' and 'ptrdiff_t'
are both 4 bytes and initialized to 0 which should trigger the merge.
However for reasons we haven't delved into when the attribute 'section
(".data.rel.ro")' is added to the mix, only variables of the same exact
types are merged. As far as we know this behavior is not specified
anywhere and could change with a new compiler version, hence this patch.

Declare both variables as regular static variables and add 'extern
const' aliases for the ABI. This has the added bonus of removing the asm
workaround to set the values on rseq registration.

Tested on Debian 12 with GCC 12.2.

Signed-off-by: Michael Jeanson <mjeanson@efficios.com>
Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
 sysdeps/nptl/dl-tls_init_tp.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/sysdeps/nptl/dl-tls_init_tp.c b/sysdeps/nptl/dl-tls_init_tp.c
index 092c274f36..7f66f6de32 100644
--- a/sysdeps/nptl/dl-tls_init_tp.c
+++ b/sysdeps/nptl/dl-tls_init_tp.c
@@ -45,8 +45,15 @@ rtld_mutex_dummy (pthread_mutex_t *lock)
 #endif
 
 const unsigned int __rseq_flags;
-const unsigned int __rseq_size attribute_relro;
-const ptrdiff_t __rseq_offset attribute_relro;
+
+/* Due to the use of '-fmerge-all-constants' the compiler is allowed to merge
+   all 'const' variables of the same size that are initialized to the same
+   value.  To work around this, declare them as regular static variable and use
+   an alias that is 'const'.  */
+static unsigned int rseq_size attribute_relro;
+extern const unsigned int __attribute__ ((alias ("rseq_size"))) __rseq_size;
+static ptrdiff_t rseq_offset attribute_relro;
+extern const ptrdiff_t __attribute__ ((alias ("rseq_offset"))) __rseq_offset;
 
 void
 __tls_pre_init_tp (void)
@@ -105,10 +112,8 @@ __tls_init_tp (void)
     do_rseq = TUNABLE_GET (rseq, int, NULL);
     if (rseq_register_current_thread (pd, do_rseq))
       {
-        /* We need a writable view of the variables.  They are in
-           .data.relro and are not yet write-protected.  */
-        extern unsigned int size __asm__ ("__rseq_size");
-        size = sizeof (pd->rseq_area);
+	/* The variables are in .data.relro but are not yet write-protected.  */
+        rseq_size = sizeof (pd->rseq_area);
       }
 
 #ifdef RSEQ_SIG
@@ -117,8 +122,7 @@ __tls_init_tp (void)
        all targets support __thread_pointer, so set __rseq_offset only
        if the rseq registration may have happened because RSEQ_SIG is
        defined.  */
-    extern ptrdiff_t offset __asm__ ("__rseq_offset");
-    offset = (char *) &pd->rseq_area - (char *) __thread_pointer ();
+    rseq_offset = (char *) &pd->rseq_area - (char *) __thread_pointer ();
 #endif
   }
 
-- 
2.34.1

Re: [PATCH] nptl: fix potential merge of __rseq_* relro symbols

[H.J. Lu]

On Mon, Jan 15, 2024 at 1:10 PM Michael Jeanson <mjeanson@efficios.com> wrote:
>
> While working on a patch to add support for the extensible rseq ABI, we
> came across an issue where a new 'const' variable would be merged with
> the existing '__rseq_size' variable. We tracked this to the use of
> '-fmerge-all-constants' which allows the compiler to merge identical
> constant variables. This means that all 'const' variables in a compile
> unit that are of the same size and are initialized to the same value can
> be merged.
>
> In this specific case, on 32 bit systems 'unsigned int' and 'ptrdiff_t'
> are both 4 bytes and initialized to 0 which should trigger the merge.
> However for reasons we haven't delved into when the attribute 'section
> (".data.rel.ro")' is added to the mix, only variables of the same exact
> types are merged. As far as we know this behavior is not specified
> anywhere and could change with a new compiler version, hence this patch.
>
> Declare both variables as regular static variables and add 'extern
> const' aliases for the ABI. This has the added bonus of removing the asm
> workaround to set the values on rseq registration.
>
> Tested on Debian 12 with GCC 12.2.
>
> Signed-off-by: Michael Jeanson <mjeanson@efficios.com>
> Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> ---
>  sysdeps/nptl/dl-tls_init_tp.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/sysdeps/nptl/dl-tls_init_tp.c b/sysdeps/nptl/dl-tls_init_tp.c
> index 092c274f36..7f66f6de32 100644
> --- a/sysdeps/nptl/dl-tls_init_tp.c
> +++ b/sysdeps/nptl/dl-tls_init_tp.c
> @@ -45,8 +45,15 @@ rtld_mutex_dummy (pthread_mutex_t *lock)
>  #endif
>
>  const unsigned int __rseq_flags;
> -const unsigned int __rseq_size attribute_relro;
> -const ptrdiff_t __rseq_offset attribute_relro;
> +
> +/* Due to the use of '-fmerge-all-constants' the compiler is allowed to merge
> +   all 'const' variables of the same size that are initialized to the same
> +   value.  To work around this, declare them as regular static variable and use
> +   an alias that is 'const'.  */
> +static unsigned int rseq_size attribute_relro;
> +extern const unsigned int __attribute__ ((alias ("rseq_size"))) __rseq_size;
> +static ptrdiff_t rseq_offset attribute_relro;
> +extern const ptrdiff_t __attribute__ ((alias ("rseq_offset"))) __rseq_offset;

Don't you want to mark them as hidden if they are internal to glibc?

>  void
>  __tls_pre_init_tp (void)
> @@ -105,10 +112,8 @@ __tls_init_tp (void)
>      do_rseq = TUNABLE_GET (rseq, int, NULL);
>      if (rseq_register_current_thread (pd, do_rseq))
>        {
> -        /* We need a writable view of the variables.  They are in
> -           .data.relro and are not yet write-protected.  */
> -        extern unsigned int size __asm__ ("__rseq_size");
> -        size = sizeof (pd->rseq_area);
> +       /* The variables are in .data.relro but are not yet write-protected.  */
> +        rseq_size = sizeof (pd->rseq_area);
>        }
>
>  #ifdef RSEQ_SIG
> @@ -117,8 +122,7 @@ __tls_init_tp (void)
>         all targets support __thread_pointer, so set __rseq_offset only
>         if the rseq registration may have happened because RSEQ_SIG is
>         defined.  */
> -    extern ptrdiff_t offset __asm__ ("__rseq_offset");
> -    offset = (char *) &pd->rseq_area - (char *) __thread_pointer ();
> +    rseq_offset = (char *) &pd->rseq_area - (char *) __thread_pointer ();
>  #endif
>    }
>
> --
> 2.34.1
>


-- 
H.J.

Re: [PATCH] nptl: fix potential merge of __rseq_* relro symbols

[Michael Jeanson]

On 2024-01-15 16:20, H.J. Lu wrote:
>> +/* Due to the use of '-fmerge-all-constants' the compiler is allowed to merge
>> +   all 'const' variables of the same size that are initialized to the same
>> +   value.  To work around this, declare them as regular static variable and use
>> +   an alias that is 'const'.  */
>> +static unsigned int rseq_size attribute_relro;
>> +extern const unsigned int __attribute__ ((alias ("rseq_size"))) __rseq_size;
>> +static ptrdiff_t rseq_offset attribute_relro;
>> +extern const ptrdiff_t __attribute__ ((alias ("rseq_offset"))) __rseq_offset;
> 
> Don't you want to mark them as hidden if they are internal to glibc?

Indeed, the static variables should be hidden, I thought the build system was 
setup to make them by default, I'll send a v2.

Thanks,

Michael

Re: [PATCH] nptl: fix potential merge of __rseq_* relro symbols

[Michael Jeanson]

On 2024-01-15 16:34, Michael Jeanson wrote:
> On 2024-01-15 16:20, H.J. Lu wrote:
>>> +/* Due to the use of '-fmerge-all-constants' the compiler is allowed to merge
>>> +   all 'const' variables of the same size that are initialized to the same
>>> +   value.  To work around this, declare them as regular static variable 
>>> and use
>>> +   an alias that is 'const'.  */
>>> +static unsigned int rseq_size attribute_relro;
>>> +extern const unsigned int __attribute__ ((alias ("rseq_size"))) __rseq_size;
>>> +static ptrdiff_t rseq_offset attribute_relro;
>>> +extern const ptrdiff_t __attribute__ ((alias ("rseq_offset"))) __rseq_offset;
>>
>> Don't you want to mark them as hidden if they are internal to glibc?
> 
> Indeed, the static variables should be hidden, I thought the build system was 
> setup to make them by default, I'll send a v2.

I answered this one too quickly, the 'static' symbols rseq_size and 
rseq_offset are hidden which is okay and the alias symbols __rseq_size and 
__rseq_offset are public and part of the API/ABI since 2.35.

Michael

Re: [PATCH] nptl: fix potential merge of __rseq_* relro symbols

[Florian Weimer]

* Michael Jeanson:

> While working on a patch to add support for the extensible rseq ABI, we
> came across an issue where a new 'const' variable would be merged with
> the existing '__rseq_size' variable. We tracked this to the use of
> '-fmerge-all-constants' which allows the compiler to merge identical
> constant variables. This means that all 'const' variables in a compile
> unit that are of the same size and are initialized to the same value can
> be merged.
>
> In this specific case, on 32 bit systems 'unsigned int' and 'ptrdiff_t'
> are both 4 bytes and initialized to 0 which should trigger the merge.
> However for reasons we haven't delved into when the attribute 'section
> (".data.rel.ro")' is added to the mix, only variables of the same exact
> types are merged. As far as we know this behavior is not specified
> anywhere and could change with a new compiler version, hence this patch.
>
> Declare both variables as regular static variables and add 'extern
> const' aliases for the ABI. This has the added bonus of removing the asm
> workaround to set the values on rseq registration.

I think we should move the definition of these variables into an
assembler file, like elf/dl-debug-symbols.S.  This way, we can also
create the desired hidden aliases more easily, too.

I can come up with a patch or review yours, please tell me what you
prefer.

Thanks,
Florian

Re: [PATCH] nptl: fix potential merge of __rseq_* relro symbols

[Michael Jeanson]

On 2024-01-16 06:25, Florian Weimer wrote:
> * Michael Jeanson:
> 
> I think we should move the definition of these variables into an
> assembler file, like elf/dl-debug-symbols.S.  This way, we can also
> create the desired hidden aliases more easily, too.
> 
> I can come up with a patch or review yours, please tell me what you
> prefer.

I'm not exactly sure I understand what your are trying to address, if you can 
come up with a patch that would be great.

Thanks!

Michael

[PATCH v2] nptl: fix potential merge of __rseq_* relro symbols

[Michael Jeanson]

While working on a patch to add support for the extensible rseq ABI, we
came across an issue where a new 'const' variable would be merged with
the existing '__rseq_size' variable. We tracked this to the use of
'-fmerge-all-constants' which allows the compiler to merge identical
constant variables. This means that all 'const' variables in a compile
unit that are of the same size and are initialized to the same value can
be merged.

In this specific case, on 32 bit systems 'unsigned int' and 'ptrdiff_t'
are both 4 bytes and initialized to 0 which should trigger the merge.
However for reasons we haven't delved into when the attribute 'section
(".data.rel.ro")' is added to the mix, only variables of the same exact
types are merged. As far as we know this behavior is not specified
anywhere and could change with a new compiler version, hence this patch.

Move the definitions of these variables into an assembler file and add
hidden writable aliases for internal use. This has the added bonus of
removing the asm workaround to set the values on rseq registration.

Tested on Debian 12 with GCC 12.2.

Signed-off-by: Michael Jeanson <mjeanson@efficios.com>
Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Florian Weimer <fweimer@redhat.com>
---
Changes since v1:
- Use an assembler file instead of static variables
---
 csu/Makefile                  |  2 +-
 csu/rseq-sizes.sym            |  8 +++++
 elf/Makefile                  |  1 +
 elf/dl-rseq-symbols.S         | 55 +++++++++++++++++++++++++++++++++++
 sysdeps/nptl/dl-tls_init_tp.c | 14 ++++-----
 5 files changed, 71 insertions(+), 9 deletions(-)
 create mode 100644 csu/rseq-sizes.sym
 create mode 100644 elf/dl-rseq-symbols.S

diff --git a/csu/Makefile b/csu/Makefile
index ac05ab24d5..0bf51a0e48 100644
--- a/csu/Makefile
+++ b/csu/Makefile
@@ -99,7 +99,7 @@ before-compile += $(objpfx)abi-tag.h
 generated += abi-tag.h
 
 # Put it here to generate it earlier.
-gen-as-const-headers += rtld-sizes.sym
+gen-as-const-headers += rtld-sizes.sym rseq-sizes.sym
 
 # These are the special initializer/finalizer files.  They are always the
 # first and last file in the link.  crti.o ... crtn.o define the global
diff --git a/csu/rseq-sizes.sym b/csu/rseq-sizes.sym
new file mode 100644
index 0000000000..c959758ff0
--- /dev/null
+++ b/csu/rseq-sizes.sym
@@ -0,0 +1,8 @@
+#include <stddef.h>
+
+--
+RSEQ_SIZE_SIZE		sizeof (unsigned int)
+RSEQ_SIZE_ALIGN		__alignof (unsigned int)
+
+RSEQ_OFFSET_SIZE	sizeof (ptrdiff_t)
+RSEQ_OFFSET_ALIGN	__alignof (ptrdiff_t)
diff --git a/elf/Makefile b/elf/Makefile
index 5d78b659ce..7d711aedf0 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -73,6 +73,7 @@ dl-routines = \
   dl-origin \
   dl-printf \
   dl-reloc \
+  dl-rseq-symbols \
   dl-runtime \
   dl-scope \
   dl-setup_hash \
diff --git a/elf/dl-rseq-symbols.S b/elf/dl-rseq-symbols.S
new file mode 100644
index 0000000000..2d8e88367f
--- /dev/null
+++ b/elf/dl-rseq-symbols.S
@@ -0,0 +1,55 @@
+/* Define symbols used by rseq.
+   Copyright (C) 2024 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 <rseq-sizes.h>
+#include <sysdep.h>
+
+/* Some targets define a macro to denote the zero register.  */
+#undef zero
+
+/* Define 2 symbols, __rseq_size is public const and _rseq_size, which is an
+   alias of __rseq_size, but hidden and writable for internal use.  */
+
+	.globl	__rseq_size
+	.type	__rseq_size, %object
+	.size	__rseq_size, RSEQ_SIZE_SIZE
+	.hidden _rseq_size
+	.globl	_rseq_size
+	.type	_rseq_size, %object
+	.size	_rseq_size, RSEQ_SIZE_SIZE
+	.section .data.rel.ro
+	.balign	RSEQ_SIZE_ALIGN
+__rseq_size:
+_rseq_size:
+	.zero	RSEQ_SIZE_SIZE
+
+/* Define 2 symbols, __rseq_offset is public const and _rseq_offset, which is an
+   alias of __rseq_offset, but hidden and writable for internal use.  */
+
+	.globl	__rseq_offset
+	.type	__rseq_offset, %object
+	.size	__rseq_offset, RSEQ_OFFSET_SIZE
+	.hidden _rseq_offset
+	.globl	_rseq_offset
+	.type	_rseq_offset, %object
+	.size	_rseq_offset, RSEQ_OFFSET_SIZE
+	.section .data.rel.ro
+	.balign	RSEQ_OFFSET_ALIGN
+__rseq_offset:
+_rseq_offset:
+	.zero	RSEQ_OFFSET_SIZE
diff --git a/sysdeps/nptl/dl-tls_init_tp.c b/sysdeps/nptl/dl-tls_init_tp.c
index 092c274f36..80eb0107b5 100644
--- a/sysdeps/nptl/dl-tls_init_tp.c
+++ b/sysdeps/nptl/dl-tls_init_tp.c
@@ -45,8 +45,10 @@ rtld_mutex_dummy (pthread_mutex_t *lock)
 #endif
 
 const unsigned int __rseq_flags;
-const unsigned int __rseq_size attribute_relro;
-const ptrdiff_t __rseq_offset attribute_relro;
+
+/* The variables are in .data.relro but are not yet write-protected.  */
+extern unsigned int _rseq_size attribute_relro attribute_hidden;
+extern ptrdiff_t _rseq_offset attribute_relro attribute_hidden;
 
 void
 __tls_pre_init_tp (void)
@@ -105,10 +107,7 @@ __tls_init_tp (void)
     do_rseq = TUNABLE_GET (rseq, int, NULL);
     if (rseq_register_current_thread (pd, do_rseq))
       {
-        /* We need a writable view of the variables.  They are in
-           .data.relro and are not yet write-protected.  */
-        extern unsigned int size __asm__ ("__rseq_size");
-        size = sizeof (pd->rseq_area);
+        _rseq_size = sizeof (pd->rseq_area);
       }
 
 #ifdef RSEQ_SIG
@@ -117,8 +116,7 @@ __tls_init_tp (void)
        all targets support __thread_pointer, so set __rseq_offset only
        if the rseq registration may have happened because RSEQ_SIG is
        defined.  */
-    extern ptrdiff_t offset __asm__ ("__rseq_offset");
-    offset = (char *) &pd->rseq_area - (char *) __thread_pointer ();
+    _rseq_offset = (char *) &pd->rseq_area - (char *) __thread_pointer ();
 #endif
   }
 
-- 
2.34.1

Re: [PATCH v2] nptl: fix potential merge of __rseq_* relro symbols

[Michael Jeanson]

On 2024-01-22 17:11, Michael Jeanson wrote:
> While working on a patch to add support for the extensible rseq ABI, we
> came across an issue where a new 'const' variable would be merged with
> the existing '__rseq_size' variable. We tracked this to the use of
> '-fmerge-all-constants' which allows the compiler to merge identical
> constant variables. This means that all 'const' variables in a compile
> unit that are of the same size and are initialized to the same value can
> be merged.
> 
> In this specific case, on 32 bit systems 'unsigned int' and 'ptrdiff_t'
> are both 4 bytes and initialized to 0 which should trigger the merge.
> However for reasons we haven't delved into when the attribute 'section
> (".data.rel.ro")' is added to the mix, only variables of the same exact
> types are merged. As far as we know this behavior is not specified
> anywhere and could change with a new compiler version, hence this patch.
> 
> Move the definitions of these variables into an assembler file and add
> hidden writable aliases for internal use. This has the added bonus of
> removing the asm workaround to set the values on rseq registration.
> 
> Tested on Debian 12 with GCC 12.2.

Hi Florian,

I took a crack at it, hopefully it's close to what you had in mind.

Michael