This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readelf] - Do not crash when the PT_INTERP has an invalid offset.
ClosedPublic

Authored by grimar on Apr 28 2020, 8:35 AM.

Details

Summary

We do not verify the p_offset of the PT_INTERP header and tool may
crash when a program interpreter name string goes past the end of the file.

Depends on D78805.

Diff Detail

Event Timeline

grimar created this revision.Apr 28 2020, 8:35 AM
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay added inline comments.Apr 28 2020, 10:53 AM
llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test
316

Isn't the test covered by ERROR-INTERP2?

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

The two lines can be joined.

grimar marked an inline comment as done.Apr 29 2020, 12:05 AM
grimar added inline comments.
llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test
316

Nope. They have different error messages. In this case the offset points past the EOF. In ERROR-INTERP2 it is withing the file space.

grimar planned changes to this revision.Apr 29 2020, 12:34 AM

Going to merge 2 tests

grimar updated this revision to Diff 260854.Apr 29 2020, 12:46 AM
  • Merged 2 test cases.
MaskRay accepted this revision.Apr 29 2020, 9:18 AM
MaskRay added inline comments.
llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test
326

The main problem with the test is that if yaml2obj layout is updated, the p_offset fields will need an update which may be a bit tricky. Since the file size is displayed, this is probably fine.

560 = size(Ehdr) + size(Phdrs) + section contents with padding + size(Shdrs) = 64 + 56*5 + alignTo(1+0x13, 8) + 64*3 = 560

This revision is now accepted and ready to land.Apr 29 2020, 9:18 AM

a broken offset.

an invalid offset

jhenderson added inline comments.Apr 30 2020, 1:26 AM
llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test
316

used in the YAML below.

326

You could improve things slightly with a bit of cleverness in FileCheck, although I don't know whether it's a good idea. I think it should work as described, possibly with a few tweaks:

Firstly, emit the wc output to a file, and concatenate the llvm-readelf program header dump to the same file. Then, use FileCheck's numeric variables to capture the size and re-use it:

# RUN: wc -c < %t.err > %t.err.out
# RUN: FileCheck %s --check-prefix=SIZE --input-file=%t.err.out
# SIZE: 560
## Optionally use echo to add some extra data here that you can check to avoid matching something else.

## Write the additional 'C', '\0, 'C' bytes to the end.
# RUN: echo -n -e "C\x00C" >> %t.err

# RUN: llvm-readelf --program-headers %t.err 2>&1 >> %t.err.out
# RUN: FileCheck %s --input-file=%t.err.out -DFILE=%t.err --check-prefix=ERROR-INTERP

# ERROR-INTERP: [[#SIZE:]]
## If you add the extra data, check it here.
# ERROR-INTERP:      Type           Offset
# ERROR-INTERP-NEXT: INTERP         [[#SIZE]]
# ERROR-INTERP-NEXT:     [Requesting program interpreter: C]
# ERROR-INTERP-NEXT: INTERP         [[#SIZE+1]]
# ERROR-INTERP-NEXT:     [Requesting program interpreter: ]

and so on.

This has the benefit of only needing to change the YAML and one single check, should the layout change. You could even avoid the file concatenation etc by just capturing the variable based on the first INTERP's offset, if you want.

349

is a null

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

Maybe rename to ReportBadInterp or something. ReportError is a bit confusing when it's not an error!

grimar retitled this revision from [llvm-readelf] - Do not crash when the PT_INTERP has a broken offset. to [llvm-readelf] - Do not crash when the PT_INTERP has an invalid offset..Apr 30 2020, 4:41 AM
grimar updated this revision to Diff 261198.Apr 30 2020, 5:44 AM
grimar marked 6 inline comments as done.
  • Addressed review comments.
grimar added inline comments.Apr 30 2020, 5:44 AM
llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test
326

You could even avoid the file concatenation etc by just capturing the variable based on the first INTERP's offset, if you want.

This looks good I think. Thanks for suggestion!
I've updated the test.

grimar updated this revision to Diff 261200.Apr 30 2020, 5:47 AM
  • Rebase properly.
jhenderson accepted this revision.May 1 2020, 2:11 AM

LGTM, with one remaining request.

llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test
327

I believe you don't want the leading 0x000 bit, and then you also won't need the %x either. I believe FileCheck will automatically recognise it as hex that way (as things stand, I think it might not, but can't remember the details). If you want ot be sure that it is hex though, you can keep the %x (and might need it in the lines below too to be sure its always hex, but I'm not sure - experimentation needed). FWIW, I don't think it being hex is a necessary check for this specific case, since we check it earlier.

grimar marked an inline comment as done.May 1 2020, 2:32 AM
grimar added inline comments.
llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test
327

I believe you don't want the leading 0x000 bit, and then you also won't need the %x either.

It does not work for me, this was the only way I've managed to make this work.

The following fail:

