[llvm-readobj/elf] - Refine the code for broken PT_DYNAMIC segment diagnostic.

[grimar]

Summary

The code that reports "PT_DYNAMIC segment offset + size exceeds the size of the file"
has an issue: it is possible to bypass the validation by overflowing the size + offset result.

[jhenderson]

Maybe it would be a good idea to split this change in two - one to update the test, and the other to change the behaviour - to ensure we don't accidentally break something in the test and behaviour at the same time. What do you think?

llvm/test/tools/llvm-readobj/ELF/malformed-pt-dynamic.test

68–69

Do we actually need to strip the section headers any more? I'm not sure the original issue really applies any more, as it was just to do with yaml2obj's previous limitations.

[grimar]

In D85519#2206037, @jhenderson wrote:

Maybe it would be a good idea to split this change in two - one to update the test, and the other to change the behaviour - to ensure we don't accidentally break something in the test and behaviour at the same time. What do you think?

Well, OK. It might require more than 2 changes then, I think. Currently we report "PT_DYNAMIC segment offset + size exceeds the size of the file" for two slightly different cases
and it does not provide enough information about the difference. So I'd start from improving this message first. Then partially rewriting the test to a new one, simplified, which will use the new messages.
And then a third patch will change the logic and add the corresponding testing.

llvm/test/tools/llvm-readobj/ELF/malformed-pt-dynamic.test

68–69

I'll answer on this later. I am observing a different behavior without stripping headers - llvm-readobj complains about broken PT_DYNAMIC and says it going to use some information from the sections header instead. But I had no chance to investigate it better yet.

[grimar]

llvm/test/tools/llvm-readobj/ELF/malformed-pt-dynamic.test

68–69

The behavior with and without the section header table is slightly different. In D86073 I am testing both cases.

[grimar]

• Rebased and updated after landing patches that were splitted out.

(Now we have here a behavior update and the corresponding test case changes only).

[jhenderson]

LGTM!

[grimar]

Reverted, because broke UBSan. The test triggers an overflow issue in the different place in the llvm-readobj code.
(http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap-ubsan/builds/21386/steps/check-llvm%20ubsan/logs/stdio)

[grimar]

• Updated checks in createDRI to fix issue reported by UBSan.

