Page MenuHomeFreeBSD

Fix ldd to work with more ELF files.
ClosedPublic

Authored by jhb on Jan 25 2021, 11:23 PM.

Details

Summary
  • Use libelf to parse ELF data structures and remove code duplication for ELF32.
  • Don't require the OSABI field to be set to the FreeBSD OSABI for shared libraries. Both AArch64 and RISC-V leave it set to "none" and instead depend on the ABI tag note. For ldd, this means falling back to walking the sections and then walking the notes in SHT_NOTE sections to find the ABI tag note to determine if an ELF shared library without OSABI set in the header file is a FreeBSD shared library.

Sponsored by: DARPA

Test Plan
  • ldd /lib/libcasper.so.1 works

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jhb requested review of this revision.Jan 25 2021, 11:23 PM

Note that in terms of future changes, if @arichardson 's changes to make ld-elf32.so.1 honor LD_* land, then we can simplify ldd further to only set LD_* and ignore LD_32_*. CheriBSD also has local changes to direct-exec rtld for shared libraries instead of using dlopen(). I might bring this in as well which would let us retire ldd32.

usr.bin/ldd/ldd.c
314

Maybe I don't need the uintptr_t cast here as both are the source and destination types are const

usr.bin/ldd/ldd.c
314

Bah, it's due to a warning about the alignment changing. I might still replace it with a const void * cast to make it a bit shorter.

usr.bin/ldd/ldd.c
281

Isn't this section iteration, as opposed to segments? I.e. the new code seems to iterate over optional sections headers, which, unless I am mistaken, would be a regression against current code. You need to iterate over segments from program headers, find dynamic segment and then find flags_1.

usr.bin/ldd/ldd.c
281

libelf's APIs only let you (gracefully) get data from sections. However, I could use gelf_xlatetom() on data manually read from the binary and still only iterate over a segment. I would probably do the same for notes in that case. That said, do we really expect to have FreeBSD ELF files that only have program headers and no section headers?

usr.bin/ldd/ldd.c
281

  • Do not depend on ELF section headers.

I've tested this version on both a plain PDE (/bin/sh), a PIE (/usr/tests/lib/csu/dynamicpie/init_test), and a shared library (/lib/libcasper.so.1) on RISC-V and on the provided 'ls' binary, a plain PDE, and a shared library on amd64 (I didn't have a PIE handy on my amd64 desktop).

usr.bin/ldd/ldd.c
281

The updated version works on this file fine (though being an executable with EI_OSABI set, it probably also worked with the first version. We only need to check PT_NOTE if EI_OSABI isn't set (and even then we only care if it is a shared library), and we only need to check PT_DYNAMIC for PIE binaries (PDEs will still work even if the PT_DYNAMIC bits were broken).

usr.bin/ldd/ldd.c
446

I could perhaps have a bool pie and call is_pie here in the ET_DYN case to save the 'dynphdr' variable.

So is there a check that machine type in ELF header is compatible with the system?

usr.bin/ldd/ldd.c
309

I believe it is more holistic to use sizeof(Elf32_Addr) instead of 4.

315

Perhaps you need to compare namesz before doing strncmp(). I also checked that descsz == sizeof(int32_t).

384

IMO it is more clean to save the result of DF_1_PIE check into local variable and only leave one free() call instead of three.

jhb marked 2 inline comments as done.Jan 27 2021, 1:06 AM
In D28342#634503, @kib wrote:

So is there a check that machine type in ELF header is compatible with the system?

Neither the old code nor the new do this. It is left up to the runtime linker / kernel to determine that and error if the binary is unsupported when execve() or dlopen() are called.

The current code did check EI_CLASS to avoid having to deal with byte-swapping, but libelf handles that transparently when using gelf_*().

usr.bin/ldd/ldd.c
309

The only thing is that notes are aligned on a 4 byte boundary always and not due to holding an address. Someone might think while reading it that it should be Elf64_Addr for 64-bit. libelf's source uses 'ROUNDUP2(namesz, 4U)` (contrib/elftoolchain/libelf/libelf_convert.m4). I think the actual "true" source of the alignment might be something like:

namesz = roundup2(note->n_namesz, _Alignof(*note));

But that seems verbose. Perhaps sizeof(uint32_t) would be ok as a Elf_Note is just 3 of those.

Hmm, in imgact_elf.c we have a private #define ELF_NOTE_ROUNDSIZE 4 that is used in parse_notes().

  • Avoid multiple free() calls in is_pie().
  • Stricter checks on the abi tag note.
  • Use a local 'pie' to replace 'dynphdr'.
usr.bin/ldd/ldd.c
265–266

The other two functions also return bool.

309

Maybe add a comment that elf notes must be multiples of 4 bytes according to the spec? That would stop people from changing it to sizeof(Elf_Addr).

usr.bin/ldd/ldd.c
317

Could you please add static const char[] array for "FreeBSD" ?

usr.bin/ldd/ldd.c
317

Should this be spelled ELF_NOTE_FREEBSD while we're at it?

jhb marked an inline comment as done.
  • Use ELF_NOTE_FREEBSD and replace 4 with sizeof(uint32_t).
jhb marked 2 inline comments as done.Jan 27 2021, 6:59 PM
jhb added inline comments.
usr.bin/ldd/ldd.c
265–266

That would be a bit of a larger change as the caller stores the return value in an int. I prefer bool for new code, and would use bool if refactoring the calling code, but would be hesitant to change the calling code to bool especially when the meat of the changes are in this function and not the caller. If we did change the caller to use bool in the future I would also change is_shlib to be bool instead of int at the same time.

309

I went with sizeof(uint32_t). Better than a bare 4 and matches what we use in the definition of Elf_Note.

317

ELF_NOTE_FREEBSD works. I don't know that we really need a static const char array for it
though.

This revision is now accepted and ready to land.Jan 27 2021, 7:17 PM
This revision was automatically updated to reflect the committed changes.