Page MenuHomePhabricator

[llvm-readobj] - Fix possible crashes related to dumping gnu hash symbols.
ClosedPublic

Authored by grimar on Sep 30 2020, 5:22 AM.

Details

Summary

It fixes possible scenarios when we crash/assert with --hash-symbols when
dumping an invalid GNU hash table which has a broken value in the buckets array.

This fixes a crash reported in comments for
https://bugs.llvm.org/show_bug.cgi?id=47681

Diff Detail

Event Timeline

grimar created this revision.Sep 30 2020, 5:22 AM
Herald added a project: Restricted Project. · View Herald Transcript
grimar requested review of this revision.Sep 30 2020, 5:22 AM
jhenderson added inline comments.Sep 30 2020, 5:49 AM
llvm/test/tools/llvm-readobj/ELF/hash-symbols.test
662

It's not clear from this comment what is causing the attempt to read past the end of file. Please could you clarify.

672–673
679–680

Surely we should be diagnosing the attempt to read using an invalid dynamic symbol index in the first place, so that we don't see semi-random warnings?

691

Ditto.

llvm/tools/llvm-readobj/ELFDumper.cpp
4098

Perhaps this - "length" implies the number of entries in the array, whereas "size" could mean either that or the total size taken up by the array (e.g. 4 times the length, if the elements are 4 bytes in size each).

grimar marked an inline comment as done.Sep 30 2020, 7:15 AM
grimar added inline comments.
llvm/test/tools/llvm-readobj/ELF/hash-symbols.test
679–680

I am not sure honestly. Currently our logic is that we can't get here when there is no SHT_DYNSYM section,
because we take the size of the dynamic symbol table from there and there is no other way to get it.
I.e. there is no dynamic tag for that, so in case when we e.g. don't have a .hash table to infer the size,
we will never be able to dump symbols currently and we return earlier:

