Re: [PATCH v2] dl: Use "adr" assembler command to get proper load address on ARM

[Lukasz Majewski <lukma@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 7521 bytes --]

Hi Szabolcs,

> The 10/15/2021 09:54, Lukasz Majewski wrote:
> > This change is a partial revert of commit
> > bca0f5cbc9257c13322b99e55235c4f21ba0bd82
> > "arm: Simplify elf_machine_{load_address,dynamic}" which imposed
> > usage of __ehdr_start linker variable to get the address of loaded
> > program.
> > 
> > The elf_machine_load_address() function is declared in the
> > sysdeps/arm/dl-machine.h header. It is called from (very early)
> > _dl_start() entry point for the program. It shall return the load
> > address of the dynamic linker program.
> > 
> > With this revert the 'adr' assembler instruction is used instead of
> > a place holder:
> > 
> > arm-poky-linux-gnueabi-objdump -t ld-linux-armhf.so.3 | grep ehdr
> > 00000000 l       .note.gnu.build-id     00000000      __ehdr_start
> > 
> > which is pre-set by binutils.
> > 
> > The problem starts when one runs 'prelink' on the rootfs created
> > with for example OE/Yocto.
> > Then the _ehdr_start stays as 0x0, but the ELF header's sections
> > have different addresses - for example 0x41000000 instead of the
> > originally set 0x0.
> > 
> > This is crucial when /sbin/init is executed. Value set in
> > __ehdr_start symbol is not updated. This causes the program to
> > crash very early when ld-linux-armhf.so.3's _dl_start is executed,
> > as calculated offset for loader relocation is going to hit the
> > kernel space (0xf7xxyyyy).
> > 
> > It looks like the correct way to obtain the _dl_start offset on ARM
> > is to use assembler instruction 'adr' at execution time (so the
> > prelink assigned offset is taken into consideration) instead of
> > __ehdr_start.
> > 
> > With this patch we only modify the elf_machine_load_address()
> > function, as it is called very early, before the
> > ld-linux-armhf.so.3 is performing relocation (also its own one).  
> 
> i'd use an explanation like:
> 
> __ehdr_start is a linker created symbol that points to the elf header.
> The elf header is at the beginning of the elf file and normally its
> virtual address is 0 in a shared library.  This means the runtime
> address of __ehdr_start is the load address of the module.  However if
> prelinking is applied to ld.so then all virtual addresses are moved by
> an offset so the runtime address of the elf header becomes the load
> address + prelink offset.  The kernel does not treat prelinked ld.so
> specially so the load address is not 0, it still has to be computed,
> but simply using __ehdr_start no longer gives a correct value for
> that.
> 
> This issue affects all targets with prelinking support, but so far we
> only got reports from OE/Yocto builds for arm that has prelinked
> ld.so.
> 

Thanks for a very detailed description.

> but i think a better fix is possible than revert:
> 
> ElfW(Addr)
> elf_machine_load_address ()
> {
>   extern ElfW(Dyn) _DYNAMIC[] attribute_hidden;
>   extern ElfW(Dyn) extern_DYNAMIC[] asm ("_DYNAMIC");
> 

So the _DYNAMIC = GOT[0] (and it points into the .dynamic section)
objdump -d -j .got ld-linux-armhf.so.3

Disassembly of section .got:

41036fbc <.got+0x41000000>:
41036fbc:       41036ef4        .word   0x41036ef4

So it indeed points into the
  [16] .dynamic          DYNAMIC         41036ef4

>   /* Uses pc-relative address computation.  */
>   ElfW(Addr) runtime_addr = (ElfW(Addr)) &_DYNAMIC;

I guess that the &_DYNAMIC gives the address around which $pc runs 
(e.g. 0xb6fc9504) - this is the actual address of run program.

(A side question - is there any way to read the _DYNAMIC symbol value
directly - via e.g. readelf or objdump?)

> 
>   /* Loads an unrelocated GOT entry.  */
>   ElfW(Addr) linktime_addr = (ElfW(Addr)) &extern_DYNAMIC;
> 

This is the prelink'ed address -> 0x41036ef4 in our case?

>   return runtime_addr - linktime_addr;

And the address to which we shall relocated would be:

0xb6fc9504 - 0x41036ef4 = 0x75f92610 - which is the address to which
the ld.so (ld-linux-armhf.so.3) will re-relocate itself?

> }
> 
> I expect this to work on most targets and very similar to the code
> that was originally used on other targets: only a new GOT entry is
> introduced instead of using GOT[0].

