[Buildroot] [PATCH v3] Config.in: ban textrels on musl toolchains

[Buildroot] [PATCH v3] Config.in: ban textrels on musl toolchains

[J. Neuschäfer via buildroot]

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>
---
Changes in v3:
- drop micropython patch (already merged)
- rewrite to positive logic, as suggested by Yann E. MORIN
- Link to v2: https://lore.kernel.org/r/20240529-ztext-v2-0-82985032f169@gmx.net

Changes in v2:
- Slightly different wording
- Enable the option by default on musl toolchains
- Add patch to fix build of micropython

Link to v1:
- https://lore.kernel.org/r/20240419-ztext-v1-1-a8d5c2cfcf57@gmx.net
---

I'm not sure about the Kconfig help text for BR2_LINK_ALLOW_TEXTREL.
Perhaps it could be written a little nicer.


v3:
- rewrite to positive logic, as suggested by Yann E. MORIN
- adjust the name and help text accordingly

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(+)

diff --git a/Config.in b/Config.in
index bdf5fa8647..c8b52c0d74 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_ALLOW_TEXTREL
+	bool "Allow text section relocations (TEXTRELs)"
+	default y  # Legacy
+	depends on !BR2_TOOLCHAIN_USES_MUSL
+	depends on !BR2_STATIC_LIBS
+	help
+	  Enable this option to allow TEXTRELs. Otherwise "-z text" will be
+	  passed to the linker to detect them and throw an error if they occur.
+
+	  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 5476234c7e..3f1a1d3a35 100644
--- a/package/Makefile.in
+++ b/package/Makefile.in
@@ -149,6 +149,10 @@ endif

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

+ifeq ($(BR2_LINK_ALLOW_TEXTREL),)
+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

---
base-commit: 3ebc7c69d56430c34eba4c869d1d4fe4d1e8de55
change-id: 20240417-ztext-5accbab61c0a

Best regards,
--
J. Neuschäfer <j.neuschaefer@gmx.net>

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v3] Config.in: ban textrels on musl toolchains

[Arnout Vandecappelle via buildroot]

  Hi J.,

  [BTW are you OK with me calling you "J."? It feels a bit weird...]

On 04/07/2024 20:05, J. Neuschäfer via buildroot wrote:
> 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>
> ---
> Changes in v3:
> - drop micropython patch (already merged)
> - rewrite to positive logic, as suggested by Yann E. MORIN
> - Link to v2: https://lore.kernel.org/r/20240529-ztext-v2-0-82985032f169@gmx.net
> 
> Changes in v2:
> - Slightly different wording
> - Enable the option by default on musl toolchains
> - Add patch to fix build of micropython
> 
> Link to v1:
> - https://lore.kernel.org/r/20240419-ztext-v1-1-a8d5c2cfcf57@gmx.net
> ---
> 
> I'm not sure about the Kconfig help text for BR2_LINK_ALLOW_TEXTREL.
> Perhaps it could be written a little nicer.
> 
> 
> v3:
> - rewrite to positive logic, as suggested by Yann E. MORIN
> - adjust the name and help text accordingly
> 
> 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(+)
> 
> diff --git a/Config.in b/Config.in
> index bdf5fa8647..c8b52c0d74 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_ALLOW_TEXTREL
> +	bool "Allow text section relocations (TEXTRELs)"
> +	default y  # Legacy
> +	depends on !BR2_TOOLCHAIN_USES_MUSL
> +	depends on !BR2_STATIC_LIBS
> +	help
> +	  Enable this option to allow TEXTRELs. Otherwise "-z text" will be
> +	  passed to the linker to detect them and throw an error if they occur.
> +
> +	  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 5476234c7e..3f1a1d3a35 100644
> --- a/package/Makefile.in
> +++ b/package/Makefile.in
> @@ -149,6 +149,10 @@ endif
> 
>   TARGET_LDFLAGS = $(call qstrip,$(BR2_TARGET_LDFLAGS))
> 
> +ifeq ($(BR2_LINK_ALLOW_TEXTREL),)
> +TARGET_LDFLAGS += -z text
> +endif

  After some discussion, we concluded that we don't see the point of making this 