void GNUStyle<ELFT>::printGnuHashTableSymbols(const Elf_GnuHash &GnuHash) {
  StringRef StringTable = this->dumper().getDynamicStringTable();
  if (StringTable.empty())
    return;

  Elf_Sym_Range DynSyms = this->dumper().dynamic_symbols();
  const Elf_Sym *FirstSym = DynSyms.empty() ? nullptr : &DynSyms[0];
  if (!FirstSym) {
    Optional<DynRegionInfo> DynSymRegion = this->dumper().getDynSymRegion();
    this->reportUniqueWarning(createError(
        Twine("unable to print symbols for the .gnu.hash table: the "
              "dynamic symbol table ") +
        (DynSymRegion ? "is empty" : "was not found")));
    return;
  }
....

At the same time I think loaders have no any checks (i.e. they don't try to use the size from SHT_DYNSYM) and will try to use a dynamic symbol found via DT_SYMTAB only. In this case it might probably be useful to try to dump symbols here,
even if they are invalid to reveal what loaders see on their side? Perhaps worth showing an additional warning, saying that we think/found that an index is invalid though. And we can also probably change the code shown above to allow us to pass when DynSymRegion.Addr != 0 (i.e. when we found a DT_SYMTAB, but no SHT_DYNSYM and the address is within the file).

If not, we can probably just skip symbols with invalid indices, just like you've suggested.

What do you think?

I think the handling of the GNU hash table is actually already fully correct. The crash (or error message when using this patch) is simply a side effect of the fact that size of the DynSymRegion was clobbered here (near the end of ELFDumper<ELFT>::parseDynamicTable()):

DynSymRegion->Size = HashTable->nchain * DynSymRegion->EntSize;

This in turn is because HashTable->nchain (like all of HashTable) is just incorrect on s390x at the moment, as discussed in https://bugs.llvm.org/show_bug.cgi?id=47681

jhenderson added inline comments.Oct 1 2020, 1:26 AM
llvm/test/tools/llvm-readobj/ELF/hash-symbols.test
679–680

Ah, I forgot that the dynsym has no size in the dynamic table (that's still dumb, but oh well). I think if we have information implying the index looks broken, due to other information (e.g. the section header), we should at least warn that this is probably not correct. I think the warning would be more than enough to say something is a bit bogus. I also think that means we don't need to try to dump invalid index symbols if we know they are bogus (why would a user care what the loader sees if they know that whatever it is is bad - in such situations they'd need to fix the source of the bad index).

I'm a little struggling to follow the full details of your comment though. so I may be misunderstanding your points/questions.

uweigand added inline comments.Oct 1 2020, 1:38 AM
llvm/test/tools/llvm-readobj/ELF/hash-symbols.test
679–680

As I said in my comment yesterday, in this particular executable we do have the correct size of dynsym from the section table -- it's just that this (correct) size is later replaced by (incorrect) information derived from the .hash section.

grimar marked an inline comment as done.Oct 1 2020, 2:35 AM

I think the handling of the GNU hash table is actually already fully correct. The crash (or error message when using this patch) is simply a side effect of the fact that size of the DynSymRegion was clobbered here (near the end of ELFDumper<ELFT>::parseDynamicTable()):

DynSymRegion->Size = HashTable->nchain * DynSymRegion->EntSize;

This in turn is because HashTable->nchain (like all of HashTable) is just incorrect on s390x at the moment, as discussed in https://bugs.llvm.org/show_bug.cgi?id=47681

That is true for you executable. But this patch fixes a bit different case. Here I am creating the .gnu.hash table with a broken value in the buckets array.
Currently with the test case provided we crash an all platforms at the same place where your executable crashes too.
So with this patch we fix the crash, but not the reason of PR47681, which is s390x specific.

grimar added inline comments.Oct 1 2020, 2:37 AM
llvm/test/tools/llvm-readobj/ELF/hash-symbols.test
679–680

I'm a little struggling to follow the full details of your comment though. so I may be misunderstanding your points/questions.

Nope. You got me right. I'll update the patch to report a warning and will stop printing a bogus symbol.

I think the handling of the GNU hash table is actually already fully correct. The crash (or error message when using this patch) is simply a side effect of the fact that size of the DynSymRegion was clobbered here (near the end of ELFDumper<ELFT>::parseDynamicTable()):

DynSymRegion->Size = HashTable->nchain * DynSymRegion->EntSize;

This in turn is because HashTable->nchain (like all of HashTable) is just incorrect on s390x at the moment, as discussed in https://bugs.llvm.org/show_bug.cgi?id=47681

That is true for you executable. But this patch fixes a bit different case. Here I am creating the .gnu.hash table with a broken value in the buckets array.
Currently with the test case provided we crash an all platforms at the same place where your executable crashes too.
So with this patch we fix the crash, but not the reason of PR47681, which is s390x specific.

I see. In this case this patch doesn't really address PR47681, but only replaces a crash with wrong output ... Still preferable to not crash, but of course we still need to fix the root cause (at the least, by ignoring the .hash section if the entry size doesn't match).

grimar added a comment.EditedOct 1 2020, 3:01 AM

I see. In this case this patch doesn't really address PR47681, but only replaces a crash with wrong output ... Still preferable to not crash, but of course we still need to fix the root cause (at the least, by ignoring the .hash section if the entry size doesn't match).

The fix for PR47681 (s390 specific) is independent from the crash (all platforms), so it should be fixed separatelly.
I am going to prepare a follow-up to fix the PR47681 after landing this.

I see. In this case this patch doesn't really address PR47681, but only replaces a crash with wrong output ... Still preferable to not crash, but of course we still need to fix the root cause (at the least, by ignoring the .hash section if the entry size doesn't match).

The fix for PR47681 (s390 specific) is independent from the crash (all platforms), so it should be fixed separatelly.
I'll am going to prepare a follow-up to fix the PR47681 after landing this.

Thanks!

grimar updated this revision to Diff 295537.EditedOct 1 2020, 5:37 AM
grimar marked 6 inline comments as done.
  • Update implementation (validate the symbol index and related changes and simplifications).
llvm/test/tools/llvm-readobj/ELF/hash-symbols.test
662

I've updated implementation and had to rework all test cases. Now there are no cases when we try read past the EOF - or at least I can't imagine an input for that atm.

llvm/tools/llvm-readobj/ELFDumper.cpp
4098

This place is gone.

jhenderson added inline comments.Oct 6 2020, 3:08 AM
llvm/test/tools/llvm-readobj/ELF/hash-symbols.test
723
724
llvm/tools/llvm-readobj/ELFDumper.cpp
2745–2746
4061
4073–4074

I'm not sure I understand what 'Values' here is referring to. Is it the hash values, or something else? Either way, there needs to be more context here.

grimar updated this revision to Diff 296443.Oct 6 2020, 6:41 AM
grimar marked 5 inline comments as done.
  • Addressed review comments.
jhenderson accepted this revision.Oct 8 2020, 12:56 AM

Looks good aside from a possible broken test.

llvm/test/tools/llvm-readobj/ELF/gnuhash.test
128

This test looks like it might need updating?

This revision is now accepted and ready to land.Oct 8 2020, 12:56 AM
grimar added inline comments.Oct 8 2020, 1:35 AM
llvm/test/tools/llvm-readobj/ELF/gnuhash.test
128

This message comes from a different place,
for LLVM style we show it when fail to dump Values:

void ELFDumper<ELFT>::printGnuHashTable() {
.....

  Expected<ArrayRef<Elf_Word>> Chains =
      getGnuHashTableChains<ELFT>(DynSymRegion, GnuHashTable);
  if (!Chains) {
    reportUniqueWarning(
        createError("unable to dump 'Values' for the SHT_GNU_HASH "
                    "section: " +
                    toString(Chains.takeError())));
    return;
  }

  W.printHexList("Values", *Chains);

I.e. it is unrelated to this patch, I think I just copypasted this error previously.
And for this particular place it seems that mentioning 'Values' in the message is reasonable.

jhenderson added inline comments.Oct 8 2020, 1:40 AM
llvm/test/tools/llvm-readobj/ELF/gnuhash.test
128

Okay (but I don't think 'Values' makes sense - values of what?)

grimar added inline comments.Oct 8 2020, 1:43 AM
llvm/test/tools/llvm-readobj/ELF/gnuhash.test
128

An example of normal output is:

GnuHashTable {
  Num Buckets: 3
  First Hashed Symbol Index: 1
  Num Mask Words: 2
  Shift Count: 2
  Bloom Filter: [0x3, 0x4]
  Buckets: [5, 6, 7]
  Values: [0x8, 0x9, 0xA, 0xB]
}

I.e. Values refers to the field that was expected to be dumped, but was not because of an error.