[PATCH] D76706: [llvm-readobj] - Fix the crash when DT_STRTAB is broken.
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 26 07:01:02 PDT 2020
grimar updated this revision to Diff 252827.
grimar marked 4 inline comments as done.
grimar added a comment.
- Addressed review comments.
In D76706#1943097 <https://reviews.llvm.org/D76706#1943097>, @jhenderson wrote:
> 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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76706/new/
https://reviews.llvm.org/D76706
Files:
llvm/lib/Object/ELF.cpp
llvm/test/tools/llvm-readobj/ELF/loadname.test
Index: llvm/test/tools/llvm-readobj/ELF/loadname.test
===================================================================
--- llvm/test/tools/llvm-readobj/ELF/loadname.test
+++ llvm/test/tools/llvm-readobj/ELF/loadname.test
@@ -1,6 +1,8 @@
## Check we are able to dump library soname properly.
-# RUN: yaml2obj %s -o %t.o
+## Test a valid object case first. We set 'FileSize' to 0x48, because this is a no-op,
+## i.e. this value would be set if we had no 'FileSize' at all.
+# RUN: yaml2obj -DDTSTRTABVAL=0x0 -DPHDRFILESIZE="0x48" %s -o %t.o
# RUN: llvm-readobj %t.o | FileCheck %s --check-prefix LLVM
# RUN: llvm-readelf --dynamic-table %t.o | FileCheck %s --check-prefix GNU
@@ -33,7 +35,7 @@
Link: .dynstr
Entries:
- Tag: DT_STRTAB
- Value: 0x0000000000000000
+ Value: [[DTSTRTABVAL]]
- Tag: DT_STRSZ
Value: 0x0000000000000007
- Tag: DT_SONAME
@@ -41,9 +43,23 @@
- Tag: DT_NULL
Value: 0x0000000000000000
ProgramHeaders:
- - Type: PT_LOAD
- Flags: [ PF_R ]
- VAddr: 0x0000
+ - Type: PT_LOAD
+ Flags: [ PF_R ]
+ VAddr: 0x0000
+ FileSize: [[PHDRFILESIZE]]
Sections:
- Section: .dynstr
- Section: .dynamic
+
+## Check we do not crash when an object contains a DT_STRTAB entry whose address
+## is past the end of the object.
+## Note that we have to set p_filesz for PT_LOAD larger than DT_STRTAB value
+## to trigger this particular warning.
+
+# RUN: yaml2obj -DDTSTRTABVAL=0xFFFE -DPHDRFILESIZE=0xFFFF %s -o %t.err.1.o
+# RUN: llvm-readobj %t.err.1.o 2>&1 | FileCheck %s -DFILE=%t.err.1.o --check-prefixes=BROKEN-OFFSET,BROKEN-OFFSET-LLVM
+# RUN: llvm-readelf --dynamic-table %t.err.1.o 2>&1 | FileCheck %s -DFILE=%t.err.1.o --check-prefixes=BROKEN-OFFSET,BROKEN-OFFSET-GNU
+
+# BROKEN-OFFSET: warning: '[[FILE]]': Unable to parse DT_STRTAB: can't map virtual address 0xfffe to the segment with index 1: the segment ends at 0x10077, what is greater than the file size (0x228)
+# BROKEN-OFFSET-LLVM: LoadName: <String table is empty or was not found>
+# BROKEN-OFFSET-GNU: 0x000000000000000e (SONAME) Library soname: [<String table is empty or was not found>]
Index: llvm/lib/Object/ELF.cpp
===================================================================
--- llvm/lib/Object/ELF.cpp
+++ llvm/lib/Object/ELF.cpp
@@ -580,7 +580,18 @@
if (Delta >= Phdr.p_filesz)
return createError("virtual address is not in any segment: 0x" +
Twine::utohexstr(VAddr));
- return base() + Phdr.p_offset + Delta;
+
+ uint64_t Offset = Phdr.p_offset + Delta;
+ if (Offset >= getBufSize())
+ return createError("can't map virtual address 0x" +
+ Twine::utohexstr(VAddr) + " to the segment with index " +
+ Twine(&Phdr - (*ProgramHeadersOrError).data() + 1) +
+ ": the segment ends at 0x" +
+ Twine::utohexstr(Phdr.p_offset + Phdr.p_filesz) +
+ ", what is greater than the file size (0x" +
+ Twine::utohexstr(getBufSize()) + ")");
+
+ return base() + Offset;
}
template class llvm::object::ELFFile<ELF32LE>;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D76706.252827.patch
Type: text/x-patch
Size: 3205 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200326/488fc2f5/attachment.bin>
More information about the llvm-commits
mailing list