This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj] - Fix the crash when DT_STRTAB is broken.
ClosedPublic

Authored by grimar on Mar 24 2020, 7:55 AM.

Details

Summary

We might have a crash scenario when we have an invalid DT_STRTAB value
that is larger than the file size. I've added a test case to demonstrate.

Diff Detail

Event Timeline

grimar created this revision.Mar 24 2020, 7:55 AM
grimar retitled this revision from [llvm-readobj] - Fix a crash when DT_STRTAB is broken. to [llvm-readobj] - Fix the crash when DT_STRTAB is broken..Mar 24 2020, 7:59 AM

Just to make sure I understand the issue: if DT_STRTAB is small enough to fit within a segment, according to the segment's size/offset fields, we currently crash, if the size/offset fields themselves are invalid? What about if only part of the DT_STRTAB range is within the file (i.e. DT_STRTAB is less than file size but DT_STRTAB + DT_STRSZ is past the end)?

llvm/lib/Object/ELF.cpp
586–589

I wonder if this error message could be a little clearer. It's really an issue with the segment properties rather than the virtual address itself. Would something like "can't map virtual address 0x1234 to the segment with index 1: the segment goes past the end of the file" feel okay? Optionally even add in information about the segment end and file size to the message.

llvm/test/tools/llvm-readobj/ELF/loadname.test
3

this is a no-op

54

contains a DT_STRTAB entry whose address is past the end of the object.

grimar updated this revision to Diff 252827.Mar 26 2020, 6:40 AM
grimar marked 4 inline comments as done.
  • Addressed review comments.

Just to make sure I understand the issue: if DT_STRTAB is small enough to fit within a segment, according to the segment's size/offset fields, we currently crash, if the size/offset fields themselves are invalid?

Generally this bug exploits the situation when a segment's file size and DT_STRTAB together are broken in the way that allows a DT_STRTAB value to pass all current
checks, but triggers an access to area that is out of object buffer bounds.

E.g. We have VAddr == DT_STRTAB == 0xFFFE. We want to pass the last check:

uint64_t Delta = VAddr - Phdr.p_vaddr;
if (Delta >= Phdr.p_filesz)
  return createError("virtual address is not in any segment: 0x" +
                     Twine::utohexstr(VAddr));

So we need to have a p_filesz that is larger than 0xFFFE.

if DT_STRTAB is small enough to fit within a segment

I.e technically DT_STRTAB value kind of fits the segment (0xFFFE < 0xFFFF), but it is still broken too.
(Or, perhaps, from another POV: we have a file that is truncated, what is probably a more real case).

What about if only part of the DT_STRTAB range is within the file (i.e. DT_STRTAB is less than file size but DT_STRTAB + DT_STRSZ is past the end)?

Yes, I think this might trigger an issue too. If you do not mind I'd fix it in a follow-up. I have found a few situations (different ones, not just related to dynamic tags) where we might crash
and plan to investigate and prepare patches for all such places. I can start looking at DT_STRSZ case (and I guess some other dynamic tags) in the first place
while I am here in the code.

llvm/lib/Object/ELF.cpp
586–589

I've updated the message shown to refine and include more information. What do you think about it?

jhenderson accepted this revision.Mar 27 2020, 2:23 AM

LGTM, with one fix.

llvm/lib/Object/ELF.cpp
591

what is greater -> which is greater

This revision is now accepted and ready to land.Mar 27 2020, 2:23 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2020, 3:44 AM