(I've verified that malformed-pt-dynamic.test now pass with -DLLVM_USE_SANITIZER=Undefined)

[grimar]

PTAL

[jhenderson]

Looks okay, but what was causing the UBSAN failure?

[grimar]

In D85519#2230120, @jhenderson wrote:

Looks okay, but what was causing the UBSAN failure?

This line:

const uint8_t *Addr = Obj->base() + Offset;

I believe pointers are not allowed to overflow.

[jhenderson]

In D85519#2230138, @grimar wrote:

In D85519#2230120, @jhenderson wrote:

Looks okay, but what was causing the UBSAN failure?

This line:

const uint8_t *Addr = Obj->base() + Offset;

I believe pointers are not allowed to overflow.

Makes sense, thanks!

llvm/test/Object/invalid.test

- Name:    .dynamic
     Content: "00"
 
 ## PT_DYNAMIC's p_offset field is so large that p_offset + p_filesz is larger
 ## than the object size. Check llvm-readobj reports it.
 
 # RUN: yaml2obj %s --docnum=22 -o %t22
 # RUN: llvm-readobj --dyn-relocations %t22 2>&1 | FileCheck -DFILE=%t22 --check-prefix=DYN-TABLE-PHDR %s
 
-# DYN-TABLE-PHDR: warning: '[[FILE]]': PT_DYNAMIC segment offset + size exceeds the size of the file
+# DYN-TABLE-PHDR: warning: '[[FILE]]': PT_DYNAMIC segment offset (0xffff0000) + file size (0x0) exceeds the size of the file (0x150)
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 ProgramHeaders:
   - Type:   PT_DYNAMIC
     Offset: 0xffff0000
 
 ## PT_DYNAMIC's p_filesz field is so large that p_offset + p_filesz is larger
 ## than the object size. Check llvm-readobj reports it.
 
 # RUN: yaml2obj %s --docnum=23 -o %t23
 # RUN: llvm-readobj --dyn-relocations %t23 2>&1 \
-# RUN:  | FileCheck -DFILE=%t23 --check-prefix=DYN-TABLE-PHDR %s
+# RUN:  | FileCheck -DFILE=%t23 --check-prefix=DYN-TABLE-PHDR2 %s
+
+# DYN-TABLE-PHDR2: warning: '[[FILE]]': PT_DYNAMIC segment offset (0x78) + file size (0xffff0000) exceeds the size of the file (0x1a8)
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:

llvm/test/tools/llvm-readobj/ELF/malformed-pt-dynamic.test

-# If the offset and/or size fields of the PT_DYNAMIC field become corrupted,
-# we should report a sensible message.
+## If the offset and/or size fields of the PT_DYNAMIC field become corrupted,
+## we should report a sensible message.
 
-# Creating such a malformed file is hard. The easiest way to simulate it is to
-# truncate the file. Note that the section headers must first be stripped or
-# llvm-readobj will fail to parse the file due to the section header table
-# offset pointing outside the file.
-
-# RUN: yaml2obj %s -o %t.base
-# RUN: llvm-objcopy --strip-sections %t.base %t.stripped
-
-# Test case where the size is too large to fit in the file, but the start is
-# within the file.
-# RUN: cp %t.stripped %t.truncated1
-# RUN: %python -c "with open(r'%t.truncated1', 'r+') as f: f.truncate(0x1001)"
-# RUN: llvm-readobj %t.truncated1 --dynamic-table 2>&1 | FileCheck -DFILE=%t.truncated1 %s
-
-# Test case where the offset is too large to be in the file.
-# RUN: cp %t.stripped %t.truncated2
-# RUN: %python -c "with open(r'%t.truncated2', 'r+') as f: f.truncate(0xFFF)"
-# RUN: llvm-readobj %t.truncated2 --dynamic-table 2>&1 | FileCheck -DFILE=%t.truncated2 %s
+## Case A: Test case where the size is too large to fit in the file, but the start is
+##         within the file.
+# RUN: yaml2obj %s -DFILESIZE=0x119 -o %t1
+# RUN: llvm-readobj %t1 --dynamic-table 2>&1 | FileCheck -DFILE=%t1 %s --check-prefix=WARN1
+# RUN: llvm-readelf %t1 --dynamic-table 2>&1 | FileCheck -DFILE=%t1 %s --check-prefix=WARN1
 
-# CHECK: warning: '[[FILE]]': PT_DYNAMIC segment offset + size exceeds the size of the file
+# WARN1: warning: '[[FILE]]': PT_DYNAMIC segment offset (0x1000) + file size (0x119) exceeds the size of the file (0x1118)
+
+## Case B: Test case where the offset is too large to be in the file.
+# RUN: yaml2obj %s -DOFFSET=0x1119 -o %t2
+# RUN: llvm-readobj %t2 --dynamic-table 2>&1 | FileCheck -DFILE=%t2 %s --check-prefix=WARN2
+# RUN: llvm-readelf %t2 --dynamic-table 2>&1 | FileCheck -DFILE=%t2 %s --check-prefix=WARN2
+
+# WARN2: warning: '[[FILE]]': PT_DYNAMIC segment offset (0x1119) + file size (0x10) exceeds the size of the file (0x1118)
+
+## Case C: test we report a warning when the offset + the file size of the PT_DYNAMIC is so large a
+##         value that it overflows the platform address size type.
+
+# RUN: yaml2obj %s -DOFFSET=0xffffffffffffffff -o %t3
+# RUN: llvm-readobj %t3 --dynamic-table 2>&1 | FileCheck -DFILE=%t3 %s --check-prefix=WARN3
+# RUN: llvm-readelf %t3 --dynamic-table 2>&1 | FileCheck -DFILE=%t3 %s --check-prefix=WARN3
+
+# WARN3: warning: '[[FILE]]': PT_DYNAMIC segment offset (0xffffffffffffffff) + file size (0x10) exceeds the size of the file (0x1118)
+
+# RUN: yaml2obj %s -DFILESIZE=0xffffffffffffffff -o %t4
+# RUN: llvm-readobj %t4 --dynamic-table 2>&1 | FileCheck -DFILE=%t4 %s --check-prefix=WARN4
+# RUN: llvm-readelf %t4 --dynamic-table 2>&1 | FileCheck -DFILE=%t4 %s --check-prefix=WARN4
+
+# WARN4: warning: '[[FILE]]': PT_DYNAMIC segment offset (0x1000) + file size (0xffffffffffffffff) exceeds the size of the file (0x1118)
+
+## Case D: the same as "Case C", but for a 32-bit object.
+
+# RUN: yaml2obj %s -DBITS=32 -DOFFSET=0xffffffff -o %t5
+# RUN: llvm-readobj %t5 --dynamic-table 2>&1 | FileCheck -DFILE=%t5 %s --check-prefix=WARN5
+# RUN: llvm-readelf %t5 --dynamic-table 2>&1 | FileCheck -DFILE=%t5 %s --check-prefix=WARN5
+
+# WARN5: warning: '[[FILE]]': PT_DYNAMIC segment offset (0xffffffff) + file size (0x8) exceeds the size of the file (0x10ac)
+
+# RUN: yaml2obj %s -DBITS=32 -DFILESIZE=0xffffffff -o %t6
+# RUN: llvm-readobj %t6 --dynamic-table 2>&1 | FileCheck -DFILE=%t6 %s --check-prefix=WARN6
+# RUN: llvm-readelf %t6 --dynamic-table 2>&1 | FileCheck -DFILE=%t6 %s --check-prefix=WARN6
+
+# WARN6: warning: '[[FILE]]': PT_DYNAMIC segment offset (0x1000) + file size (0xffffffff) exceeds the size of the file (0x10ac)
 
 --- !ELF
 FileHeader:
-  Class:   ELFCLASS64
+  Class:   ELFCLASS[[BITS=64]]
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
-  Machine: EM_X86_64
+  Machine: EM_NONE
 Sections:
-  - Name:         .dynamic
-    Type:         SHT_DYNAMIC
-    Address:      0x1000
-    AddressAlign: 0x1000
+  - Name:     .dynamic
+    Type:     SHT_DYNAMIC
+    Address:  0x1000
+    Offset:   0x1000
+    ShOffset: [[OFFSET=<none>]]
     Entries:
       - Tag:   DT_NULL
         Value: 0
 ProgramHeaders:
-  - Type: PT_LOAD
-    VAddr: 0x1000
-    Sections:
-      - Section: .dynamic
-  - Type: PT_DYNAMIC
-    VAddr: 0x1000
+  - Type:     PT_DYNAMIC
+    FileSize: [[FILESIZE=<none>]]
     Sections:
       - Section: .dynamic
+SectionHeaderTable:
+  NoHeaders: true

llvm/tools/llvm-readobj/ELFDumper.cpp

ELFDumper<ELFT>::findDynamic(const ELFFile<ELFT> *Obj) {
   const Elf_Shdr *DynamicSec = nullptr;
   for (const Elf_Shdr &Sec : cantFail(Obj->sections())) {
     if (Sec.sh_type != ELF::SHT_DYNAMIC)
       continue;
     DynamicSec = &Sec;
     break;
   }
 
-  if (DynamicPhdr && DynamicPhdr->p_offset + DynamicPhdr->p_filesz >
-                         ObjF->getMemoryBufferRef().getBufferSize()) {
-    reportWarning(
-        createError(
-            "PT_DYNAMIC segment offset + size exceeds the size of the file"),
-        ObjF->getFileName());
+  if (DynamicPhdr && ((DynamicPhdr->p_offset + DynamicPhdr->p_filesz >
+                       ObjF->getMemoryBufferRef().getBufferSize()) ||
+                      (DynamicPhdr->p_offset + DynamicPhdr->p_filesz <
+                       DynamicPhdr->p_offset))) {
+    reportUniqueWarning(createError(
+        "PT_DYNAMIC segment offset (0x" +
+        Twine::utohexstr(DynamicPhdr->p_offset) + ") + file size (0x" +
+        Twine::utohexstr(DynamicPhdr->p_filesz) +
+        ") exceeds the size of the file (0x" +
+        Twine::utohexstr(ObjF->getMemoryBufferRef().getBufferSize()) + ")"));
     // Don't use the broken dynamic header.
     DynamicPhdr = nullptr;
   }
 
   if (DynamicPhdr && DynamicSec) {
     StringRef Name =
         unwrapOrError(ObjF->getFileName(), Obj->getSectionName(DynamicSec));
     if (DynamicSec->sh_addr + DynamicSec->sh_size >