Page MenuHomePhabricator

[llvm-readobj/elf] - Don't crash when the size of a dynamic symbol table, inferred from the hash table, is broken.
ClosedPublic

Authored by grimar on Sep 1 2020, 3:16 AM.

Details

Summary

Currently we might derive the dynamic symbol table size from the DT_HASH hash table (using its nchain field).
It is possible to crash dumpers with a broken relocation that refers to a symbol with an index
that is too large. To trigger it, the inferred size of the dynamic symbol table should go past the end of the object.

This patch adds a size validation + warning.

Diff Detail

Event Timeline

grimar created this revision.Sep 1 2020, 3:16 AM
Herald added a project: Restricted Project. · View Herald Transcript
grimar requested review of this revision.Sep 1 2020, 3:16 AM
grimar retitled this revision from [llvm-readobj/elf] - Don't crash when the size of s dynamic symbol table, inferred from the hash table, is broken. to [llvm-readobj/elf] - Don't crash when the size of a dynamic symbol table, inferred from the hash table, is broken..Sep 1 2020, 4:26 AM
MaskRay added inline comments.Sep 1 2020, 12:08 PM
llvm/tools/llvm-readobj/ELFDumper.cpp
2261

Do you mean "the hash table size (0x...) derived from ..." ?

jhenderson added inline comments.Sep 2 2020, 1:10 AM
llvm/test/tools/llvm-readobj/ELF/dyn-symbols-size-from-hash-table.test
330
339

Here and below, I'd prefer it if we can avoid hard coding the values into the CHECKs, because if the yaml2obj layout changes at all, these values will have to be updated.

Can you use FileCheck numeric variables to capture the sizes according to the section headers and the file size somewhere, and then use those?

grimar updated this revision to Diff 290932.Sep 10 2020, 3:49 AM
grimar marked an inline comment as done.
  • Addressed review comments.
llvm/test/tools/llvm-readobj/ELF/dyn-symbols-size-from-hash-table.test
339

Done.

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

Nope. This message is about the size of the dynamic symbol table. This size was derived from the hash table
and the value is too large to be used as a size of the dynamic symbol table.

jhenderson added inline comments.Sep 14 2020, 3:28 AM
llvm/test/tools/llvm-readobj/ELF/dyn-symbols-size-from-hash-table.test
340

I don't think you need the {{^}} here and below, since the FileCheck value will start from the start of the number anyway, and we know it will be at the start of the file.

340–341

Be consistent with your spacing after the , in the FileCheck numeric variables. I don't mind which you use, but you've got some cases with a space after it, and others without.

llvm/tools/llvm-readobj/ELFDumper.cpp
2261–2265

I'm not sure I like this message. The first clause, whilst I can figure out what it means, feels very clunky. I think your best option is "The size (0x1234), derived from the hash table, of the dynamic symbol table..." or "The size (0x1234) of the dynamic symbol table at 0x1234, derived from the hash table, goes past ..."

grimar updated this revision to Diff 291840.Sep 15 2020, 2:58 AM
grimar marked 4 inline comments as done.
  • Addressed review comments.
llvm/tools/llvm-readobj/ELFDumper.cpp
2261–2265

Fixed, thanks!

This revision is now accepted and ready to land.Sep 15 2020, 5:54 AM

Looks like this breaks tests on mac: http://45.33.8.238/mac/20491/step_11.txt

Please take a look and revert for now if it takes a while to fix.

Looks like this breaks tests on mac: http://45.33.8.238/mac/20491/step_11.txt

Please take a look and revert for now if it takes a while to fix.

Thanks for letting me know. Fixed in rGef4851742de5e64a1ba9de51e375ac503d2d7ecb.

grimar added inline comments.Sep 16 2020, 2:55 AM
llvm/test/tools/llvm-readobj/ELF/dyn-symbols-size-from-hash-table.test
340

note: there is a solid reason not to use {{^}} here:
I've tested on mac that wc -c does not start printing from the begining of the line, it adds whitespaces.
So with {{^}} it just doesn't pass there.

phosek added a subscriber: phosek.Sep 22 2020, 12:18 AM

