diff mbox series

[uclibc-ng-devel] powerpc: fix builds with newer gcc/binutils which use secureplt by default

Message ID 20210524190209.2598507-1-yann@sionneau.net
State Superseded
Headers show
Series [uclibc-ng-devel] powerpc: fix builds with newer gcc/binutils which use secureplt by default | expand

Commit Message

Yann Sionneau May 24, 2021, 7:02 p.m. UTC 
From: Yann Sionneau <yann@sionneau.net>

This patch fixes segfault of all user space processes (including init, which caused a panic) on recent buildroot powerpc32 builds.

Recent buildroot toolchain enables secure PLT in powerpc gcc.
The latter will then supply -msecure-plt to gas invocations by default.

For the secure PLT to work, the r30 register needs to point to the GOT.
Old "bss plt" was just a one-instruction-wide PLT slot, pointed-to by a R_PPC_JMP_SLOT relocation, which was written on-the-fly to contain a branch instruction to the correct address. It therefore had to stay writable.
New secure PLT only contains read-only code which loads the branch address from the writable GOT.

Signed-off-by: Yann Sionneau <yann@sionneau.net>
---
 Rules.mak                         | 4 +++-
 ldso/ldso/powerpc/dl-startup.h    | 3 +++
 libc/sysdeps/linux/powerpc/crt1.S | 5 ++++-
 3 files changed, 10 insertions(+), 2 deletions(-)

Comments

Romain Naour May 24, 2021, 9:45 p.m. UTC | #1 
Hello Yann,

Thanks for the help!

Le 24/05/2021 à 21:02, yann@sionneau.net a écrit :
> From: Yann Sionneau <yann@sionneau.net>
> 
> This patch fixes segfault of all user space processes (including init, which caused a panic) on recent buildroot powerpc32 builds.

Indeed, recent Buildroot enable by default BR2_PIC_PIE option.

> 
> Recent buildroot toolchain enables secure PLT in powerpc gcc.
> The latter will then supply -msecure-plt to gas invocations by default.

I guess the problem is using secure PLT and PIC/PIE at the same time. By
disabling BR2_PIC_PIE the issue is gone.

Maybe you can keep in the commit log the link to the initial report:
https://mailman.uclibc-ng.org/pipermail/devel/2021-May/002068.html

> 
> For the secure PLT to work, the r30 register needs to point to the GOT.
> Old "bss plt" was just a one-instruction-wide PLT slot, pointed-to by a R_PPC_JMP_SLOT relocation, which was written on-the-fly to contain a branch instruction to the correct address. It therefore had to stay writable.
> New secure PLT only contains read-only code which loads the branch address from the writable GOT.

With this patch applied, the init program still crash during boot:

transfering control to application @ 0x690a40
init[1]: segfault (11) at 378 nip 695e80 lr 0 code 1 in busybox[5e0000+e1000]
init[1]: code: 817e97a4 7d6903a6 4e800420 60000000 817ebedc 7d6903a6 4e800420
60000000
init[1]: code: 817ed9b8 7d6903a6 4e800420 60000000 <817e0378> 7d6903a6 4e800420
60000000
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b

It's due to the PPC_HAS_SECUREPLT test where the '\' charactere is still present
in the generated source file:

  printf '\#include <stdio.h>\nint main(void)
          ^
There is no need to escape the '#'.

With that fixed the system boot correctly:

Tested-by: Romain Naour <romain.naour@gmail.com>

Best regards,
Romain


> 
> Signed-off-by: Yann Sionneau <yann@sionneau.net>
> ---
>  Rules.mak                         | 4 +++-
>  ldso/ldso/powerpc/dl-startup.h    | 3 +++
>  libc/sysdeps/linux/powerpc/crt1.S | 5 ++++-
>  3 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/Rules.mak b/Rules.mak
> index 1fa09be23..0ab41e800 100644
> --- a/Rules.mak
> +++ b/Rules.mak
> @@ -58,6 +58,7 @@ LD         = $(CROSS_COMPILE)ld
>  NM         = $(CROSS_COMPILE)nm
>  OBJDUMP    = $(CROSS_COMPILE)objdump
>  STRIPTOOL  = $(CROSS_COMPILE)strip
> +READELF    = $(CROSS_COMPILE)readelf
>  
>  INSTALL    = install
>  LN         = ln
> @@ -483,9 +484,10 @@ ifeq ($(TARGET_ARCH),powerpc)
>  	PICFLAG:=-fpic
>  	PIEFLAG_NAME:=-fpie
>  	PPC_HAS_REL16:=$(shell printf "\t.text\n\taddis 11,30,_GLOBAL_OFFSET_TABLE_-.@ha\n" | $(CC) -c -x assembler -o /dev/null -  2> /dev/null && echo -n y || echo -n n)
> +	PPC_HAS_SECUREPLT:=$(shell tmpfile=$$(mktemp); printf '\#include <stdio.h>\nint main(void) { puts("hello"); return 0;}' > $${tmpfile}.c; $(CC) -x c $${tmpfile}.c -o $${tmpfile}; $(READELF) -d $${tmpfile} | grep PPC_GOT > /dev/null && echo -n y || echo -n n; rm $${tmpfile}.c $${tmpfile})
> +	CPU_CFLAGS-$(PPC_HAS_SECUREPLT) += -DPPC_HAS_SECUREPLT
>  	CPU_CFLAGS-$(PPC_HAS_REL16)+= -DHAVE_ASM_PPC_REL16
>  	CPU_CFLAGS-$(CONFIG_E500) += "-D__NO_MATH_INLINES"
> -
>  endif
>  
>  ifeq ($(TARGET_ARCH),bfin)
> diff --git a/ldso/ldso/powerpc/dl-startup.h b/ldso/ldso/powerpc/dl-startup.h
> index 8b2a517e2..7749395eb 100644
> --- a/ldso/ldso/powerpc/dl-startup.h
> +++ b/ldso/ldso/powerpc/dl-startup.h
> @@ -25,6 +25,9 @@ __asm__(
>  #else
>      "	bl	_GLOBAL_OFFSET_TABLE_-4@local\n" /*  Put our GOT pointer in r31, */
>      "	mflr	31\n"
> +#endif
> +#ifdef PPC_HAS_SECUREPLT
> +    "   mr      30,31\n"
>  #endif
>      "	addi	1,1,16\n" /* Restore SP */
>      "	lwz	7,_dl_skip_args@got(31)\n" /* load EA of _dl_skip_args */
> diff --git a/libc/sysdeps/linux/powerpc/crt1.S b/libc/sysdeps/linux/powerpc/crt1.S
> index 27bfc5a5a..78b946ad6 100644
> --- a/libc/sysdeps/linux/powerpc/crt1.S
> +++ b/libc/sysdeps/linux/powerpc/crt1.S
> @@ -47,7 +47,7 @@
>  _start:
>  	mr	r9,r1 	/* Save the stack pointer and pass it to __uClibc_main */
>  	clrrwi	r1,r1,4	/* Align stack ptr to 16 bytes */
> -#ifdef __PIC__
> +#if defined(__PIC__) || defined(PPC_HAS_SECUREPLT)
>  # ifdef HAVE_ASM_PPC_REL16
>  	bcl	20,31,1f
>  1:	mflr	r31
> @@ -57,6 +57,9 @@ _start:
>  	bl	_GLOBAL_OFFSET_TABLE_-4@local
>  	mflr	r31
>  # endif
> +#endif
> +#ifdef PPC_HAS_SECUREPLT
> +	mr	30,31
>  #endif
>  	/* Set up the small data pointer in r13.  */
>  #ifdef __PIC__
>
Yann Sionneau May 25, 2021, 7:44 a.m. UTC | #2 
Hmm my patch is not good!

Romain : I agree that the printf is currently incorrect because the \# 
ends up in the .c file which is wrong. But if I just remove the \ then 
the # is interpreted as a comment for the rest of the line in GNU/Make. 
I need to take some time to fix that. I think using " instead of ' would 
help.

Also, the patch is currently very wrong because I try to #include 
stdio.h but in a clean build since we did not yet compile uClibc-ng we 
cannot use stdio.h yet with our toolchain. I need to find out some 
different way to detect the secure plt setting. (just grepping gcc 
-dumpspecs is not so easy either because the secure-plt string is always 
present).

So, please disregard current patch it seems I need to iterate some more :)

Yann

On 24/05/2021 23:45, Romain Naour wrote:
> Hello Yann,
>
> Thanks for the help!
>
> Le 24/05/2021 à 21:02, yann@sionneau.net a écrit :
>> From: Yann Sionneau <yann@sionneau.net>
>>
>> This patch fixes segfault of all user space processes (including init, which caused a panic) on recent buildroot powerpc32 builds.
> Indeed, recent Buildroot enable by default BR2_PIC_PIE option.
>
>> Recent buildroot toolchain enables secure PLT in powerpc gcc.
>> The latter will then supply -msecure-plt to gas invocations by default.
> I guess the problem is using secure PLT and PIC/PIE at the same time. By
> disabling BR2_PIC_PIE the issue is gone.
>
> Maybe you can keep in the commit log the link to the initial report:
> https://mailman.uclibc-ng.org/pipermail/devel/2021-May/002068.html
>
>> For the secure PLT to work, the r30 register needs to point to the GOT.
>> Old "bss plt" was just a one-instruction-wide PLT slot, pointed-to by a R_PPC_JMP_SLOT relocation, which was written on-the-fly to contain a branch instruction to the correct address. It therefore had to stay writable.
>> New secure PLT only contains read-only code which loads the branch address from the writable GOT.
> With this patch applied, the init program still crash during boot:
>
> transfering control to application @ 0x690a40
> init[1]: segfault (11) at 378 nip 695e80 lr 0 code 1 in busybox[5e0000+e1000]
> init[1]: code: 817e97a4 7d6903a6 4e800420 60000000 817ebedc 7d6903a6 4e800420
> 60000000
> init[1]: code: 817ed9b8 7d6903a6 4e800420 60000000 <817e0378> 7d6903a6 4e800420
> 60000000
> Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
>
> It's due to the PPC_HAS_SECUREPLT test where the '\' charactere is still present
> in the generated source file:
>
>    printf '\#include <stdio.h>\nint main(void)
>            ^
> There is no need to escape the '#'.
>
> With that fixed the system boot correctly:
>
> Tested-by: Romain Naour <romain.naour@gmail.com>
>
> Best regards,
> Romain
>
>
>> Signed-off-by: Yann Sionneau <yann@sionneau.net>
>> ---
>>   Rules.mak                         | 4 +++-
>>   ldso/ldso/powerpc/dl-startup.h    | 3 +++
>>   libc/sysdeps/linux/powerpc/crt1.S | 5 ++++-
>>   3 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/Rules.mak b/Rules.mak
>> index 1fa09be23..0ab41e800 100644
>> --- a/Rules.mak
>> +++ b/Rules.mak
>> @@ -58,6 +58,7 @@ LD         = $(CROSS_COMPILE)ld
>>   NM         = $(CROSS_COMPILE)nm
>>   OBJDUMP    = $(CROSS_COMPILE)objdump
>>   STRIPTOOL  = $(CROSS_COMPILE)strip
>> +READELF    = $(CROSS_COMPILE)readelf
>>   
>>   INSTALL    = install
>>   LN         = ln
>> @@ -483,9 +484,10 @@ ifeq ($(TARGET_ARCH),powerpc)
>>   	PICFLAG:=-fpic
>>   	PIEFLAG_NAME:=-fpie
>>   	PPC_HAS_REL16:=$(shell printf "\t.text\n\taddis 11,30,_GLOBAL_OFFSET_TABLE_-.@ha\n" | $(CC) -c -x assembler -o /dev/null -  2> /dev/null && echo -n y || echo -n n)
>> +	PPC_HAS_SECUREPLT:=$(shell tmpfile=$$(mktemp); printf '\#include <stdio.h>\nint main(void) { puts("hello"); return 0;}' > $${tmpfile}.c; $(CC) -x c $${tmpfile}.c -o $${tmpfile}; $(READELF) -d $${tmpfile} | grep PPC_GOT > /dev/null && echo -n y || echo -n n; rm $${tmpfile}.c $${tmpfile})
>> +	CPU_CFLAGS-$(PPC_HAS_SECUREPLT) += -DPPC_HAS_SECUREPLT
>>   	CPU_CFLAGS-$(PPC_HAS_REL16)+= -DHAVE_ASM_PPC_REL16
>>   	CPU_CFLAGS-$(CONFIG_E500) += "-D__NO_MATH_INLINES"
>> -
>>   endif
>>   
>>   ifeq ($(TARGET_ARCH),bfin)
>> diff --git a/ldso/ldso/powerpc/dl-startup.h b/ldso/ldso/powerpc/dl-startup.h
>> index 8b2a517e2..7749395eb 100644
>> --- a/ldso/ldso/powerpc/dl-startup.h
>> +++ b/ldso/ldso/powerpc/dl-startup.h
>> @@ -25,6 +25,9 @@ __asm__(
>>   #else
>>       "	bl	_GLOBAL_OFFSET_TABLE_-4@local\n" /*  Put our GOT pointer in r31, */
>>       "	mflr	31\n"
>> +#endif
>> +#ifdef PPC_HAS_SECUREPLT
>> +    "   mr      30,31\n"
>>   #endif
>>       "	addi	1,1,16\n" /* Restore SP */
>>       "	lwz	7,_dl_skip_args@got(31)\n" /* load EA of _dl_skip_args */
>> diff --git a/libc/sysdeps/linux/powerpc/crt1.S b/libc/sysdeps/linux/powerpc/crt1.S
>> index 27bfc5a5a..78b946ad6 100644
>> --- a/libc/sysdeps/linux/powerpc/crt1.S
>> +++ b/libc/sysdeps/linux/powerpc/crt1.S
>> @@ -47,7 +47,7 @@
>>   _start:
>>   	mr	r9,r1 	/* Save the stack pointer and pass it to __uClibc_main */
>>   	clrrwi	r1,r1,4	/* Align stack ptr to 16 bytes */
>> -#ifdef __PIC__
>> +#if defined(__PIC__) || defined(PPC_HAS_SECUREPLT)
>>   # ifdef HAVE_ASM_PPC_REL16
>>   	bcl	20,31,1f
>>   1:	mflr	r31
>> @@ -57,6 +57,9 @@ _start:
>>   	bl	_GLOBAL_OFFSET_TABLE_-4@local
>>   	mflr	r31
>>   # endif
>> +#endif
>> +#ifdef PPC_HAS_SECUREPLT
>> +	mr	30,31
>>   #endif
>>   	/* Set up the small data pointer in r13.  */
>>   #ifdef __PIC__
>>
> _______________________________________________
> devel mailing list
> devel@uclibc-ng.org
> https://mailman.uclibc-ng.org/cgi-bin/mailman/listinfo/devel
diff mbox series

Patch

diff --git a/Rules.mak b/Rules.mak
index 1fa09be23..0ab41e800 100644
--- a/Rules.mak
+++ b/Rules.mak
@@ -58,6 +58,7 @@  LD         = $(CROSS_COMPILE)ld
 NM         = $(CROSS_COMPILE)nm
 OBJDUMP    = $(CROSS_COMPILE)objdump
 STRIPTOOL  = $(CROSS_COMPILE)strip
+READELF    = $(CROSS_COMPILE)readelf
 
 INSTALL    = install
 LN         = ln
@@ -483,9 +484,10 @@  ifeq ($(TARGET_ARCH),powerpc)
 	PICFLAG:=-fpic
 	PIEFLAG_NAME:=-fpie
 	PPC_HAS_REL16:=$(shell printf "\t.text\n\taddis 11,30,_GLOBAL_OFFSET_TABLE_-.@ha\n" | $(CC) -c -x assembler -o /dev/null -  2> /dev/null && echo -n y || echo -n n)
+	PPC_HAS_SECUREPLT:=$(shell tmpfile=$$(mktemp); printf '\#include <stdio.h>\nint main(void) { puts("hello"); return 0;}' > $${tmpfile}.c; $(CC) -x c $${tmpfile}.c -o $${tmpfile}; $(READELF) -d $${tmpfile} | grep PPC_GOT > /dev/null && echo -n y || echo -n n; rm $${tmpfile}.c $${tmpfile})
+	CPU_CFLAGS-$(PPC_HAS_SECUREPLT) += -DPPC_HAS_SECUREPLT
 	CPU_CFLAGS-$(PPC_HAS_REL16)+= -DHAVE_ASM_PPC_REL16
 	CPU_CFLAGS-$(CONFIG_E500) += "-D__NO_MATH_INLINES"
-
 endif
 
 ifeq ($(TARGET_ARCH),bfin)
diff --git a/ldso/ldso/powerpc/dl-startup.h b/ldso/ldso/powerpc/dl-startup.h
index 8b2a517e2..7749395eb 100644
--- a/ldso/ldso/powerpc/dl-startup.h
+++ b/ldso/ldso/powerpc/dl-startup.h
@@ -25,6 +25,9 @@  __asm__(
 #else
     "	bl	_GLOBAL_OFFSET_TABLE_-4@local\n" /*  Put our GOT pointer in r31, */
     "	mflr	31\n"
+#endif
+#ifdef PPC_HAS_SECUREPLT
+    "   mr      30,31\n"
 #endif
     "	addi	1,1,16\n" /* Restore SP */
     "	lwz	7,_dl_skip_args@got(31)\n" /* load EA of _dl_skip_args */
diff --git a/libc/sysdeps/linux/powerpc/crt1.S b/libc/sysdeps/linux/powerpc/crt1.S
index 27bfc5a5a..78b946ad6 100644
--- a/libc/sysdeps/linux/powerpc/crt1.S
+++ b/libc/sysdeps/linux/powerpc/crt1.S
@@ -47,7 +47,7 @@ 
 _start:
 	mr	r9,r1 	/* Save the stack pointer and pass it to __uClibc_main */
 	clrrwi	r1,r1,4	/* Align stack ptr to 16 bytes */
-#ifdef __PIC__
+#if defined(__PIC__) || defined(PPC_HAS_SECUREPLT)
 # ifdef HAVE_ASM_PPC_REL16
 	bcl	20,31,1f
 1:	mflr	r31
@@ -57,6 +57,9 @@  _start:
 	bl	_GLOBAL_OFFSET_TABLE_-4@local
 	mflr	r31
 # endif
+#endif
+#ifdef PPC_HAS_SECUREPLT
+	mr	30,31
 #endif
 	/* Set up the small data pointer in r13.  */
 #ifdef __PIC__