In fact we only rely on _DYNAMIC symbol -> which points into .dynamic
section.

> (that new got entry will have a
> relative relocation which means there must be a dynamic section even
> in a static PIE, so i expect _DYNAMIC to be defined.

Ok.

> this also means
> that it's slightly more expensive than &__ehdr_start, so it is for
> targets that want to support prelinked ld.so)
> 
> The original arm code used _dl_start symbol, likely because that's
> within range for the adr instruction for more efficient pc-relative
> computation. But that's a function symbol that requires fixups due
> to thumb interworking issues and is not available in static PIE, so
> using _DYNAMIC sounds better even on arm.

+1.

> 
> > 
> > HW:
> > Hardware name:
> > 	- ARM-Versatile Express (Run with QEMU)
> > 	- Beagle Bone Black
> > 
> > Build Environment: OE/Yocto -> poky
> > SHA1: 1e2e9a84d6dd81d7f6dd69c0d119d0149d10ade1
> > 
> > Fixes: BZ #28293
> > ---
> >  sysdeps/arm/dl-machine.h | 28 +++++++++++++++++++++++++---
> >  1 file changed, 25 insertions(+), 3 deletions(-)
> > 
> > diff --git a/sysdeps/arm/dl-machine.h b/sysdeps/arm/dl-machine.h
> > index dfa05eee44..d6e5f1d5ec 100644
> > --- a/sysdeps/arm/dl-machine.h
> > +++ b/sysdeps/arm/dl-machine.h
> > @@ -39,11 +39,33 @@ elf_machine_matches_host (const Elf32_Ehdr
> > *ehdr) }
> >  
> >  /* Return the run-time load address of the shared object.  */
> > -static inline ElfW(Addr) __attribute__ ((unused))
> > +static inline Elf32_Addr __attribute__ ((unused))
> >  elf_machine_load_address (void)
> >  {
> > -  extern const ElfW(Ehdr) __ehdr_start attribute_hidden;
> > -  return (ElfW(Addr)) &__ehdr_start;
> > +  Elf32_Addr pcrel_addr;
> > +#ifdef SHARED
> > +  extern Elf32_Addr __dl_start (void *) asm ("_dl_start");
> > +  Elf32_Addr got_addr = (Elf32_Addr) &__dl_start;
> > +  asm ("adr %0, _dl_start" : "=r" (pcrel_addr));
> > +#else
> > +  extern Elf32_Addr __dl_relocate_static_pie (void *)
> > +    asm ("_dl_relocate_static_pie") attribute_hidden;
> > +  Elf32_Addr got_addr = (Elf32_Addr) &__dl_relocate_static_pie;
> > +  asm ("adr %0, _dl_relocate_static_pie" : "=r" (pcrel_addr));
> > +#endif
> > +#ifdef __thumb__
> > +  /* Clear the low bit of the function address.
> > +
> > +     NOTE: got_addr is from GOT table whose lsb is always set by
> > linker if it's
> > +     Thumb function address.  PCREL_ADDR comes from PC-relative
> > calculation
> > +     which will finish during assembling.  GAS assembler before
> > the fix for
> > +     PR gas/21458 was not setting the lsb but does after that.
> > Always do the
> > +     strip for both, so the code works with various combinations
> > of glibc and
> > +     Binutils.  */
> > +  got_addr &= ~(Elf32_Addr) 1;
> > +  pcrel_addr &= ~(Elf32_Addr) 1;
> > +#endif
> > +  return pcrel_addr - got_addr;
> >  }
> >  
> >  /* Return the link-time address of _DYNAMIC.  */
> > -- 
> > 2.20.1
> >   




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]