Hello,

Looks great :)

Comments inline:

Le 15/07/2022 à 22:26, linted a écrit :
> Sujet :
> [uclibc-ng-devel] [Patch v2] Static PIE support for i386, x86_64, and arm
> De :
> linted <linted90@gmail.com>
> Date :
> 15/07/2022, 22:26
>
> Pour :
> devel@uclibc-ng.org
>
>
> Hello,
> This is an updated patch for static pie support on i386, x86_64, and
> arm. I added dependency checks to ensure it only affects supported
> architectures. I also added a check to ensure an MMU is present on arm
> since there is no support for FDPIC yet.
>
> 0001-Added-static-pie-support-for-i368-x86_64-and-arm.patch
>
>  From 98313d17220486a75a0e552df045c6752a5ad25b Mon Sep 17 00:00:00 2001
> From: linted<linted@users.noreply.github.com>
> Date: Fri, 15 Jul 2022 16:15:14 -0400
> Subject: [PATCH] Added static pie support for i368, x86_64 and arm
Maybe add some more description of how this is done in the commit message?
>
> Signed-off-by: linted<linted@users.noreply.github.com>
> ---
>   Makerules                              |  5 ++++
>   extra/Configs/Config.in                |  7 ++++-
>   libc/misc/internals/Makefile.in        |  1 +
>   libc/misc/internals/reloc_static_pie.c | 39 ++++++++++++++++++++++++++
>   libc/sysdeps/linux/arm/crt1.S          | 15 ++++++++++
>   libc/sysdeps/linux/i386/crt1.S         | 20 +++++++++++++
>   libc/sysdeps/linux/x86_64/crt1.S       | 16 ++++++++++-
>   7 files changed, 101 insertions(+), 2 deletions(-)
>   create mode 100644 libc/misc/internals/reloc_static_pie.c
>
> diff --git a/Makerules b/Makerules
> index fd40e6c7b..845d81897 100644
> --- a/Makerules
> +++ b/Makerules
> @@ -405,8 +405,13 @@ else
>   CRTS=$(top_builddir)lib/$(CRT).o
>   endif
>   
> +ifeq ($(STATIC_PIE),y)
> +CRTS+=$(top_builddir)lib/r$(CRT).o
> +endif
> +
>   ASFLAGS-$(CRT).o := -DL_$(CRT)
>   ASFLAGS-S$(CRT).o := $(PIEFLAG) -DL_S$(CRT)
> +ASFLAGS-r$(CRT).o := $(PIEFLAG) -DL_r$(CRT)
>   $(CRTS): $(top_srcdir)libc/sysdeps/linux/$(TARGET_ARCH)/$(CRT).S
>       $(compile.S)
>       $(Q)$(STRIPTOOL) -x -R .note -R .comment $@
> diff --git a/extra/Configs/Config.in b/extra/Configs/Config.in
> index a58ceb265..ac5c494c3 100644
> --- a/extra/Configs/Config.in
> +++ b/extra/Configs/Config.in
> @@ -301,6 +301,11 @@ config DOPIC
>         If you wish to build all of uClibc as PIC objects, then answer Y here.
>         If you are unsure, then you should answer N.
>   
> +config STATIC_PIE
> +     bool "Add support for Static Position Independent Executables (PIE)"
> +     default n
> +     depends on DOPIC && !UCLIBC_FORMAT_FDPIC_ELF && (TARGET_arm || TARGET_i386 || TARGET_x86_64)
> +
>   config ARCH_HAS_NO_SHARED
>       bool
>   
> @@ -1757,7 +1762,7 @@ config UCLIBC_HAS_STDIO_AUTO_RW_TRANSITION
>         fflush function or to a file positioning function (fseek, fsetpos,
>         or rewind), and input shall not be directly followed by output without
>         an intervening call to a file positioning function, unless the input
> -       operation encounters end�of�file.
> +       operation encounters end�of�file.
This seems like a spurious modification due to the text editor?
>   
>         Most people will answer Y.
>   
> diff --git a/libc/misc/internals/Makefile.in b/libc/misc/internals/Makefile.in
> index a8e4e36f9..4a6e73d2d 100644
> --- a/libc/misc/internals/Makefile.in
> +++ b/libc/misc/internals/Makefile.in
> @@ -34,6 +34,7 @@ libc-static-$(UCLIBC_FORMAT_FLAT_SEP_DATA) += \
>   libc-static-$(UCLIBC_FORMAT_SHARED_FLAT) += \
>     $(MISC_INTERNALS_OUT)/shared_flat_initfini.o \
>     $(MISC_INTERNALS_OUT)/shared_flat_add_library.o
> +libc-static-$(STATIC_PIE) += $(MISC_INTERNALS_OUT)/reloc_static_pie.o
>   libc-shared-$(UCLIBC_FORMAT_SHARED_FLAT) += \
>     $(MISC_INTERNALS_OUT)/shared_flat_initfini.os \
>     $(MISC_INTERNALS_OUT)/shared_flat_add_library.os
> diff --git a/libc/misc/internals/reloc_static_pie.c b/libc/misc/internals/reloc_static_pie.c
> new file mode 100644
> index 000000000..0736bc8a1
> --- /dev/null
> +++ b/libc/misc/internals/reloc_static_pie.c
> @@ -0,0 +1,39 @@
> +/* Support for relocating static PIE.
> +   Copyright (C) 2017-2022 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 <link.h>
> +#include <elf.h>
> +#include <dl-elf.h>
> +
> +void
> +reloc_static_pie (ElfW(Addr) load_addr)
> +{
> +    /* Read our own dynamic section and fill in the info array.  */
> +    ElfW(Dyn) * dyn_addr = ((void *) load_addr + elf_machine_dynamic ());
> +
> +    unsigned long dynamic_info[36] = {0};

