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

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

[Lukasz Majewski]

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

On Fri, 15 Oct 2021 05:21:23 -0700
"H.J. Lu" <hjl.tools@gmail.com> wrote:

> On Fri, Oct 15, 2021 at 5:09 AM Szabolcs Nagy via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
> >
> > 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.
> >
> > but i think a better fix is possible than revert:  
> 
> I think either prelink should be fixed not to prelink ld.so or Yocto
> should be fixed not to prelink ld.so.
> 

Could you explain why?

Was the relocation of ld.so (I guess that ld.so = ld-linux-arm.so) a
bug from the very beginning and it was apparent just now?

From my point of view - the original change to use __ehdr_start broke
working setups, so it is a regression and shall be fixed in glibc.

Anyway, it would be beneficial to have input from other glibc
developers how to proceed with this issue.


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 --]

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

[Lukasz Majewski]

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

Hi Florian,

> * Szabolcs Nagy:
> 
> > i don't know much about pelinking, but i'd expect that ld.so
> > has to be prelinked for it to work:
> >
> > if the kernel can load ld.so anywhere it will conflict with
> > other libraries that prelinking allocated to a fixed location.  
> 
> I think ld.so can back out prelinking if it detects any conflicts.
> (ld.so doesn't use MAP_FIXED for the initial ET_DYN mapping even when
> prelinking.)
> 
> > instead ld.so has to be prelinked to an offset that comes after
> > all other prelinked libraries in the system, then the kernel
> > will place it after all other libraries at runtime.
> >
> > i don't have a prelinked system to check if this is the case.  
> 
> I tried on glibc 2.12-based system with prelink enabled and got this:
> 
> # ldd /bin/bash
> 	linux-vdso.so.1 =>  (0x00007ffc7e798000)
> 	libtinfo.so.5 => /lib64/libtinfo.so.5 (0x0000003da9800000)
> 	libdl.so.2 => /lib64/libdl.so.2 (0x0000003da7400000)
> 	libc.so.6 => /lib64/libc.so.6 (0x0000003da7800000)
> 	/lib64/ld-linux-x86-64.so.2 (0x00007f8dc919c000)
> # ldd /bin/bash
> 	linux-vdso.so.1 =>  (0x00007ffef3bf4000)
> 	libtinfo.so.5 => /lib64/libtinfo.so.5 (0x0000003da9800000)
> 	libdl.so.2 => /lib64/libdl.so.2 (0x0000003da7400000)
> 	libc.so.6 => /lib64/libc.so.6 (0x0000003da7800000)
> 	/lib64/ld-linux-x86-64.so.2 (0x00007ff9e66a6000)
> # eu-readelf -d /lib64/ld-linux-x86-64.so.2
> 
> Dynamic segment contains 25 entries:
>  Addr: 0x0000003da7220df0  Offset: 0x020df0  Link to section: [ 5]
> '.dynstr' Type              Value
>   SONAME            Library soname: [ld-linux-x86-64.so.2]
>   HASH              0x0000003da70001f0
>   GNU_HASH          0x0000003da70002a8
>   STRTAB            0x0000003da7000608
>   SYMTAB            0x0000003da7000380
>   STRSZ             380 (bytes)
>   SYMENT            24 (bytes)
>   PLTGOT            0x0000003da7220f80
>   PLTRELSZ          144 (bytes)
>   PLTREL            RELA
>   JMPREL            0x0000003da7000a30
>   RELA              0x0000003da7000868
>   RELASZ            456 (bytes)
>   RELAENT           24 (bytes)
>   VERDEF            0x0000003da70007c0
>   VERDEFNUM         5
>   BIND_NOW          
>   FLAGS_1           NOW
>   VERSYM            0x0000003da7000784
>   RELACOUNT         16
>   CHECKSUM          0x00000000e90e92bc
>   GNU_PRELINKED     0x00000000616d5a26
>   NULL              
>   NULL              
>   NULL
> # 
> 
> As expected based on the previous discussion here, the kernel maps
> ld.so at random addresses even though it has been prelinked.
> 
> This looks like another place where ASLR layout as to be tweaked
> carefully to avoid obscure failure modes.

Is the approach proposed by Szabolcs acceptable for 32 bit ARM? Or
shall we look for other way to proceed with this issue?

> 
> Thanks,
> Florian
> 


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 --]

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

[Lukasz Majewski]

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

