[llvm-objdump][ELF] Ensure offset to verdaux entry array does not go past size #115284

antoniofrighetto · 2024-11-07T09:35:52Z

Validate vd_aux while parsing Elf_Verdef structure.

Fixes: #86611.

llvmbot · 2024-11-07T09:36:27Z

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

@llvm/pr-subscribers-objectyaml

Author: Antonio Frighetto (antoniofrighetto)

Changes

Validate vd_aux while parsing Elf_Verdef structure.

Fixes: #86611.

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

5 Files Affected:

(modified) llvm/include/llvm/ObjectYAML/ELFYAML.h (+1)
(modified) llvm/lib/ObjectYAML/ELFEmitter.cpp (+6-6)
(modified) llvm/lib/ObjectYAML/ELFYAML.cpp (+1)
(added) llvm/test/tools/llvm-objdump/ELF/invalid-verdef.test (+29)
(modified) llvm/tools/llvm-objdump/ELFDump.cpp (+13-4)

diff --git a/llvm/include/llvm/ObjectYAML/ELFYAML.h b/llvm/include/llvm/ObjectYAML/ELFYAML.h
index 8f045d6383623b..5383037582124c 100644
--- a/llvm/include/llvm/ObjectYAML/ELFYAML.h
+++ b/llvm/include/llvm/ObjectYAML/ELFYAML.h
@@ -582,6 +582,7 @@ struct VerdefEntry {
   std::optional<uint16_t> Flags;
   std::optional<uint16_t> VersionNdx;
   std::optional<uint32_t> Hash;
+  std::optional<uint16_t> AuxVOffset;
   std::vector<StringRef> VerNames;
 };
 
diff --git a/llvm/lib/ObjectYAML/ELFEmitter.cpp b/llvm/lib/ObjectYAML/ELFEmitter.cpp
index fc234581a45a70..a379a3e0d4a596 100644
--- a/llvm/lib/ObjectYAML/ELFEmitter.cpp
+++ b/llvm/lib/ObjectYAML/ELFEmitter.cpp
@@ -1655,7 +1655,7 @@ void ELFState<ELFT>::writeSectionContent(Elf_Shdr &SHeader,
     VerDef.vd_flags = E.Flags.value_or(0);
     VerDef.vd_ndx = E.VersionNdx.value_or(0);
     VerDef.vd_hash = E.Hash.value_or(0);
-    VerDef.vd_aux = sizeof(Elf_Verdef);
+    VerDef.vd_aux = E.AuxVOffset.value_or(sizeof(Elf_Verdef));
     VerDef.vd_cnt = E.VerNames.size();
     if (I == Section.Entries->size() - 1)
       VerDef.vd_next = 0;
@@ -1665,13 +1665,13 @@ void ELFState<ELFT>::writeSectionContent(Elf_Shdr &SHeader,
     CBA.write((const char *)&VerDef, sizeof(Elf_Verdef));
 
     for (size_t J = 0; J < E.VerNames.size(); ++J, ++AuxCnt) {
-      Elf_Verdaux VernAux;
-      VernAux.vda_name = DotDynstr.getOffset(E.VerNames[J]);
+      Elf_Verdaux VerdAux;
+      VerdAux.vda_name = DotDynstr.getOffset(E.VerNames[J]);
       if (J == E.VerNames.size() - 1)
-        VernAux.vda_next = 0;
+        VerdAux.vda_next = 0;
       else
-        VernAux.vda_next = sizeof(Elf_Verdaux);
-      CBA.write((const char *)&VernAux, sizeof(Elf_Verdaux));
+        VerdAux.vda_next = sizeof(Elf_Verdaux);
+      CBA.write((const char *)&VerdAux, sizeof(Elf_Verdaux));
     }
   }
 
diff --git a/llvm/lib/ObjectYAML/ELFYAML.cpp b/llvm/lib/ObjectYAML/ELFYAML.cpp
index e97248cbcf5682..821f48723b35e5 100644
--- a/llvm/lib/ObjectYAML/ELFYAML.cpp
+++ b/llvm/lib/ObjectYAML/ELFYAML.cpp
@@ -1921,6 +1921,7 @@ void MappingTraits<ELFYAML::VerdefEntry>::mapping(IO &IO,
   IO.mapOptional("Flags", E.Flags);
   IO.mapOptional("VersionNdx", E.VersionNdx);
   IO.mapOptional("Hash", E.Hash);
+  IO.mapOptional("AuxVOffset", E.AuxVOffset);
   IO.mapRequired("Names", E.VerNames);
 }
 
diff --git a/llvm/test/tools/llvm-objdump/ELF/invalid-verdef.test b/llvm/test/tools/llvm-objdump/ELF/invalid-verdef.test
new file mode 100644
index 00000000000000..abf9a2829f248c
--- /dev/null
+++ b/llvm/test/tools/llvm-objdump/ELF/invalid-verdef.test
@@ -0,0 +1,29 @@
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objdump -p %t 2>&1 | FileCheck --check-prefix=BROKEN-VERDEF -DFILE=%t %s
+
+# BROKEN-VERDEF: Version definitions:
+# BROKEN-VERDEF-NEXT: warning: '[[FILE]]': out-of-bound while parsing verdaux entries, corrupted verdef section
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_DYN
+  Machine:         EM_X86_64
+  Entry:           0x0000000000001000
+Sections:
+  - Name:            .gnu.version_d
+    Type:            SHT_GNU_verdef
+    Flags:           [ SHF_ALLOC ]
+    Address:         0x0000000000000230
+    AddressAlign:    0x0000000000000004
+    Entries:
+      - Version:         1
+        Flags:           44
+        VersionNdx:      0
+        Hash:            12345
+        AuxVOffset:      0x45
+        Names:
+          - VERSION_1
+DynamicSymbols: []
+...
diff --git a/llvm/tools/llvm-objdump/ELFDump.cpp b/llvm/tools/llvm-objdump/ELFDump.cpp
index 5ac13495662faf..3c5d11ef8ba751 100644
--- a/llvm/tools/llvm-objdump/ELFDump.cpp
+++ b/llvm/tools/llvm-objdump/ELFDump.cpp
@@ -39,6 +39,9 @@ template <typename ELFT> class ELFDumper : public Dumper {
   void printProgramHeaders();
   void printSymbolVersion();
   void printSymbolVersionDependency(const typename ELFT::Shdr &Sec);
+  void printSymbolVersionDefinition(const typename ELFT::Shdr &Shdr,
+                                    ArrayRef<uint8_t> Contents,
+                                    StringRef StrTab);
 };
 } // namespace
 
