diff mbox series

[v2,1/2] Config.in: ban textrels on musl toolchains

Message ID 20240529-ztext-v2-1-82985032f169@gmx.net
State Superseded
Headers show
Series Add option to ban textrels | expand

Commit Message

J. Neuschäfer May 29, 2024, 6:22 p.m. UTC 
musl-libc doesn't support TEXTRELs[1] and programs with TEXTRELs will
crash on start-up under musl.

This patch forbids the use of TEXTRELs on musl, but adds an option to
either forbid them on other libcs as well, or allow them on musl.

[1]: https://www.openwall.com/lists/musl/2020/09/25/4

Signed-off-by: J. Neuschäfer <j.neuschaefer@gmx.net>
---

v2:
- slightly different wording
- default to yes on musl toolchains
- fix check-package warnings
- depends on BR2_SHARED_LIBS -> depends on !BR2_STATIC_LIBS
---
 Config.in           | 14 ++++++++++++++
 package/Makefile.in |  4 ++++
 2 files changed, 18 insertions(+)


--
2.43.0

Comments

Yann E. MORIN June 1, 2024, 8:59 p.m. UTC | #1 
On 2024-05-29 20:22 +0200, J. Neuschäfer via buildroot spake thusly:
> musl-libc doesn't support TEXTRELs[1] and programs with TEXTRELs will
> crash on start-up under musl.
> 
> This patch forbids the use of TEXTRELs on musl, but adds an option to
> either forbid them on other libcs as well, or allow them on musl.

In your v1, Thomas suggested that textrels be simply always disabled for
musl, because there is no way they can work. Allowing people to enable
them on musl will only make things reak at runtime, rather than at build
time.

I'm on the same page as Thomas here: if textrels are unsupported on
musl, then let's not allow them at all.

> [1]: https://www.openwall.com/lists/musl/2020/09/25/4
> 
> Signed-off-by: J. Neuschäfer <j.neuschaefer@gmx.net>
> ---
[--SNIP--]
> diff --git a/Config.in b/Config.in
> index b5a94325c4..92e89ed32d 100644
> --- a/Config.in
> +++ b/Config.in
> @@ -910,6 +910,20 @@ endchoice
>  comment "RELocation Read Only (RELRO) needs shared libraries"
>  	depends on !BR2_SHARED_LIBS
> 
> +config BR2_LINK_ZTEXT
> +	bool "Disallow text section relocations (TEXTRELs)"

We prefer positive logic:

    config BR2_LINK_ZTEXT
        bool "Allow text section relocations (TEXTRELs)"
        default y  # Legacy
        depends on !BR2_TOOLCHAIN_USES_MUSL
        depends on !BR2_STATIC_LIBS

and in the .mk:

    ifeq ($(BR2_LINK_ZTEXT),)
    TARGET_LDFLAGS += -z text
    endif

(Note the 'default y': textrels were previously always allowed, so we
want to keep that behaviour by default)

Regards,
Yann E. MORIN.

> +	default BR2_TOOLCHAIN_USES_MUSL
> +	depends on !BR2_STATIC_LIBS
> +	help
> +	  Pass "-z text" to the linker to detect TEXTRELs and throw an
> +	  error if they occur.
> +
> +	  This is recommended when building a system with musl-libc,
> +	  because TEXTRELs are not supported on musl-libc's dynamic
> +	  loader and will result in a run-time crash:
> +
> +	    https://www.openwall.com/lists/musl/2020/09/25/4
> +
>  config BR2_FORTIFY_SOURCE_ARCH_SUPPORTS
>  	bool
>  	default y
> diff --git a/package/Makefile.in b/package/Makefile.in
> index f415e55f84..68efd39dba 100644
> --- a/package/Makefile.in
> +++ b/package/Makefile.in
> @@ -149,6 +149,10 @@ endif
> 
>  TARGET_LDFLAGS = $(call qstrip,$(BR2_TARGET_LDFLAGS))
> 
> +ifeq ($(BR2_LINK_ZTEXT),y)
> +TARGET_LDFLAGS += -z text
> +endif
> +
>  # By design, _FORTIFY_SOURCE requires gcc optimization to be enabled.
>  # Therefore, we need to pass _FORTIFY_SOURCE and the optimization level
>  # through the same mechanism, i.e currently through CFLAGS. Passing
> 
> --
> 2.43.0
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot
J. Neuschäfer June 9, 2024, 12:54 p.m. UTC | #2 
Hello Yann, sorry for the delay,