Dear Community,

> * Szabolcs Nagy:
> 
> > i don't know much about pelinking, but i'd expect that ld.so
> > has to be prelinked for it to work:
> >
> > if the kernel can load ld.so anywhere it will conflict with
> > other libraries that prelinking allocated to a fixed location.  
> 
> I think ld.so can back out prelinking if it detects any conflicts.
> (ld.so doesn't use MAP_FIXED for the initial ET_DYN mapping even when
> prelinking.)
> 
> > instead ld.so has to be prelinked to an offset that comes after
> > all other prelinked libraries in the system, then the kernel
> > will place it after all other libraries at runtime.
> >
> > i don't have a prelinked system to check if this is the case.  
> 
> I tried on glibc 2.12-based system with prelink enabled and got this:
> 
> # ldd /bin/bash
> 	linux-vdso.so.1 =>  (0x00007ffc7e798000)
> 	libtinfo.so.5 => /lib64/libtinfo.so.5 (0x0000003da9800000)
> 	libdl.so.2 => /lib64/libdl.so.2 (0x0000003da7400000)
> 	libc.so.6 => /lib64/libc.so.6 (0x0000003da7800000)
> 	/lib64/ld-linux-x86-64.so.2 (0x00007f8dc919c000)
> # ldd /bin/bash
> 	linux-vdso.so.1 =>  (0x00007ffef3bf4000)
> 	libtinfo.so.5 => /lib64/libtinfo.so.5 (0x0000003da9800000)
> 	libdl.so.2 => /lib64/libdl.so.2 (0x0000003da7400000)
> 	libc.so.6 => /lib64/libc.so.6 (0x0000003da7800000)
> 	/lib64/ld-linux-x86-64.so.2 (0x00007ff9e66a6000)
> # eu-readelf -d /lib64/ld-linux-x86-64.so.2
> 
> Dynamic segment contains 25 entries:
>  Addr: 0x0000003da7220df0  Offset: 0x020df0  Link to section: [ 5]
> '.dynstr' Type              Value
>   SONAME            Library soname: [ld-linux-x86-64.so.2]
>   HASH              0x0000003da70001f0
>   GNU_HASH          0x0000003da70002a8
>   STRTAB            0x0000003da7000608
>   SYMTAB            0x0000003da7000380
>   STRSZ             380 (bytes)
>   SYMENT            24 (bytes)
>   PLTGOT            0x0000003da7220f80
>   PLTRELSZ          144 (bytes)
>   PLTREL            RELA
>   JMPREL            0x0000003da7000a30
>   RELA              0x0000003da7000868
>   RELASZ            456 (bytes)
>   RELAENT           24 (bytes)
>   VERDEF            0x0000003da70007c0
>   VERDEFNUM         5
>   BIND_NOW          
>   FLAGS_1           NOW
>   VERSYM            0x0000003da7000784
>   RELACOUNT         16
>   CHECKSUM          0x00000000e90e92bc
>   GNU_PRELINKED     0x00000000616d5a26
>   NULL              
>   NULL              
>   NULL
> # 
> 
> As expected based on the previous discussion here, the kernel maps
> ld.so at random addresses even though it has been prelinked.
> 
> This looks like another place where ASLR layout as to be tweaked
> carefully to avoid obscure failure modes.
> 

Do we have any idea on how to move forward with this issue?

> Thanks,
> Florian
> 

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 --]

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

[Lukasz Majewski]

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

Hi Florian,

> * Lukasz Majewski:
> 
> > Do we have any idea on how to move forward with this issue?  
> 
> Either fix the prelink tool not to prelink shared objects that do not
> have a dependency on libc.so.6, or fix the dynamic loader to work if
> prelinked on AArch64.

Just for the correctness - both 64 and 32 bit ARMs are affected.

> I do not have a strong opinion.

Thanks for your opinion. Let's wait for other community members
opinions.

> 
> Thanks,
> Florian
> 



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 --]

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

[Lukasz Majewski]

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

Hi Szabolcs,

> The 10/25/2021 12:53, Lukasz Majewski wrote:
> > Hi Florian,
> >   
> > > * Lukasz Majewski:
> > >   
> > > > Do we have any idea on how to move forward with this issue?    
> > > 
> > > Either fix the prelink tool not to prelink shared objects that do
> > > not have a dependency on libc.so.6, or fix the dynamic loader to
> > > work if prelinked on AArch64.  
> > 
> > Just for the correctness - both 64 and 32 bit ARMs are affected.  
> 
> last time i looked, prelinking did not support tlsdesc
> correctly so it is unusable for aarch64.
> 
> does yocto/oe use prelinking on aarch64?

I think yes - the 
USER_CLASSES ?= "buildstats image-prelink"

is added by default to local.conf

> 
> >   
> > > I do not have a strong opinion.  
> > 
> > Thanks for your opinion. Let's wait for other community members
> > opinions.  
> 
> i think fixing the arm load address computation makes
> sense (small extra cost of a relative reloc). i think
> the c code proposal i made in the thread is nicer than
> the old asm.
> 
> (i'm happy to make the same change on aarch64 too if
> prelinking is used there, but i think that's broken.)

+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 --]

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

[Lukasz Majewski]

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

Hi Szabolcs,

> The 10/25/2021 16:04, Lukasz Majewski wrote:
> > > > > Either fix the prelink tool not to prelink shared objects
> > > > > that do not have a dependency on libc.so.6, or fix the
> > > > > dynamic loader to work if prelinked on AArch64.    
> > > > 
> > > > Just for the correctness - both 64 and 32 bit ARMs are
> > > > affected.    
> > > 
> > > last time i looked, prelinking did not support tlsdesc
> > > correctly so it is unusable for aarch64.
> > > 
> > > does yocto/oe use prelinking on aarch64?  
> > 
> > I think yes - the 
> > USER_CLASSES ?= "buildstats image-prelink"
> > 
> > is added by default to local.conf  
> 
> ok, i think we need the patches upstream for that like
> https://sourceware.org/pipermail/libc-alpha/2015-November/066153.html
> 

Oh... I see.

> > > > > I do not have a strong opinion.    
> > > > 
> > > > Thanks for your opinion. Let's wait for other community members
> > > > opinions.    
> > > 
> > > i think fixing the arm load address computation makes
> > > sense (small extra cost of a relative reloc). i think
> > > the c code proposal i made in the thread is nicer than
> > > the old asm.
> > > 
> > > (i'm happy to make the same change on aarch64 too if
> > > prelinking is used there, but i think that's broken.)  
> > 
> > +1  
> 
> since you have a prelink setup, can you prepare the
> arm and aarch64 patches?
> 

I can prepare the patch - no problem.

Beforehand, I would like to hear from the community if we do have a
consensus about this solution...

> (i suspect x86 would need the same fix, but probably
> prelink is not used there anymore..?)

I do assume that in yocto at least it would use prelink by default as
well.

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 --]

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

[Lukasz Majewski]

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

Hi Joseph, Szabolcs 

> On Mon, 25 Oct 2021, Szabolcs Nagy via Libc-alpha wrote:
> 
> > ok, i think we need the patches upstream for that like
> > https://sourceware.org/pipermail/libc-alpha/2015-November/066153.html
> >  
> 
> The AArch64 prelink support isn't in the upstream Yocto
> cross-prelink, and the version written by Samsung in 2015 and on the
> cross_prelink_aarch64 branch has various problems resulting in test
> failures, in my experience.

Ok.

> 
> I sent patches (on top of a merge of the upstream cross_prelink and 
> cross_prelink_aarch64 branches) to the maintainer in May 2020 (the
> Yocto project mailing list doesn't accept email from non-subscribers,
> so won't have seen those patches), which made it work well enough to
> get clean prelink test results, but so far they haven't been
> committed to the upstream cross_prelink branch (or any other upstream
> branch).  I've now made those available at
> https://github.com/jsm28/prelink (cross_prelink_aarch64_fixes branch).
> 

So this branch shall be pulled by yocto's cross-prelink maintainer.
Without it the cross-prelink doesn't support aarch64?

> Note however that, like the original patches from Samsung, this
> version does indeed depend on a hack in _dl_tlsdesc_undefweak to work
> with TLS descriptors.
> 

To properly solve this issue we shall:

1. For arm
- Fix the cross-prelink (no patches available)

or

- Fix glibc (as proposed by Szabolcs)

2. For aarch64

- Try to upstream patches from Joseph to OE/Yocto's cross-prelink

or

- Fix glibc (if required)

or

- Do nothing (aarch64 will not be prelinked in OE/Yocto, which means
  that it will work correctly)


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 --]