@@ -380,9 +383,9 @@ void ELFDumper<ELFT>::printSymbolVersionDependency(
 }
 
 template <class ELFT>
-static void printSymbolVersionDefinition(const typename ELFT::Shdr &Shdr,
-                                         ArrayRef<uint8_t> Contents,
-                                         StringRef StrTab) {
+void ELFDumper<ELFT>::printSymbolVersionDefinition(
+    const typename ELFT::Shdr &Shdr, ArrayRef<uint8_t> Contents,
+    StringRef StrTab) {
   outs() << "\nVersion definitions:\n";
 
   const uint8_t *Buf = Contents.data();
@@ -398,6 +401,12 @@ static void printSymbolVersionDefinition(const typename ELFT::Shdr &Shdr,
            << format("0x%08" PRIx32 " ", (uint32_t)Verdef->vd_hash);
 
     const uint8_t *BufAux = Buf + Verdef->vd_aux;
+    if (BufAux > Contents.end()) {
+      reportWarning("out-of-bound while parsing verdaux entries, corrupted "
+                    "verdef section",
+                    Obj.getFileName());
+      break;
+    }
     uint16_t VerdauxIndex = 0;
     while (BufAux) {
       auto *Verdaux = reinterpret_cast<const typename ELFT::Verdaux *>(BufAux);
@@ -430,7 +439,7 @@ template <class ELFT> void ELFDumper<ELFT>::printSymbolVersion() {
     if (Shdr.sh_type == ELF::SHT_GNU_verneed)
       printSymbolVersionDependency(Shdr);
     else
-      printSymbolVersionDefinition<ELFT>(Shdr, Contents, StrTab);
+      printSymbolVersionDefinition(Shdr, Contents, StrTab);
   }
 }

antoniofrighetto · 2024-11-07T09:38:28Z

Change in ObjectYAML can be pre-committed separately, if it looks good.

jh7370

I think it would be worth splitting the yaml2obj part into a separate PR. It should include tests too for yaml2obj (and possibly obj2yaml, if it can be used to generate YAML for such a corrupt object).

jh7370 · 2024-11-07T12:46:16Z

llvm/lib/ObjectYAML/ELFEmitter.cpp

+      Elf_Verdaux VerdAux;
+      VerdAux.vda_name = DotDynstr.getOffset(E.VerNames[J]);
      if (J == E.VerNames.size() - 1)
-        VernAux.vda_next = 0;
+        VerdAux.vda_next = 0;
      else
-        VernAux.vda_next = sizeof(Elf_Verdaux);
-      CBA.write((const char *)&VernAux, sizeof(Elf_Verdaux));
+        VerdAux.vda_next = sizeof(Elf_Verdaux);
+      CBA.write((const char *)&VerdAux, sizeof(Elf_Verdaux));


As this is unrelated, could you put it in a separate commit, please?

jh7370 · 2024-11-07T12:50:07Z

llvm/lib/ObjectYAML/ELFYAML.cpp

@@ -1921,6 +1921,7 @@ void MappingTraits<ELFYAML::VerdefEntry>::mapping(IO &IO,
  IO.mapOptional("Flags", E.Flags);
  IO.mapOptional("VersionNdx", E.VersionNdx);
  IO.mapOptional("Hash", E.Hash);
+  IO.mapOptional("AuxVOffset", E.AuxVOffset);


If I understand it correctly, all this does is set the value - it doesn't actually cause the data to be written to do a different offset. Is that correct?

Assuming that's the case, I'd suggest naming it after the spec name of vd_aux, e.g. VDAux. This is to match the similar fields like SHName and SHOffset for sections, whose only impact is to modify the section header (in contrast with the Name and Offset fields etc).

llvm/test/tools/llvm-objdump/ELF/invalid-verdef.test

jh7370 · 2024-11-07T12:52:10Z

llvm/test/tools/llvm-objdump/ELF/invalid-verdef.test

+        Flags:           44
+        VersionNdx:      0
+        Hash:            12345
+        AuxVOffset:      0x45


I'd put a comment somewhere around here, or possibly at the start of the YAML, highlighting how this field is invalid.

llvm/test/tools/llvm-objdump/ELF/invalid-verdef.test

jh7370 · 2024-11-07T12:56:57Z

llvm/tools/llvm-objdump/ELFDump.cpp

+      reportWarning("out-of-bound while parsing verdaux entries, corrupted "
+                    "verdef section",
+                    Obj.getFileName());


Please rewrite this error to include as much context as possible. In particular, take a look at how llvm-readobj and llvm-objdump describe section names, and include the actual offset and expected offset.

I'd consider something like "vd_aux value xxx in section xxx points past the end of the section at offset xxx", where "xxx" should be replaced with appropriate strings/numbers.

I went with 'vd_aux value %val in section verdef points past end of the section, corrupted section,' as I don't think it’s that relevant to provide the exact byte count by which the aux entry went out of bounds. I added 'corrupted section' consistently with the other warnings.

antoniofrighetto · 2024-11-07T17:21:10Z

@jh7370, I rebased this to the other PR, but commits have been split up. Should have reviewed everything, feel free to let me know if it makes sense.

MaskRay · 2024-11-08T03:34:32Z

llvm/test/tools/llvm-objdump/ELF/invalid-verdef.test

+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objdump -p %t 2>&1 | FileCheck --check-prefix=BROKEN-VERDEF -DFILE=%t %s
+
+# Ensure we emit a warning when vd_aux offset field is invalid.


MaskRay · 2024-11-08T03:35:16Z

llvm/tools/obj2yaml/elf2yaml.cpp

+      return createStringError(errc::invalid_argument,
+                               "vd_aux value " + Twine(Verdef->vd_aux) +
+                                   " in section verdef points past end of the "
+                                   "section, corrupted section");


, corrupted section reads strange

Rephrased for both.

…past size Validate `vd_aux` while parsing `Elf_Verdef` structure. Fixes: llvm#86611.

antoniofrighetto · 2024-11-08T17:51:30Z

Rebased it to main.

jh7370

Please don't force push to active reviews. It's against LLVM review policy, there's actual no need to, it makes it harder to review (because you can't view changes since the previous review) and leads to "commit not found" style errors when clicking on links.

jh7370 · 2024-11-12T11:59:53Z

llvm/tools/llvm-objdump/ELFDump.cpp

+      reportWarning("corrupted section: vd_aux value " + Twine(Verdef->vd_aux) +
+                        " in section verdef points past end of the section",
+                    Obj.getFileName());


I went with 'vd_aux value %val in section verdef points past end of the section, corrupted section,' as I don't think it’s that relevant to provide the exact byte count by which the aux entry went out of bounds. I added 'corrupted section' consistently with the other warnings.

I just searched for the string "corrupted section" and haven't found any relevant reference. What other warnings are you trying to be consistent with?

Please use the describe method (in Object\ELF.h) in place of "in section verdef" to be consistent with other recent messages added/changed in llvm-readelf.

The idea of including the section size is that it could be the size that is malformed, not the vd_aux value (for whatever reason). ELFDumper.cpp in llvm-readelf has a number of examples of this sort of message pattern (search for "past the end"). There may be others too.

jh7370 · 2024-11-12T12:01:54Z

llvm/test/tools/llvm-objdump/ELF/invalid-verdef.test

+      - Version:         1
+        Flags:           0
+        VersionNdx:      0
+        VDAux:           69


This still could do with a comment saying how it's invalid (i.e. what it should be).

MaskRay · 2025-02-23T20:59:13Z

Obsoleted by #128434 . We should reuse llvm-readobj -V code and possibly reuse some llvm-readobj tests

antoniofrighetto requested a review from MaskRay November 7, 2024 09:35

llvmbot added objectyaml llvm:binary-utilities labels Nov 7, 2024

antoniofrighetto mentioned this pull request Nov 7, 2024

llvm-objdump: corrupt ELF file can crash llvm-objdump in printSymbolVersionDefinition() #86611

Open

jh7370 reviewed Nov 7, 2024

View reviewed changes

antoniofrighetto mentioned this pull request Nov 7, 2024

[ObjectYAML][ELF] Allow verdaux entry offset to be user-defined #115343

Merged

antoniofrighetto force-pushed the feature/fix-elfdump-oob branch from f94b791 to 48f9f14 Compare November 7, 2024 17:19

MaskRay reviewed Nov 8, 2024

View reviewed changes

antoniofrighetto force-pushed the feature/fix-elfdump-oob branch 2 times, most recently from ab4c234 to 5f36ef3 Compare November 8, 2024 17:49

[llvm-objdump][ELF] Ensure offset to verdaux entry array does not go …

4aa80da

…past size Validate `vd_aux` while parsing `Elf_Verdef` structure. Fixes: llvm#86611.

antoniofrighetto force-pushed the feature/fix-elfdump-oob branch from 5f36ef3 to 4aa80da Compare November 8, 2024 17:50

antoniofrighetto requested review from jh7370 and MaskRay November 11, 2024 14:59

jh7370 reviewed Nov 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[llvm-objdump][ELF] Ensure offset to verdaux entry array does not go past size #115284

[llvm-objdump][ELF] Ensure offset to verdaux entry array does not go past size #115284

antoniofrighetto commented Nov 7, 2024

llvmbot commented Nov 7, 2024 •

edited

Loading

antoniofrighetto commented Nov 7, 2024

jh7370 left a comment

jh7370 Nov 7, 2024

jh7370 Nov 7, 2024

jh7370 Nov 7, 2024

jh7370 Nov 7, 2024

antoniofrighetto Nov 7, 2024

antoniofrighetto commented Nov 7, 2024

MaskRay Nov 8, 2024

MaskRay Nov 8, 2024

antoniofrighetto Nov 8, 2024

antoniofrighetto commented Nov 8, 2024

jh7370 left a comment

jh7370 Nov 12, 2024

jh7370 Nov 12, 2024

MaskRay commented Feb 23, 2025

[llvm-objdump][ELF] Ensure offset to verdaux entry array does not go past size #115284

Are you sure you want to change the base?

[llvm-objdump][ELF] Ensure offset to verdaux entry array does not go past size #115284

Conversation

antoniofrighetto commented Nov 7, 2024

llvmbot commented Nov 7, 2024 • edited Loading

antoniofrighetto commented Nov 7, 2024

jh7370 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoniofrighetto commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoniofrighetto commented Nov 8, 2024

jh7370 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaskRay commented Feb 23, 2025

llvmbot commented Nov 7, 2024 •

edited

Loading