a user-settable Config.in option. It should be completely in the Makefile.in, 
with a comment explaining why. So something like this (untested!):

# musl's dynamic loader doesn't support DT_TEXTREL, which results in a runtime
# crash if it gets used. The "-z text" linker option issues a build-time error
# when DT_TEXREL is used, so we capture the problem earlier.
ifeq ($(BR2_TOOLCHAIN_USES_MUSL)$(BR2_STATIC_LIBS),yy)
TARGET_LDFLAGS += -#l,-z,text
endif

  Note also that we use -Wl,-z,... everywhere else to pass the -z options to the 
linker. Maybe that's because older GCC don't understand -z. But anyway, to be 
consistent, let's do the same here.

  Can you test the above version and resubmit? Oh, and please also in the commit 
message give an example of how you can test it (e.g. by removing the micropyton 
patch and building micropyton under musl?)

  Regards,
  Arnout

> +
>   # 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
> 
> ---
> base-commit: 3ebc7c69d56430c34eba4c869d1d4fe4d1e8de55
> change-id: 20240417-ztext-5accbab61c0a
> 
> Best regards,
> --
> J. Neuschäfer <j.neuschaefer@gmx.net>
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v3] Config.in: ban textrels on musl toolchains

[Thomas Petazzoni via buildroot]

On Mon, 15 Jul 2024 10:51:03 +0200
Arnout Vandecappelle <arnout@mind.be> wrote:

>   After some discussion, we concluded that we don't see the point of making this 
> a user-settable Config.in option. It should be completely in the Makefile.in, 
> with a comment explaining why. So something like this (untested!):
> 
> # musl's dynamic loader doesn't support DT_TEXTREL, which results in a runtime
> # crash if it gets used. The "-z text" linker option issues a build-time error
> # when DT_TEXREL is used, so we capture the problem earlier.
> ifeq ($(BR2_TOOLCHAIN_USES_MUSL)$(BR2_STATIC_LIBS),yy)
> TARGET_LDFLAGS += -#l,-z,text

                    ^^^ this should have been -Wl,-z,text of course

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v3] Config.in: ban textrels on musl toolchains

[J. Neuschäfer via buildroot]

On Mon, Jul 15, 2024 at 10:51:03AM +0200, Arnout Vandecappelle wrote:
>  Hi J.,
>
>  [BTW are you OK with me calling you "J."? It feels a bit weird...]

It's ok, as well as jn ("jay-enn"). which is also ok.

>
> On 04/07/2024 20:05, J. Neuschäfer via buildroot wrote:
[...]
> > +ifeq ($(BR2_LINK_ALLOW_TEXTREL),)
> > +TARGET_LDFLAGS += -z text
> > +endif
>
>  After some discussion, we concluded that we don't see the point of making
> this a user-settable Config.in option. It should be completely in the
> Makefile.in, with a comment explaining why. So something like this
> (untested!):
>
> # musl's dynamic loader doesn't support DT_TEXTREL, which results in a runtime
> # crash if it gets used. The "-z text" linker option issues a build-time error
> # when DT_TEXREL is used, so we capture the problem earlier.
> ifeq ($(BR2_TOOLCHAIN_USES_MUSL)$(BR2_STATIC_LIBS),yy)
> TARGET_LDFLAGS += -#l,-z,text
> endif
>
>  Note also that we use -Wl,-z,... everywhere else to pass the -z options to
> the linker. Maybe that's because older GCC don't understand -z. But anyway,
> to be consistent, let's do the same here.
>
>  Can you test the above version and resubmit? Oh, and please also in the
> commit message give an example of how you can test it (e.g. by removing the
> micropyton patch and building micropyton under musl?)

Yep, will do!
Thanks again for your review.


jn
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot