diff mbox series

riscv: add support for static PIE

Message ID mvmplxzvv2j.fsf@suse.de
State New
Headers show
Series riscv: add support for static PIE | expand

Commit Message

Andreas Schwab Jan. 18, 2024, 9:40 a.m. UTC 
In order to support static PIE the startup code must avoid relocations
before __libc_start_main is called.
---
 sysdeps/riscv/start.S | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Adhemerval Zanella Netto Jan. 18, 2024, 8:48 p.m. UTC | #1 
On 18/01/24 06:40, Andreas Schwab wrote:
> In order to support static PIE the startup code must avoid relocations
> before __libc_start_main is called.
> ---
>  sysdeps/riscv/start.S | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/sysdeps/riscv/start.S b/sysdeps/riscv/start.S
> index 0a1f713742..ede186ef23 100644
> --- a/sysdeps/riscv/start.S
> +++ b/sysdeps/riscv/start.S
> @@ -50,7 +50,13 @@ ENTRY (ENTRY_POINT)
>  	call  load_gp
>  	mv    a5, a0  /* rtld_fini.  */
>  	/* main may be in a shared library.  */
> +#if defined PIC && !defined SHARED
> +	/* Avoid relocation in static PIE since _start is called before it
> +	   is relocated.  */
> +	lla   a0, __wrap_main
> +#else
>  	la   a0, main
> +#endif
>  	REG_L a1, 0(sp)      /* argc.  */
>  	addi  a2, sp, SZREG  /* argv.  */
>  	andi  sp, sp, ALMASK /* Align stack. */
> @@ -62,6 +68,11 @@ ENTRY (ENTRY_POINT)
>  	ebreak
>  END (ENTRY_POINT)
>  
> +#if defined PIC && !defined SHARED
> +__wrap_main:
> +	tail  main@plt
> +#endif
> +
>  /* Dynamic links need the global pointer to be initialized prior to calling
>     any shared library's initializers, so we use preinit_array to load it.
>     This doesn't cut it for static links, though, since the global pointer

I think you also need to prevent the stack protector on memset:

diff --git a/sysdeps/riscv/Makefile b/sysdeps/riscv/Makefile
index c08753ae8a..3a324d2f56 100644
--- a/sysdeps/riscv/Makefile
+++ b/sysdeps/riscv/Makefile
@@ -15,3 +15,10 @@ ASFLAGS-.os += -Wa,-mno-relax
 ASFLAGS-.o += -Wa,-mno-relax
 sysdep-CFLAGS += -mno-relax
 endif
+
+ifeq ($(subdir),string)
+# compiler might generate libcalls on code where the stack protector is not yet
+# initialized.
+CFLAGS-memset.o += $(no-stack-protector)
+CFLAGS-memset.op += $(no-stack-protector)
+endif

Otherwise the startup code will trigger an invalid memory access (since the
stack protector is not yet initialized):

$ gdb ./elf/ldconfig
[...]
(gdb) r
[...]
(gdb) disas
Dump of assembler code for function memset:
   0x00007ffff7f6e3ec <+0>:     addi    sp,sp,-32
   0x00007ffff7f6e3ee <+2>:     auipc   a7,0x89
   0x00007ffff7f6e3f2 <+6>:     ld      a7,2018(a7) # 0x7ffff7ff7bd0
=> 0x00007ffff7f6e3f6 <+10>:    ld      a5,0(a7)

(gdb) bt
#0  0x00007ffff7f6e3f6 in memset (dstpp=dstpp@entry=0x7fffffffeca0, c=c@entry=0, len=len@entry=416) at memset.c:28
#1  0x00007ffff7f7a9a4 in _dl_aux_init (av=0x7ffffffff030) at dl-support.c:242
#2  0x00007ffff7f572e8 in __libc_start_main_impl (main=0x7ffff7f52e02 <__wrap_main>, argc=1, argv=0x7fffffffeed8, init=<optimized out>, fini=<optimized out>, rtld_fini=0x0,
    stack_end=<optimized out>) at libc-start.c:264
#3  0x00007ffff7f52e00 in _start () at ../sysdeps/riscv/start.S:67
Andreas Schwab Jan. 22, 2024, 9:35 a.m. UTC | #2 
On Jan 18 2024, Adhemerval Zanella Netto wrote:

> Otherwise the startup code will trigger an invalid memory access (since the
> stack protector is not yet initialized):
>
> $ gdb ./elf/ldconfig
> [...]
> (gdb) r
> [...]
> (gdb) disas
> Dump of assembler code for function memset:
>    0x00007ffff7f6e3ec <+0>:     addi    sp,sp,-32
>    0x00007ffff7f6e3ee <+2>:     auipc   a7,0x89
>    0x00007ffff7f6e3f2 <+6>:     ld      a7,2018(a7) # 0x7ffff7ff7bd0
> => 0x00007ffff7f6e3f6 <+10>:    ld      a5,0(a7)

I don't see that with -fstack-protector-strong.
Adhemerval Zanella Netto Jan. 22, 2024, 12:20 p.m. UTC | #3 
On 22/01/24 06:35, Andreas Schwab wrote:
> On Jan 18 2024, Adhemerval Zanella Netto wrote:
> 
>> Otherwise the startup code will trigger an invalid memory access (since the
>> stack protector is not yet initialized):
>>
>> $ gdb ./elf/ldconfig
>> [...]
>> (gdb) r
>> [...]
>> (gdb) disas
>> Dump of assembler code for function memset:
>>    0x00007ffff7f6e3ec <+0>:     addi    sp,sp,-32
>>    0x00007ffff7f6e3ee <+2>:     auipc   a7,0x89
>>    0x00007ffff7f6e3f2 <+6>:     ld      a7,2018(a7) # 0x7ffff7ff7bd0
>> => 0x00007ffff7f6e3f6 <+10>:    ld      a5,0(a7)
> 
> I don't see that with -fstack-protector-strong.
> 

It happens with -fstack-protector-all, which although it does not make much
sense to be used it is still supported.
Andreas Schwab Jan. 22, 2024, 12:28 p.m. UTC | #4 
On Jan 22 2024, Adhemerval Zanella Netto wrote:

> On 22/01/24 06:35, Andreas Schwab wrote:
>> On Jan 18 2024, Adhemerval Zanella Netto wrote:
>> 
>>> Otherwise the startup code will trigger an invalid memory access (since the
>>> stack protector is not yet initialized):
>>>
>>> $ gdb ./elf/ldconfig
>>> [...]
>>> (gdb) r
>>> [...]
>>> (gdb) disas
>>> Dump of assembler code for function memset:
>>>    0x00007ffff7f6e3ec <+0>:     addi    sp,sp,-32
>>>    0x00007ffff7f6e3ee <+2>:     auipc   a7,0x89
>>>    0x00007ffff7f6e3f2 <+6>:     ld      a7,2018(a7) # 0x7ffff7ff7bd0
>>> => 0x00007ffff7f6e3f6 <+10>:    ld      a5,0(a7)
>> 
>> I don't see that with -fstack-protector-strong.
>> 
>
> It happens with -fstack-protector-all, which although it does not make much
> sense to be used it is still supported.

I think it can be part of a separate commit.  Care to provide one?
Adhemerval Zanella Netto Jan. 22, 2024, 12:42 p.m. UTC | #5 
On 22/01/24 09:28, Andreas Schwab wrote:
> On Jan 22 2024, Adhemerval Zanella Netto wrote:
> 
>> On 22/01/24 06:35, Andreas Schwab wrote:
>>> On Jan 18 2024, Adhemerval Zanella Netto wrote:
>>>
>>>> Otherwise the startup code will trigger an invalid memory access (since the
>>>> stack protector is not yet initialized):
>>>>
>>>> $ gdb ./elf/ldconfig
>>>> [...]
>>>> (gdb) r
>>>> [...]
>>>> (gdb) disas
>>>> Dump of assembler code for function memset:
>>>>    0x00007ffff7f6e3ec <+0>:     addi    sp,sp,-32
>>>>    0x00007ffff7f6e3ee <+2>:     auipc   a7,0x89
>>>>    0x00007ffff7f6e3f2 <+6>:     ld      a7,2018(a7) # 0x7ffff7ff7bd0
>>>> => 0x00007ffff7f6e3f6 <+10>:    ld      a5,0(a7)
>>>
>>> I don't see that with -fstack-protector-strong.
>>>
>>
>> It happens with -fstack-protector-all, which although it does not make much
>> sense to be used it is still supported.
> 
> I think it can be part of a separate commit.  Care to provide one?
> 

Alright, I will send it shortly.
Florian Weimer Jan. 22, 2024, 1:14 p.m. UTC | #6 
* Adhemerval Zanella Netto:

>>> It happens with -fstack-protector-all, which although it does not make much
>>> sense to be used it is still supported.
>> 
>> I think it can be part of a separate commit.  Care to provide one?
>> 
>
> Alright, I will send it shortly.

Please put it alongside the generic implementation.  I believe every
architecture using it needs to disable stack protector.

Thanks,
Florian
Adhemerval Zanella Netto Jan. 22, 2024, 1:45 p.m. UTC | #7 
On 18/01/24 06:40, Andreas Schwab wrote:
> In order to support static PIE the startup code must avoid relocations
> before __libc_start_main is called.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  sysdeps/riscv/start.S | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/sysdeps/riscv/start.S b/sysdeps/riscv/start.S
> index 0a1f713742..ede186ef23 100644
> --- a/sysdeps/riscv/start.S
> +++ b/sysdeps/riscv/start.S
> @@ -50,7 +50,13 @@ ENTRY (ENTRY_POINT)
>  	call  load_gp
>  	mv    a5, a0  /* rtld_fini.  */
>  	/* main may be in a shared library.  */
> +#if defined PIC && !defined SHARED
> +	/* Avoid relocation in static PIE since _start is called before it
> +	   is relocated.  */
> +	lla   a0, __wrap_main
> +#else
>  	la   a0, main
> +#endif
>  	REG_L a1, 0(sp)      /* argc.  */
>  	addi  a2, sp, SZREG  /* argv.  */
>  	andi  sp, sp, ALMASK /* Align stack. */
> @@ -62,6 +68,11 @@ ENTRY (ENTRY_POINT)
>  	ebreak
>  END (ENTRY_POINT)
>  
> +#if defined PIC && !defined SHARED
> +__wrap_main:
> +	tail  main@plt
> +#endif
> +
>  /* Dynamic links need the global pointer to be initialized prior to calling
>     any shared library's initializers, so we use preinit_array to load it.
>     This doesn't cut it for static links, though, since the global pointer
diff mbox series

Patch

diff --git a/sysdeps/riscv/start.S b/sysdeps/riscv/start.S
index 0a1f713742..ede186ef23 100644
--- a/sysdeps/riscv/start.S
+++ b/sysdeps/riscv/start.S
@@ -50,7 +50,13 @@  ENTRY (ENTRY_POINT)
 	call  load_gp
 	mv    a5, a0  /* rtld_fini.  */
 	/* main may be in a shared library.  */
+#if defined PIC && !defined SHARED
+	/* Avoid relocation in static PIE since _start is called before it
+	   is relocated.  */
+	lla   a0, __wrap_main
+#else
 	la   a0, main
+#endif
 	REG_L a1, 0(sp)      /* argc.  */
 	addi  a2, sp, SZREG  /* argv.  */
 	andi  sp, sp, ALMASK /* Align stack. */
@@ -62,6 +68,11 @@  ENTRY (ENTRY_POINT)
 	ebreak
 END (ENTRY_POINT)
 
+#if defined PIC && !defined SHARED
+__wrap_main:
+	tail  main@plt
+#endif
+
 /* Dynamic links need the global pointer to be initialized prior to calling
    any shared library's initializers, so we use preinit_array to load it.
    This doesn't cut it for static links, though, since the global pointer