[ELF]Add overflow check to ELF note iterator

[cabbaken]

Add overflow check to ELF note iterator to handle large p_filesz or sh_size, avoid accessing invalid memory.

[llvmbot]

@llvm/pr-subscribers-llvm-binary-utilities

Author: Ruoyu Qiu (cabbaken)

Changes

Add overflow check to ELF note iterator to handle large p_filesz or sh_size, avoid accessing invalid memory.

---

Full diff: https://github.com/llvm/llvm-project/pull/160451.diff

1 Files Affected:

• (modified) llvm/include/llvm/Object/ELF.h (+4-2)

diff --git a/llvm/include/llvm/Object/ELF.h b/llvm/include/llvm/Object/ELF.h
index 0b362d389c177..59f63eb6b5bb6 100644
--- a/llvm/include/llvm/Object/ELF.h
+++ b/llvm/include/llvm/Object/ELF.h
@@ -407,7 +407,8 @@ class ELFFile {
   Elf_Note_Iterator notes_begin(const Elf_Phdr &Phdr, Error &Err) const {
     assert(Phdr.p_type == ELF::PT_NOTE && "Phdr is not of type PT_NOTE");
     ErrorAsOutParameter ErrAsOutParam(Err);
-    if (Phdr.p_offset + Phdr.p_filesz > getBufSize()) {
+    if (Phdr.p_offset + Phdr.p_filesz > getBufSize() ||
+        Phdr.p_offset + Phdr.p_filesz < Phdr.p_offset) {
       Err =
           createError("invalid offset (0x" + Twine::utohexstr(Phdr.p_offset) +
                       ") or size (0x" + Twine::utohexstr(Phdr.p_filesz) + ")");
@@ -435,7 +436,8 @@ class ELFFile {
   Elf_Note_Iterator notes_begin(const Elf_Shdr &Shdr, Error &Err) const {
     assert(Shdr.sh_type == ELF::SHT_NOTE && "Shdr is not of type SHT_NOTE");
     ErrorAsOutParameter ErrAsOutParam(Err);
-    if (Shdr.sh_offset + Shdr.sh_size > getBufSize()) {
+    if (Shdr.sh_offset + Shdr.sh_size > getBufSize() ||
+        Shdr.sh_offset + Shdr.sh_size < Shdr.sh_offset) {
       Err =
           createError("invalid offset (0x" + Twine::utohexstr(Shdr.sh_offset) +
                       ") or size (0x" + Twine::utohexstr(Shdr.sh_size) + ")");

[jh7370]

Test case? You should be able to craft an input using yaml2obj either in a lit or unit test that exercises these two code paths.

The PR description should mention both the size and offset fields, not just the size fields.

[cabbaken]

Will it be good to add a unit test in llvm/unittests/Object/ELFObjectFileTest.cpp?

[jh7370]

Will it be good to add a unit test in llvm/unittests/Object/ELFObjectFileTest.cpp?

ELFObjectFileTest.cpp is for testing the ELFObjectFile.cpp file. There's ELFTest.cpp for testing ELF.h/ELF.cpp, so you should add it to that one.

[cabbaken]

ELFObjectFileTest.cpp is for testing the ELFObjectFile.cpp file. There's ELFTest.cpp for testing ELF.h/ELF.cpp, so you should add it to that one.

I've added the unit test in ELFTest.cpp.

[jh7370]

One nit, otherwise looks fine.

[jh7370]

I'd avoid auto in these tests to make it clearer what the type is.

[cabbaken]

The type of PhdrsOrErr is Expected<ELFFile<ELF64LE>::Elf_Phdr_Range>, which is a bit verbose here. Since the getBuildID code uses auto, I followed the same style.
I can adjust this if needed.

[jh7370]

Still using auto here and in the similar loop below. You could probably add a couple of using declarations to avoid repeating the full ELFFile<ELF64LE>::Elf_Phdr_Range and Shdr equivalent.

[jh7370]

One remaining nit, otherwise LGTM.

[jh7370]

Nit: delete this blank line

[cabbaken]

Fixed

[jh7370]

@cabbaken, do you need me to merge this?

[cabbaken]

That would be great, thanks. I don’t have merge rights here.