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

[grimar]

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.

[jhenderson]

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–5

this is a no-op

54

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

[grimar]

• Addressed review comments.

In 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.

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]

LGTM, with one fix.

llvm/lib/Object/ELF.cpp

591

what is greater -> which is greater

llvm/lib/Object/ELF.cpp

if (I == LoadSegments.begin())
     return createError("virtual address is not in any segment: 0x" +
                        Twine::utohexstr(VAddr));
   --I;
   const Elf_Phdr &Phdr = **I;
   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));
-  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>;
 template class llvm::object::ELFFile<ELF32BE>;
 template class llvm::object::ELFFile<ELF64LE>;
 template class llvm::object::ELFFile<ELF64BE>;

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

 ## 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
 
 # LLVM:      Format: elf64-x86-64
 # LLVM-NEXT: Arch: x86_64
 # LLVM-NEXT: AddressSize: 64bit
 # LLVM-NEXT: LoadName: test.so
 
- Name:  .dynstr
     Flags: [ SHF_ALLOC ]
     Content: '746573742e736f00' ## "test.so"
   - Name:  .dynamic
     Type:  SHT_DYNAMIC
     Flags: [ SHF_ALLOC ]
     Link:  .dynstr
     Entries:
       - Tag:   DT_STRTAB
-        Value: 0x0000000000000000
+        Value: [[DTSTRTABVAL]]
       - Tag:   DT_STRSZ
         Value: 0x0000000000000007
       - Tag:   DT_SONAME
         Value: 0x0000000000000000
       - 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>]