On Sat, Jun 01, 2024 at 10:59:53PM +0200, Yann E. MORIN wrote:
> On 2024-05-29 20:22 +0200, J. Neuschäfer via buildroot spake thusly:
> > musl-libc doesn't support TEXTRELs[1] and programs with TEXTRELs will
> > crash on start-up under musl.
> >
> > This patch forbids the use of TEXTRELs on musl, but adds an option to
> > either forbid them on other libcs as well, or allow them on musl.
>
> In your v1, Thomas suggested that textrels be simply always disabled for
> musl, because there is no way they can work. Allowing people to enable
> them on musl will only make things reak at runtime, rather than at build
> time.
>
> I'm on the same page as Thomas here: if textrels are unsupported on
> musl, then let's not allow them at all.

Alright, I wasn't fully certain about it, but I'll change the Kconfig
logic accordingly.

>
> > [1]: https://www.openwall.com/lists/musl/2020/09/25/4
> >
> > Signed-off-by: J. Neuschäfer <j.neuschaefer@gmx.net>
> > ---
> [--SNIP--]
> > diff --git a/Config.in b/Config.in
> > index b5a94325c4..92e89ed32d 100644
> > --- a/Config.in
> > +++ b/Config.in
> > @@ -910,6 +910,20 @@ endchoice
> >  comment "RELocation Read Only (RELRO) needs shared libraries"
> >  	depends on !BR2_SHARED_LIBS
> >
> > +config BR2_LINK_ZTEXT
> > +	bool "Disallow text section relocations (TEXTRELs)"
>
> We prefer positive logic:
>
>     config BR2_LINK_ZTEXT
>         bool "Allow text section relocations (TEXTRELs)"
>         default y  # Legacy
>         depends on !BR2_TOOLCHAIN_USES_MUSL
>         depends on !BR2_STATIC_LIBS

Good idea, this looks a bit easier to understand.

>
> and in the .mk:
>
>     ifeq ($(BR2_LINK_ZTEXT),)
>     TARGET_LDFLAGS += -z text
>     endif
>
> (Note the 'default y': textrels were previously always allowed, so we
> want to keep that behaviour by default)
>
> Regards,
> Yann E. MORIN.

Thanks for your review!
--jn
diff mbox series

Patch

diff --git a/Config.in b/Config.in
index b5a94325c4..92e89ed32d 100644
--- a/Config.in
+++ b/Config.in
@@ -910,6 +910,20 @@  endchoice
 comment "RELocation Read Only (RELRO) needs shared libraries"
 	depends on !BR2_SHARED_LIBS

+config BR2_LINK_ZTEXT
+	bool "Disallow text section relocations (TEXTRELs)"
+	default BR2_TOOLCHAIN_USES_MUSL
+	depends on !BR2_STATIC_LIBS
+	help
+	  Pass "-z text" to the linker to detect TEXTRELs and throw an
+	  error if they occur.
+
+	  This is recommended when building a system with musl-libc,
+	  because TEXTRELs are not supported on musl-libc's dynamic
+	  loader and will result in a run-time crash:
+
+	    https://www.openwall.com/lists/musl/2020/09/25/4
+
 config BR2_FORTIFY_SOURCE_ARCH_SUPPORTS
 	bool
 	default y
diff --git a/package/Makefile.in b/package/Makefile.in
index f415e55f84..68efd39dba 100644
--- a/package/Makefile.in
+++ b/package/Makefile.in
@@ -149,6 +149,10 @@  endif

 TARGET_LDFLAGS = $(call qstrip,$(BR2_TARGET_LDFLAGS))

+ifeq ($(BR2_LINK_ZTEXT),y)
+TARGET_LDFLAGS += -z text
+endif
+
 # By design, _FORTIFY_SOURCE requires gcc optimization to be enabled.
 # Therefore, we need to pass _FORTIFY_SOURCE and the optimization level
 # through the same mechanism, i.e currently through CFLAGS. Passing