Looks like this breaks tests on mac: http://45.33.8.238/mac/20491/step_11.txt

Please take a look and revert for now if it takes a while to fix.

Thanks for letting me know. Fixed in rGef4851742de5e64a1ba9de51e375ac503d2d7ecb.

We're still seeing a failure on macOS:

/b/s/w/ir/k/llvm-project/llvm/test/tools/llvm-readobj/ELF/dyn-symbols-size-from-hash-table.test:356:23: error: BROKEN-NCHAIN-LLVM: expected string not found in input
# BROKEN-NCHAIN-LLVM: {{^}}[[#%u, FILESIZE:]]
                      ^
/b/s/w/ir/k/staging/llvm_build/test/tools/llvm-readobj/ELF/Output/dyn-symbols-size-from-hash-table.test.tmp4.out.llvm.txt:1:2: note: scanning from here
 840 /b/s/w/ir/k/staging/llvm_build/test/tools/llvm-readobj/ELF/Output/dyn-symbols-size-from-hash-table.test.tmp4.1
 ^
/b/s/w/ir/k/staging/llvm_build/test/tools/llvm-readobj/ELF/Output/dyn-symbols-size-from-hash-table.test.tmp4.out.llvm.txt:13:5: note: possible intended match here
 Name: (0)
    ^

Input file: /b/s/w/ir/k/staging/llvm_build/test/tools/llvm-readobj/ELF/Output/dyn-symbols-size-from-hash-table.test.tmp4.out.llvm.txt
Check file: /b/s/w/ir/k/llvm-project/llvm/test/tools/llvm-readobj/ELF/dyn-symbols-size-from-hash-table.test

-dump-input=help explains the following input dump.

Input was:
<<<<<<
             1:  840 /b/s/w/ir/k/staging/llvm_build/test/tools/llvm-readobj/ELF/Output/dyn-symbols-size-from-hash-table.test.tmp4.1
check:356'0      X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
             2: /b/s/w/ir/k/staging/llvm_build/bin/llvm-readobj: warning: '/b/s/w/ir/k/staging/llvm_build/test/tools/llvm-readobj/ELF/Output/dyn-symbols-size-from-hash-table.test.tmp4.1': hash table nchain (4294967295) differs from symbol count derived from SHT_DYNSYM section header (1)
check:356'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             3: /b/s/w/ir/k/staging/llvm_build/bin/llvm-readobj: warning: '/b/s/w/ir/k/staging/llvm_build/test/tools/llvm-readobj/ELF/Output/dyn-symbols-size-from-hash-table.test.tmp4.1': the size (0x17ffffffe8) of the dynamic symbol table at 0xf0, derived from the hash table, goes past the end of the file (0x348) and will be ignored
check:356'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             4: 
check:356'0     ~
             5: File: /b/s/w/ir/k/staging/llvm_build/test/tools/llvm-readobj/ELF/Output/dyn-symbols-size-from-hash-table.test.tmp4.1
check:356'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             6: Format: elf64-x86-64
check:356'0     ~~~~~~~~~~~~~~~~~~~~
             7: Arch: x86_64
check:356'0     ~~~~~~~~~~~~
             8: AddressSize: 64bit
check:356'0     ~~~~~~~~~~~~~~~~~~
             9: LoadName: 