# ERROR-INTERP:      Type           Offset
# ERROR-INTERP-NEXT: INTERP         [[#OFFSET:]]
# ERROR-INTERP-NEXT:     [Requesting program interpreter: C]
# ERROR-INTERP-NEXT: INTERP         [[#OFFSET + 1]]
# ERROR-INTERP-NEXT:     [Requesting program interpreter: ]
D:\Work3\LLVM\llvm-project\llvm\test\tools\llvm-readobj\ELF\gnu-phdrs.test:324:2
2: error: ERROR-INTERP-NEXT: expected string not found in input
# ERROR-INTERP-NEXT: INTERP [[#OFFSET + 1]]
                     ^
<stdin>:10:2: note: scanning from here
 INTERP 0x000231 0x0000000000000000 0x0000000000000000 0x000000 0x000000 0x1
 ^
<stdin>:10:2: note: with "OFFSET + 1" equal to "1"
 INTERP 0x000231 0x0000000000000000 0x0000000000000000 0x000000 0x000000 0x1
 ^
jhenderson added inline comments.
llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test
327

Ah, okay, I guess the 0x is not cosnidered part of the numeric pattern and the default only reads decimal. @thopre, is that correct?

Anyway, from my experiments the leading zeroes can be included, and please specify '%x' explicitly in the definition.

332

Don't forget to update 0x232 to 0x[[#OFFSET+2]] and similar in the messages below.

grimar updated this revision to Diff 261450.May 1 2020, 3:20 AM
grimar marked 3 inline comments as done.
  • Addressed comments in according to latest discussion. (I am not sure I got everything right).

(It still depends on D78805.)

I am leaving on vacation until 12 of May.

grimar requested review of this revision.May 1 2020, 3:20 AM
grimar added inline comments.
llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test
327

Just in case to confirm that my understanding about "leading zeroes" was correct here,
here is a result of my experiments:

The following works:

# ERROR-INTERP:      Type           Offset
# ERROR-INTERP-NEXT: INTERP         0x000[[#%x,OFFSET:0x230]]
# ERROR-INTERP:      Type           Offset
# ERROR-INTERP-NEXT: INTERP         0x000[[#%x,OFFSET:0x000230]]

The following doesn't:

# ERROR-INTERP:      Type           Offset
# ERROR-INTERP-NEXT: INTERP         0x[[#%x,OFFSET:0x230]]
# ERROR-INTERP:      Type           Offset
# ERROR-INTERP-NEXT: INTERP         0x[[#%x,OFFSET:230]]

So we need the "0x000[[" part, but do not need leading zeroes for OFFSET.

grimar planned changes to this revision.May 1 2020, 3:32 AM
grimar marked an inline comment as done.
grimar added inline comments.
llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test
332

Don't forget to update 0x232 to 0x[[#OFFSET+2]] and similar in the messages below.

Now I am not sure we want it. I guess it will accept both 0x0000000000232 and just 0x232, does not seem we want it.

jhenderson accepted this revision.May 1 2020, 3:38 AM

LGTM, with the two error message fixes. I'd appreciate @thopre's input on the numeric variable usage too, but that shouldn't block this landing.

llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test
327

I missed the bit where you were explicitly specifying the value to capture and didn't replicate it in my own experiments properly. I'm at a loss here. The problem is that if the layout changes to have fewer (or more) leading zeroes, it will fail due to that.

That seems to be missing the point of numeric variables. Leading zeroes should be able to be included in the match.

334

Can you not get rid of the %x in all the usages? I thought that should be derived from the definition of OFFSET if unspecified and only needed specifying if different from the original.

Also, there are still two instances of explicit "0x233" in these messages.

jhenderson added inline comments.May 1 2020, 3:40 AM
llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test
332

I think it's okay for either. I don't think it matters whether we have leading zeroes or not in the error messages, personally. Also, as we've seen above, the leading zeroes aren't matched anyway, right? ;)

grimar marked an inline comment as done.May 1 2020, 8:16 AM
grimar added inline comments.
llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test
332

Also, as we've seen above, the leading zeroes aren't matched anyway, right? ;)

Right. I've just confused myself.

grimar accepted this revision.EditedMay 1 2020, 8:50 AM

Changes Planned -> Accepted, since it was accepted. Will commit shortly with requested changes.

thopre added a comment.May 1 2020, 8:51 AM

LGTM, with the two error message fixes. I'd appreciate @thopre's input on the numeric variable usage too, but that shouldn't block this landing.

%x and %X deliberately don't match the 0x0* prefix to allow:

  • matching plain hex number without 0x prefix
  • testing the actual number of leading zeros in case a tool use leading zero to align numbers for instance

It might be useful to introduce a precision syntax a la printf to specify how many leading zeros are allowed, as well as a --canonicalize-numvar-leading-zero or better named option which would allow matching as many leading zeros. Leading zeros are allowed in immediates though since that has no impact on what is matched, hence why #FOO+0x00018 is allowed.

Let me know if you need further clarification.

grimar abandoned this revision.May 1 2020, 11:52 AM

Was committed as 0d54612164519c874eecf5a1be68597b96410628, phab did not close it for some reason. And does not allow me to close it normally.

grimar reclaimed this revision.May 1 2020, 11:53 AM
This revision is now accepted and ready to land.May 1 2020, 11:53 AM
grimar closed this revision.May 1 2020, 11:53 AM
  • Close it normally.