Added phdr upper bound checks to ElfObject

[mothran]

Summary

Ensure the program_headers call will fail correctly if the program
headers are larger than the underlying buffer. New test to ensure
llvm-objdump can safely process a invalid phdr in a elf file.

[davide]

You need to add a testcase, probably.

[mothran]

I am unable to construct a correctly malformed file with yaml2obj, but was able to create a test with hexedit.
This is my first time committing a patch to LLVM and was wondering, do I have to use arc to submit a patch with a binary file? And should I do that as a follow up commit or a whole new review and one patch?

[davide]

In D38512#888639, @mothran wrote:

I am unable to construct a correctly malformed file with yaml2obj, but was able to create a test with hexedit.

I'm surprised you can't craft with yaml2obj. What's the problem? If it's a limitation in yaml2obj we might consider fixing it.

This is my first time committing a patch to LLVM and was wondering, do I have to use arc to submit a patch with a binary file? And should I do that as a follow up commit or a whole new review and one patch?

You should attach your test (whatever it is) to the same patch, so it's grouped logically (and it' easier to bisect, revert, doing archeology, etc..)

[mothran]

The simplest way to trigger this issue is by adjusting the phoff in the elf header and that did not appear to be controllable from my read of the yaml2obj code at: https://github.com/llvm-mirror/llvm/blob/master/tools/yaml2obj/yaml2elf.cpp#L180

[davide]

Almost there. Do you have commit access or you want me to commit on your behalf?

include/llvm/Object/ELF.h

147

is this clang formatted?

test/Object/elf-invalid-phdr.test

2

Please add a comment on how to generate the test in case we want to reproduce in future.

[mothran]

I updated the diff to correctly use FileCheck with the llvm-objdump output and clang-formated the code in ELF.h. I don't have commit access so if you could commit on my behalf that would be great.

include/llvm/Object/ELF.h

147

Is there a way to cleanly run clang-format across the build to double check for myself?

[davide]

LGTM, do you need somebody to commit this on your behalf?

include/llvm/Object/ELF.h

public:
   Expected<Elf_Rel_Range> rels(const Elf_Shdr *Sec) const {
     return getSectionContentsAsArray<Elf_Rel>(Sec);
   }
 
   /// \brief Iterate over program header table.
   Expected<Elf_Phdr_Range> program_headers() const {
     if (getHeader()->e_phnum && getHeader()->e_phentsize != sizeof(Elf_Phdr))
       return createError("invalid e_phentsize");
+    if (getHeader()->e_phoff +
+            (getHeader()->e_phnum * getHeader()->e_phentsize) >
+        getBufSize())
+      return createError("program headers longer than binary");
     auto *Begin =
         reinterpret_cast<const Elf_Phdr *>(base() + getHeader()->e_phoff);
     return makeArrayRef(Begin, Begin + getHeader()->e_phnum);
   }
 
   Expected<StringRef> getSectionStringTable(Elf_Shdr_Range Sections) const;
   Expected<uint32_t> getSectionIndex(const Elf_Sym *Sym, Elf_Sym_Range Syms,
                                      ArrayRef<Elf_Word> ShndxTable) const;

test/Object/elf-invalid-phdr.test

+# invalid-phdr.elf is generated by creating a simple elf file with yaml2obj:
+# !ELF
+# FileHeader:
+#   Class:           ELFCLASS64
+#   Data:            ELFDATA2LSB
+#   Type:            ET_EXEC
+#   Machine:         EM_X86_64
+# Sections:
+#   - Name:            .text
+#     Type:            SHT_PROGBITS
+#     Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+#     AddressAlign:    0x0000000000001000
+#     Content:         "00000000"
+# ProgramHeaders:
+#   - Type: PT_LOAD
+#     Flags: [ PF_X, PF_R ]
+#     VAddr: 0xAAAA1000
+#     PAddr: 0xFFFF1000
+#     Sections:
+#       - Section: .text
+#
+# The editing the e_phoff in with a hexeditor to set it to 0xffffff
+RUN: not llvm-objdump -private-headers %p/Inputs/invalid-phdr.elf 2>&1 \
+RUN:         | FileCheck %s
+
+CHECK: LLVM ERROR: Invalid data was encountered while parsing the file
+