check:356'0     ~~~~~~~~~~
            10: Sections [
check:356'0     ~~~~~~~~~~
            11:  Section {
check:356'0     ~~~~~~~~~~
            12:  Index: 0
check:356'0     ~~~~~~~~~
            13:  Name: (0)
check:356'0     ~~~~~~~~~~
check:356'1         ?      possible intended match
            14:  Type: SHT_NULL (0x0)
check:356'0     ~~~~~~~~~~~~~~~~~~~~~
            15:  Flags [ (0x0)
check:356'0     ~~~~~~~~~~~~~~
            16:  ]
check:356'0     ~~
            17:  Address: 0x0
check:356'0     ~~~~~~~~~~~~~
            18:  Offset: 0x0
check:356'0     ~~~~~~~~~~~~
             .
             .
             .
>>>>>>

Looks like this breaks tests on mac: http://45.33.8.238/mac/20491/step_11.txt

Please take a look and revert for now if it takes a while to fix.

Thanks for letting me know. Fixed in rGef4851742de5e64a1ba9de51e375ac503d2d7ecb.

We're still seeing a failure on macOS:

/b/s/w/ir/k/llvm-project/llvm/test/tools/llvm-readobj/ELF/dyn-symbols-size-from-hash-table.test:356:23: error: BROKEN-NCHAIN-LLVM: expected string not found in input
# BROKEN-NCHAIN-LLVM: {{^}}[[#%u, FILESIZE:]]
                      ^
/b/s/w/ir/k/staging/llvm_build/test/tools/llvm-readobj/ELF/Output/dyn-symbols-size-from-hash-table.test.tmp4.out.llvm.txt:1:2: note: scanning from here
 840 /b/s/w/ir/k/staging/llvm_build/test/tools/llvm-readobj/ELF/Output/dyn-symbols-size-from-hash-table.test.tmp4.1
 ^
/b/s/w/ir/k/staging/llvm_build/test/tools/llvm-readobj/ELF/Output/dyn-symbols-size-from-hash-table.test.tmp4.out.llvm.txt:13:5: note: possible intended match here
 Name: (0)
    ^

Input file: /b/s/w/ir/k/staging/llvm_build/test/tools/llvm-readobj/ELF/Output/dyn-symbols-size-from-hash-table.test.tmp4.out.llvm.txt
Check file: /b/s/w/ir/k/llvm-project/llvm/test/tools/llvm-readobj/ELF/dyn-symbols-size-from-hash-table.test

-dump-input=help explains the following input dump.

Input was:
<<<<<<
             1:  840 /b/s/w/ir/k/staging/llvm_build/test/tools/llvm-readobj/ELF/Output/dyn-symbols-size-from-hash-table.test.tmp4.1
check:356'0      X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
             2: /b/s/w/ir/k/staging/llvm_build/bin/llvm-readobj: warning: '/b/s/w/ir/k/staging/llvm_build/test/tools/llvm-readobj/ELF/Output/dyn-symbols-size-from-hash-table.test.tmp4.1': hash table nchain (4294967295) differs from symbol count derived from SHT_DYNSYM section header (1)
check:356'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             3: /b/s/w/ir/k/staging/llvm_build/bin/llvm-readobj: warning: '/b/s/w/ir/k/staging/llvm_build/test/tools/llvm-readobj/ELF/Output/dyn-symbols-size-from-hash-table.test.tmp4.1': the size (0x17ffffffe8) of the dynamic symbol table at 0xf0, derived from the hash table, goes past the end of the file (0x348) and will be ignored
check:356'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             4: 
check:356'0     ~
             5: File: /b/s/w/ir/k/staging/llvm_build/test/tools/llvm-readobj/ELF/Output/dyn-symbols-size-from-hash-table.test.tmp4.1
check:356'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             6: Format: elf64-x86-64
check:356'0     ~~~~~~~~~~~~~~~~~~~~
             7: Arch: x86_64
check:356'0     ~~~~~~~~~~~~
             8: AddressSize: 64bit
check:356'0     ~~~~~~~~~~~~~~~~~~
             9: LoadName: 
check:356'0     ~~~~~~~~~~
            10: Sections [
check:356'0     ~~~~~~~~~~
            11:  Section {
check:356'0     ~~~~~~~~~~
            12:  Index: 0
check:356'0     ~~~~~~~~~
            13:  Name: (0)
check:356'0     ~~~~~~~~~~
check:356'1         ?      possible intended match
            14:  Type: SHT_NULL (0x0)
check:356'0     ~~~~~~~~~~~~~~~~~~~~~
            15:  Flags [ (0x0)
check:356'0     ~~~~~~~~~~~~~~
            16:  ]
check:356'0     ~~
            17:  Address: 0x0
check:356'0     ~~~~~~~~~~~~~
            18:  Offset: 0x0
check:356'0     ~~~~~~~~~~~~
             .
             .
             .
>>>>>>

Never mind, looks like rGef4851742de5e64a1ba9de51e375ac503d2d7ecb addressed it.