Could you use some generic define for the size of dynamic_info instead
of the hard coded 36?

Like DYNAMIC_SIZE for instance? (I haven't checked if it's the correct
value for you)

> +
> +    /* Use the underlying function to avoid TLS access before initialization */
> +    __dl_parse_dynamic_info(dyn_addr, dynamic_info, NULL, load_addr);
> +
> +    ElfW(Word) relative_count = dynamic_info[DT_RELCONT_IDX];
> +    ElfW(Addr) rel_addr = dynamic_info[DT_RELOC_TABLE_ADDR];
Maybe declare relative_count and rel_addr at the beginning of the function?
> +    elf_machine_relative(load_addr, rel_addr, relative_count);
> +
> +    return;
I would remove the return.
> +}
> diff --git a/libc/sysdeps/linux/arm/crt1.S b/libc/sysdeps/linux/arm/crt1.S
> index a1d7f0f23..adaf4b983 100644
> --- a/libc/sysdeps/linux/arm/crt1.S
> +++ b/libc/sysdeps/linux/arm/crt1.S
> @@ -246,6 +246,18 @@ _start:
>       mov lr, #0
>   
>   #ifdef __ARCH_USE_MMU__
> +#ifdef L_rcrt1
> +     /* We don't need to save a1 since no dynamic linker should have run */
> +     ldr a1, .L_GOT          /* Get value at .L_GOT + 0  (offset to GOT)*/
> +     adr a2, .L_GOT          /* Get address of .L_GOT */
> +     ldr a3, .L_GOT+16       /* get value of _start(GOT) stored in .L_GOT */
> +     adr a4, _start          /* get address of _start after relocation (changes to pc - ~30 or so) */
> +     add a1, a1, a2          /* calculate where the GOT is */
> +     ldr a2, [a1, a3]        /* GOT + _start(GOT) = offset of _start from begin of file */
> +     sub a1, a4, a2          /* current addr of _start - offset from beginning of file = load addr */
> +     bl reloc_static_pie
> +     mov a1, #0                      /* Clean up a1 so that a random address won't get called at the end of program */
> +#endif
>       /* Pop argc off the stack and save a pointer to argv */
>       ldr a2, [sp], #4
>       mov a3, sp
> @@ -309,6 +321,9 @@ _start:
>       .word _fini(GOT)
>       .word _init(GOT)
>       .word main(GOT)
> +#ifdef L_rcrt1
> +     .word _start(GOT)
> +#endif
>   #endif
>   #endif
>   
> diff --git a/libc/sysdeps/linux/i386/crt1.S b/libc/sysdeps/linux/i386/crt1.S
> index 35a6552e8..4e4cef3ce 100644
> --- a/libc/sysdeps/linux/i386/crt1.S
> +++ b/libc/sysdeps/linux/i386/crt1.S
> @@ -67,6 +67,9 @@
>   #endif
>   .type   main,%function
>   .type   __uClibc_main,%function
> +#ifdef L_rcrt1
> +.type        reloc_static_pie,%function
> +#endif
>   _start:
>       /* Clear the frame pointer.  The ABI suggests this be done, to mark
>          the outermost frame obviously.  */
> @@ -100,6 +103,23 @@ _start:
>       pop %ebx
>       addl $_GLOBAL_OFFSET_TABLE_+[.-.L0],%ebx
>   
> +#ifdef L_rcrt1
> +     /* We cannot rely on _DYNAMIC being usable here due to RELRO.
> +        Instead we calculate the load address based off a symbol
> +        that we know will exist, _start. */
nitpick: the comments of the assembly code for arm crt1.S are well
aligned whereas those for i386/x86_64 are not well aligned.
> +     pushl %ecx                                              /* Save ecx so it won't get clobbered */
> +     pushl %ebx                                              /* Save ebx so it won't get clobbered */
> +     xorl %ecx, %ecx                                 /* Clear ecx */
> +     addl _start@GOT(%ebx), %ecx     /* Get the offset of _start */
> +     movl _start@GOT(%ebx), %eax     /* Get the run time address of _start */
> +     subl %ecx, %eax                                 /* Subtract to find the load address */
> +     pushl %eax                                              /* Pass the load address */
> +     call reloc_static_pie@PLT
> +     popl %eax                                               /* Clean up from function call */
> +     popl %ebx                                               /* Restore the GOT address */
> +     popl %ecx                                               /* restore ecx */
> +#endif
> +
>       /* Push address of our own entry points to .fini and .init.  */
>       pushl _fini@GOT(%ebx)
>       pushl _init@GOT(%ebx)
> diff --git a/libc/sysdeps/linux/x86_64/crt1.S b/libc/sysdeps/linux/x86_64/crt1.S
> index 87777dd5d..04536d07f 100644
> --- a/libc/sysdeps/linux/x86_64/crt1.S
> +++ b/libc/sysdeps/linux/x86_64/crt1.S
> @@ -80,6 +80,20 @@ _start:
>          the outermost frame obviously.  */
>       xorl %ebp, %ebp
>   
> +#ifdef L_rcrt1
> +     pushq %rdi                                                      /* save rdi (but should be 0...) */
> +     pushq %rdx                                                      /* store rdx (rtld_fini) */
> +     xorq %rcx, %rcx                                         /* ensure rcx is 0 */
> +     addq _start@GOTPCREL(%rip), %rcx        /* get offset of _start from beginning of file */
> +     movq _start@GOTPCREL(%rip), %rax        /* get run time address of _start */
> +     subq %rcx, %rax                                         /* calculate run time load offset */
> +     movq %rax, %rdi                                         /* load offset -> param 1 */
> +     call reloc_static_pie                           /* relocate dynamic addrs */
> +     xorq %rax, %rax                                         /* cleanup */
> +     popq %rdx
> +     popq %rdi
> +#endif
> +
>       /* Extract the arguments as encoded on the stack and set up
>          the arguments for __libc_start_main (int (*main) (int, char **, char **),
>                  int argc, char *argv,
> @@ -107,7 +121,7 @@ _start:
>          which grow downwards).  */
>       pushq %rsp
>   
> -#if defined(L_Scrt1)
> +#if defined(L_Scrt1) || defined(L_rcrt1)
>       /* Give address for main() */
>       movq main@GOTPCREL(%rip), %rdi
>   
> -- 2.34.1
>
> _______________________________________________
> devel mailing list --devel@uclibc-ng.org
> To unsubscribe send an email todevel-leave@uclibc-ng.org
> Pièces jointes :
>
> 0001-Added-static-pie-support-for-i368-x86_64-and-arm.patch   8,2 Ko
>
_______________________________________________
devel mailing list -- devel@uclibc-ng.org
To unsubscribe send an email to devel-leave@uclibc-ng.org