On Thu, Apr 15, 2021 at 03:05:45PM +0200, Mark Kettenis wrote:
> > Date: Thu, 15 Apr 2021 14:20:00 +0200
> > From: Martin Pieuchot <m...@openbsd.org>
> > Content-Type: text/plain; charset=utf-8
> > Content-Disposition: inline
> >
> > On 14/04/21(Wed) 18:33, Klemens Nanni wrote:
> > > A bogus libvorbisenc.so.3.1 causes ld.so(1) to crash on my Pinebook Pro
> > > which saw a few NVMe/power related panics:
> > >
> > > $ ogg123 song62.ogg
> > > Segmentation fault (core dumped)
> > >
> > > $ egdb -q ogg123 ogg123.core
> > > Reading symbols from ogg123...(no debugging symbols found)...done.
> > > [New process 512916]
> > > Core was generated by `ogg123'.
> > > Program terminated with signal SIGSEGV, Segmentation fault.
> > > #0 0x0000000d388623e4 in _dl_find_symbol_obj (obj=0xcfae56000,
> > > sl=0x7ffffdd728) at /usr/src/libexec/ld.so/resolve.c:614
> > > 614 for (si = obj->buckets_elf[sl->sl_elf_hash %
> > > obj->nbuckets];
> > > (gdb) p obj->buckets_elf
> > > There is no member named buckets_elf.
> > >
> > > (`buckets_elf' is a macro)
> > >
> > > (gdb) p obj->hash_u.u_elf.buckets
> > > $1 = (const Elf_Hash_Word *) 0x0
> > > (gdb) p obj->nbuckets
> > > $2 = 0
> > >
> > > Backtrace for completeness:
> > >
> > > (gdb) bt
> > > #0 0x0000000d388623e4 in _dl_find_symbol_obj (obj=0xcfae56000,
> > > sl=0x7ffffdd728) at /usr/src/libexec/ld.so/resolve.c:614
> > > #1 0x0000000d38862210 in _dl_find_symbol (name=0xd9e9e6919
> > > "malloc_options", flags=16, ref_sym=0xd9e9d23f8, req_obj=0xcfae52000) at
> > > /usr/src/libexec/ld.so/resolve.c:704
> > > #2 0x0000000d388603b8 in _dl_md_reloc (object=<optimized out>,
> > > rel=<optimized out>, relsz=<optimized out>) at
> > > /usr/src/libexec/ld.so/aarch64/rtld_machine.c:170
> > > #3 0x0000000d38864714 in _dl_rtld (object=0xcfae52000) at
> > > /usr/src/libexec/ld.so/loader.c:722
> > > #4 0x0000000d388646dc in _dl_rtld (object=0xcfae52400) at
> > > /usr/src/libexec/ld.so/loader.c:712
> > > #5 0x0000000d388646dc in _dl_rtld (object=0xd6d641400) at
> > > /usr/src/libexec/ld.so/loader.c:712
> > > #6 0x0000000d388646dc in _dl_rtld (object=0xcfae52800) at
> > > /usr/src/libexec/ld.so/loader.c:712
> > > #7 0x0000000d388646dc in _dl_rtld (object=0xcfae53c00) at
> > > /usr/src/libexec/ld.so/loader.c:712
> > > #8 0x0000000d388646dc in _dl_rtld (object=0xcfae56800) at
> > > /usr/src/libexec/ld.so/loader.c:712
> > > #9 0x0000000d388646dc in _dl_rtld (object=0xcfae56400) at
> > > /usr/src/libexec/ld.so/loader.c:712
> > > #10 0x0000000d388646dc in _dl_rtld (object=0xcfae56c00) at
> > > /usr/src/libexec/ld.so/loader.c:712
> > > #11 0x0000000d388646dc in _dl_rtld (object=0xd0b867400) at
> > > /usr/src/libexec/ld.so/loader.c:712
> > > #12 0x0000000d388646dc in _dl_rtld (object=0xd6d63f400) at
> > > /usr/src/libexec/ld.so/loader.c:712
> > > #13 0x0000000d388646dc in _dl_rtld (object=0xcfae53400) at
> > > /usr/src/libexec/ld.so/loader.c:712
> > > #14 0x0000000d388646dc in _dl_rtld (object=0xd6d63f800) at
> > > /usr/src/libexec/ld.so/loader.c:712
> > > #15 0x0000000d388646dc in _dl_rtld (object=0xcfae52c00) at
> > > /usr/src/libexec/ld.so/loader.c:712
> > > #16 0x0000000d388646dc in _dl_rtld (object=0xcfae53000) at
> > > /usr/src/libexec/ld.so/loader.c:712
> > > #17 0x0000000d388646dc in _dl_rtld (object=0xd0b867c00) at
> > > /usr/src/libexec/ld.so/loader.c:712
> > > #18 0x0000000d388646dc in _dl_rtld (object=0xd6d640000) at
> > > /usr/src/libexec/ld.so/loader.c:712
> > > #19 0x0000000d388646dc in _dl_rtld (object=0xd6d640c00) at
> > > /usr/src/libexec/ld.so/loader.c:712
> > > #20 0x0000000d388646dc in _dl_rtld (object=0xd0b867000) at
> > > /usr/src/libexec/ld.so/loader.c:712
> > > #21 0x0000000d388646dc in _dl_rtld (object=0xcfae56000) at
> > > /usr/src/libexec/ld.so/loader.c:712
> > > #22 0x0000000d388646dc in _dl_rtld (object=0xd0b867800) at
> > > /usr/src/libexec/ld.so/loader.c:712
> > > #23 0x0000000d3886b618 in _dl_boot (argv=<optimized out>,
> > > envp=<optimized out>, dyn_loff=56782815232, dl_data=0x7ffffddde4) at
> > > /usr/src/libexec/ld.so/loader.c:663
> > > #24 0x0000000d3886a044 in _dl_start () at
> > > /usr/src/libexec/ld.so/aarch64/ldasm.S:59
> > > Backtrace stopped: previous frame identical to this frame (corrupt
> > > stack?)
> > >
> > >
> > > libvorbis being the culprit wasn't clear at all, but I went through
> > > pkg_check(1) to see checksum mismatches in the "libvorbis" package.
> > >
> > > tl;dr: I Upgraded to the latest snapshot, saved the currupted libraries,
> > > deinstalled all packages with `pkg_delete -X', installed "vorbis-tools"
> > > for ogg123(1) (pulling in "libvorbis") and the song played fine using
> > > valid files.
> > >
> > > Forcing the corrupted library I can reproduce, though:
> > >
> > > $ LD_PRELOAD=./libvorbisenc.so.3.1 ogg123 song62.ogg
> > > Segmentation fault (core dumped)
> > >
> > > So that's really this shared object and not any other currupted files.
> > >
> > > I'm not familiar with ld.so internals but the NULL dereference seems
> > > easy to avoid based on the condition that is used to set the `buckets'
> > > member in resolve.c:408f:
> > >
> > > if (object->Dyn.info[DT_HASH] != 0) {
> > > Elf_Hash_Word *hashtab =
> > > (Elf_Hash_Word *)(object->Dyn.info[DT_HASH] + obase);
> > >
> > > object->nchains = hashtab[1];
> > > if (object->nbuckets == 0) {
> > > object->nbuckets = hashtab[0];
> > > object->buckets_elf = hashtab + 2;
> > > object->chains_elf = object->buckets_elf +
> > > object->nbuckets;
> > > }
> > > }
> > >
> > > Hence, only access buckets in _dl_find_symbol_obj() if there are any;
> > > this fixes the crash and in fact allows me to play the song even when
> > > preloading the currupted library, i.e.
> > >
> > > $ LD_PRELOAD=./libvorbisenc.so.3.1 ogg123 song62.ogg
> > >
> > > now also works with patched ld.so installed -- I'd expected ld.so,
> > > libvorbis or ogg123 to crash on some other place...
> > >
> > > I'm not sure what to make of this, I also don't know enough about ld.so
> > > to judge this diff in context, it does however fix an obvious error.
> > > FWIW, regress/libexec/ld.so runs fine with this diff.
> >
> > I'm not sure if silently ignoring the corruption is the best way to go.
>
> It certainly isn't. If corruption is detected, the prcess should
> terminate immedtaley.
I totally agree, mention of regress passing shouldn't imply that I want
that diff in, but rather show that it didn't have unexpected drawbacks.
> > Do you know why `nbuckets' and `buckets_elf' aren't initialized for this
> > object? Do you know if _dl_finalize_object() has been call for it?
Yes, _dl_finalize_object() is always called for it.
I compared my corrupted shared library with an intact copy from ports
and it showed that the corrupted one was simply zeroed out at some point
(not truncated) until the end, e.g. readelf(1)'s `-h' or `-l' report
"Error: no .dynamic section in the dynamic segment".
So this isn't a case of some badly linked library or one that has a few
bits flipped, it's simply a partial one... seems like bad luck?
> > > Is this a code path that can happen with intact objects?
> > > Given that the file is obviously corrupted but programs using it still
> > > (partially) work, should a warning be printed in this case?
> >
> > Indicating that the library is corrupted might indeed be better than
> > crashing. However it isn't clear to me where such check should happen.
I've done just that now as there's nothing else to do. It is an obscure
case that I cannot explain without corruption, so very unlikely to
happen, but now it did...
> > > Index: resolve.c
> > > ===================================================================
> > > RCS file: /cvs/src/libexec/ld.so/resolve.c,v
> > > retrieving revision 1.94
> > > diff -u -p -r1.94 resolve.c
> > > --- resolve.c 4 Oct 2019 17:42:16 -0000 1.94
> > > +++ resolve.c 14 Apr 2021 15:56:14 -0000
> > > @@ -608,7 +608,7 @@ _dl_find_symbol_obj(elf_object_t *obj, s
> > > return r > 0;
> > > }
> > > } while ((*hashval++ & 1U) == 0);
> > > - } else {
> > > + } else if (obj->nbuckets > 0) {
> > > Elf_Word si;
> > >
> > > for (si = obj->buckets_elf[sl->sl_elf_hash % obj->nbuckets];
> > >
> >
> >
readelf(1) detects this corruption as missing (or empty/zeroed out as
code reading showed); we could probably to that as well but it'd be
less trivial and a generalisation of my issue.
So the new diff simply bails out if there is no symbol hash table, which
is equally relevant for both ELF and GNU hashes:
$ LD_PRELOAD=./bad.so ./ogg123 ~/song62.ogg
ld.so: ogg123.test: ./bad.so: no buckets
killed
$
Feedback? Objections? OK?
Index: resolve.c
===================================================================
RCS file: /cvs/src/libexec/ld.so/resolve.c,v
retrieving revision 1.94
diff -u -p -r1.94 resolve.c
--- resolve.c 4 Oct 2019 17:42:16 -0000 1.94
+++ resolve.c 29 Apr 2021 22:07:46 -0000
@@ -573,6 +573,9 @@ _dl_find_symbol_obj(elf_object_t *obj, s
{
const Elf_Sym *symt = obj->dyn.symtab;
+ if (obj->nbuckets == 0)
+ _dl_die("%s: no buckets", obj->load_name);
+
if (obj->status & STAT_GNU_HASH) {
uint32_t hash = sl->sl_gnu_hash;
Elf_Addr bloom_word;
