[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

136–137

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

136–137

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

136–137

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/tools/llvm-readobj/ELF/malformed-pt-dynamic.test

 # RUN: yaml2obj %s -DOFFSET=0x1119 -DNOHEADERS=true -o %t2.noheaders
 # RUN: llvm-readobj %t2.noheaders --dynamic-table 2>&1 | FileCheck -DFILE=%t2.noheaders %s \
 # RUN:   --check-prefix=WARN2-NOHEADERS --implicit-check-not="DynamicSection ["
 # RUN: llvm-readelf %t2.noheaders --dynamic-table 2>&1 | FileCheck -DFILE=%t2.noheaders %s \
 # RUN:   --check-prefix=WARN2-NOHEADERS --implicit-check-not="Dynamic section"
 
 # WARN2-NOHEADERS: 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: not llvm-readobj %t3 --dynamic-table 2>&1 | FileCheck -DFILE=%t3 %s --check-prefix=WARN3
+# RUN: not 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 (0x1130)
+# WARN3: error:   '[[FILE]]': Invalid data was encountered while parsing the file
+
+# RUN: yaml2obj %s -DNOHEADERS=true -DOFFSET=0xffffffffffffffff -o %t3.noheaders
+# RUN: llvm-readobj %t3.noheaders --dynamic-table 2>&1 | \
+# RUN:   FileCheck -DFILE=%t3.noheaders %s --check-prefix=WARN3-NOHEADERS
+# RUN: llvm-readelf %t3.noheaders --dynamic-table 2>&1 | \
+# RUN:   FileCheck -DFILE=%t3.noheaders %s --check-prefix=WARN3-NOHEADERS
+
+# WARN3-NOHEADERS: 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 (0x1130)
+
+# RUN: yaml2obj %s -DNOHEADERS=true -DFILESIZE=0xffffffffffffffff -o %t4.noheaders
+# RUN: llvm-readobj %t4.noheaders --dynamic-table 2>&1 | \
+# RUN:   FileCheck -DFILE=%t4.noheaders %s --check-prefix=WARN4-NOHEADERS
+# RUN: llvm-readelf %t4.noheaders --dynamic-table 2>&1 | \
+# RUN:   FileCheck -DFILE=%t4.noheaders %s --check-prefix=WARN4-NOHEADERS
+
+# WARN4-NOHEADERS: 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: not llvm-readobj %t5 --dynamic-table 2>&1 | FileCheck -DFILE=%t5 %s --check-prefix=WARN5
+# RUN: not 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 (0x10c8)
+# WARN5: error:   '[[FILE]]': Invalid data was encountered while parsing the file
+
+# RUN: yaml2obj %s -DNOHEADERS=true -DBITS=32 -DOFFSET=0xffffffff -o %t5.noheaders
+# RUN: llvm-readobj %t5.noheaders --dynamic-table 2>&1 | \
+# RUN:   FileCheck -DFILE=%t5.noheaders %s --check-prefix=WARN5-NOHEADERS
+# RUN: llvm-readelf %t5.noheaders --dynamic-table 2>&1 | \
+# RUN:   FileCheck -DFILE=%t5.noheaders %s --check-prefix=WARN5-NOHEADERS
+
+# WARN5-NOHEADERS: 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 (0x10c8)
+
+# RUN: yaml2obj %s -DNOHEADERS=true -DBITS=32 -DFILESIZE=0xffffffff -o %t6.noheaders
+# RUN: llvm-readobj %t6.noheaders --dynamic-table 2>&1 | \
+# RUN:   FileCheck -DFILE=%t6.noheaders %s --check-prefix=WARN6-NOHEADERS
+# RUN: llvm-readelf %t6.noheaders --dynamic-table 2>&1 | \
+# RUN:   FileCheck -DFILE=%t6.noheaders %s --check-prefix=WARN6-NOHEADERS
+
+# WARN6-NOHEADERS: 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
 Sections:
   - Name:     .dynamic
     Type:     SHT_DYNAMIC
     Address:  0x1000
     Offset:   0x1000
     ShOffset: [[OFFSET=<none>]]
     Entries:
       - Tag:   DT_NULL
         Value: 0
 ProgramHeaders:
   - Type:     PT_DYNAMIC
     FileSize: [[FILESIZE=<none>]]
     Sections:
       - Section: .dynamic
 SectionHeaderTable:
   NoHeaders: [[NOHEADERS=false]]

llvm/tools/llvm-readobj/ELFDumper.cpp

 
 private:
   std::unique_ptr<DumpStyle<ELFT>> ELFDumperStyle;
 
   TYPEDEF_ELF_TYPES(ELFT)
 
   DynRegionInfo createDRI(uint64_t Offset, uint64_t Size, uint64_t EntSize) {
     const ELFFile<ELFT> *Obj = ObjF->getELFFile();
-    const uint8_t *Addr = Obj->base() + Offset;
-    if (Addr < Obj->base() || Addr + Size > Obj->base() + Obj->getBufSize())
+    if (Offset + Size < Offset || Offset + Size > Obj->getBufSize())
       reportError(errorCodeToError(llvm::object::object_error::parse_failed),
                   ObjF->getFileName());
-
-    return {Addr, Size, EntSize, ObjF->getFileName()};
+    return {Obj->base() + Offset, Size, EntSize, ObjF->getFileName()};
   }
 
   void printAttributes();
   void printMipsReginfo();
   void printMipsOptions();
 
   std::pair<const Elf_Phdr *, const Elf_Shdr *>
   findDynamic(const ELFFile<ELFT> *Obj);
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()) {
+  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;