[DWARF] Introduce verification for the unit header chain in .debug_info section to llvm-dwarfdump.

[sgravani]

Summary

This patch adds verification checks for the unit header chain in the .debug_info section.

Specifically, for each unit in the .debug_info section, the verifier checks that:

1. The unit length is valid (i.e. the unit can actually fit in the .debug_info section)
2. The dwarf version of the unit is valid
3. The address size is valid (4 or 8)
4. The unit type (if the unit is in dwarf5) is valid
5. The debug_abbrev_offset is valid

I'm still working on test cases, but I wanted to submit the patch here in order to get feedback.

[dblaikie]

Just a few drive by thoughts, but happy to leave this mostly to Adrian

include/llvm/DebugInfo/DWARF/DWARFUnit.h

250

Might be nice for consistency to have the default in the switch as well.

Also might be easier to return directly rather than having an extra variable:

switch (UnitType) {
case skeleton:
case split_compile:
  return 20;
case type:
case split-type:
  return 24;
case compile:
case partial:
default:
  return 12;
}

That said, should this switch have a default? Or is it reasonable to say "this must be called with a valid unit type, of which there are only these 6"?

include/llvm/DebugInfo/DWARF/DWARFVerifier.h

34

Why is this a member variable? It looks like it's only used within a local scope of handleDebugInfoUnitHeaderChain

lib/DebugInfo/DWARF/DWARFContext.cpp

427–429

Arguably, each unit with a valid header (that doesn't go off the end, etc) could be validated - just because one header is mangled doesn't mean the rest are (granted if it's mangled in a way that means the length is wrong and the rest of the headers from then on are probably off - then there's not much you can do with everything after the broken one, of course). Not necessary, but not impossible either. If it's going to stay this way maybe the error could be rephrased "unit contents will not be verified" (not that tehy don't need to be, or that they wouldn't benefit from being verified, etc)

lib/DebugInfo/DWARF/DWARFVerifier.cpp

50

Looks like this could never return false - since if the Offset+HeaderSize is a valid offset, then 'Offset' must be too (since it's a zero based array index - so X + N (where X and N are positive) can't be valid while X is invalid)

74

Looks like this probably only needs to be a boolean flag (& a local, as mentioned earlier) rather than a counter.

75

When does this happen? Seems like it could only happen if the debug_info section was empty, maybe? (Otherwise the previous unit would've already failed because it was too long) Perhaps it'd be better to have a special case for empty? (or maybe it's not an error, really?)

[probinson]

include/llvm/DebugInfo/DWARF/DWARFUnit.h

241

If these static methods are used only by the verifier, maybe they should just be local functions in DWARFVerifier.cpp.

lib/DebugInfo/DWARF/DWARFVerifier.cpp

39

This is assuming DWARF-32 format. It's okay to support only one format, if you have a check for DWARF-64 and explicitly reject it.

44

This could call computeDWARF5HeaderSize with an invalid unit type. That's okay if you document the contract in the declaration of that method.

51

Might want a DWARF-64 check here.

54

Improper TOCTOU. Admittedly this is far from security-sensitive, but using data before validating it is generally poor practice.

59

Again could call computeDWARF5HeaderSize with an invalid unit type.

[sgravani]

Added code that stops verification if it identifies a unit with DWARF64 format.
Addressed other reviewers' comments.

include/llvm/DebugInfo/DWARF/DWARFUnit.h

250

I think it makes more sense not to have the default. I modified this to reflect that the only valid arguments are these 6 types for dwarf5.
Thanks!

lib/DebugInfo/DWARF/DWARFContext.cpp

427–429

I agree that there's still value in verifying the contents of .debug_info, even when the unit header chain check fails. I'm changing this to have verification of the debug info section in any case. Thanks.

lib/DebugInfo/DWARF/DWARFVerifier.cpp

50

You're right. I fixed this part.

75

You're right. The only case would be if .debug_info section was empty.
I'm not sure if we should handle this as an error.
I think I would like to go with printing a warning message for this case (since we're verifying the .debug_info section and .debug_info is empty).

[sgravani]

lib/DebugInfo/DWARF/DWARFVerifier.cpp

39

I'm stopping verification if I detect a unit with 64-bit format (without generating error).
Thanks!

[friss]

Some random comments:

include/llvm/DebugInfo/DWARF/DWARFUnit.h

241

Not important, but I would call this isValidUnitType().

254

Really nit-picky: This doesn't seem to 'compute' anything. Why not just getDWARF5HeaderSize()?

264

I don't think we should return 12 when hitting the default case. If every use of computeDWARF5HeaderSize() is guarded by validateUnitType(), then having the default case be llvm_unreachable makes more sense.

include/llvm/DebugInfo/DWARF/DWARFVerifier.h

30

I don't like having that state which is only used for one check at the class level. Although, the name of the struct is way too generic. 'struct UnitHeaderFlags' maybe?

lib/DebugInfo/DWARF/DWARFVerifier.cpp

47–48

I wouldn't try to get a size if the unit type is not recognized. It seems wrong. I'm not sure we should try to go any further than this when we can't figure out the unit type anyway.

85

I understand that we do not really support DWARF64, but why make it completely fatal. We could still extract the length of the unit and skip it, right? Of course, I doubt many programs link DWARF64 with 32...

87–98

Why not emit the verifier errors from inside the verifyUnit() function? This would remove the need for the VerifyFlags struct. I believe it would also make the logic in verifyUnit simpler.

[sgravani]

Addressed reviewer's comments.

[sgravani]

lib/DebugInfo/DWARF/DWARFVerifier.cpp

85

I'm not exactly sure how to "skip" the unit.. I would need to be handling 64-bit offsets in order to get the next Unit in the .debug_info section. We could do that, but at the moment I prefer not to deal with DWARF64 and stop the verification when we identify a unit in the the 64-bit format.

[friss]

Just one comment:

lib/DebugInfo/DWARF/DWARFVerifier.cpp

103–104

I still don't like that isUnitDWARF64 is a member when it's used only for communicating between handleDebugInfoUnitHeaderChain() and verifyUnit(). I think I'd prefer is it was a local variable passed as an out-parameter to verifyUnit.

[sgravani]

• Made isUnitDWARF64 local variable
• Added description for verifyUnitHeader()

[friss]

LGTM thanks!

[sgravani]

lib/DebugInfo/DWARF/DWARFVerifier.cpp

103–104

Changed initialization of isUnitDWARF64 to false so that the while loop that goes over units doesn't break after a failure in verifyUnitHeader().

[sgravani]

Closed with commit: https://reviews.llvm.org/rL307975

include/llvm/DebugInfo/DWARF/DWARFUnit.h

public:
   }
 
   const DWARFAbbreviationDeclarationSet *getAbbreviations() const {
     return Abbrevs;
   }
 
   uint8_t getUnitType() const { return UnitType; }
 
+  static bool isValidUnitType(uint8_t UnitType) {
+    return UnitType == dwarf::DW_UT_compile || UnitType == dwarf::DW_UT_type ||
+           UnitType == dwarf::DW_UT_partial ||
+           UnitType == dwarf::DW_UT_skeleton ||
+           UnitType == dwarf::DW_UT_split_compile ||
+           UnitType == dwarf::DW_UT_split_type;
+  }
+
+  /// \brief Return the number of bytes for the header of a unit of
+  /// UnitType type.
+  ///
+  /// This function must be called with a valid unit type which in
+  /// DWARF5 is defined as one of the following six types.
+  static uint32_t getDWARF5HeaderSize(uint8_t UnitType) {
+    switch (UnitType) {
+    case dwarf::DW_UT_compile:
+    case dwarf::DW_UT_partial:
+      return 12;
+    case dwarf::DW_UT_skeleton:
+    case dwarf::DW_UT_split_compile:
+      return 20;
+    case dwarf::DW_UT_type:
+    case dwarf::DW_UT_split_type:
+      return 24;
+    }
+    llvm_unreachable("Invalid UnitType.");
+  }
+
   uint64_t getBaseAddress() const { return BaseAddr; }
 
   void setBaseAddress(uint64_t base_addr) {
     BaseAddr = base_addr;
   }
 
   DWARFDie getUnitDIE(bool ExtractUnitDIEOnly = true) {
     extractDIEsIfNeeded(ExtractUnitDIEOnly);

include/llvm/DebugInfo/DWARF/DWARFVerifier.h

 
 namespace llvm {
 class raw_ostream;
 struct DWARFAttribute;
 class DWARFContext;
 class DWARFDie;
 class DWARFUnit;
 class DWARFAcceleratorTable;
+class DWARFDataExtractor;
 
 /// A class that verifies DWARF debug information given a DWARF Context.
 class DWARFVerifier {
   raw_ostream &OS;
   DWARFContext &DCtx;
   /// A map that tracks all references (converted absolute references) so we
   /// can verify each reference points to a valid DIE and not an offset that
   /// lies between to valid DIEs.
   std::map<uint64_t, std::set<uint32_t>> ReferenceToDIEOffsets;
   uint32_t NumDebugInfoErrors = 0;
   uint32_t NumDebugLineErrors = 0;
   uint32_t NumAppleNamesErrors = 0;
 
+  bool isUnitDWARF64 = false;
+
+  bool verifyUnit(const DWARFDataExtractor DebugInfoData, uint32_t *Offset,
+                  unsigned UnitIndex);
   /// Verifies the attribute's DWARF attribute and its value.
   ///
   /// This function currently checks for:
   /// - DW_AT_ranges values is a valid .debug_ranges offset
   /// - DW_AT_stmt_list is a valid .debug_line offset
   ///
   /// \param Die          The DWARF DIE that owns the attribute value
   /// \param AttrValue    The DWARF attribute value to check
class DWARFVerifier {
   /// This function currently checks for:
   /// - addresses within a sequence that decrease in value
   /// - invalid file indexes
   void verifyDebugLineRows();
 
 public:
   DWARFVerifier(raw_ostream &S, DWARFContext &D)
       : OS(S), DCtx(D) {}
+  /// Verify the unit header chain in the .debug_info section.
+  ///
+  /// Any errors are reported to the stream that this object was
+  /// constructed with.
+  ///
+  /// \returns true if the unit header chain verifies successfully, false
+  /// otherwise.
+  bool handleDebugInfoUnitHeaderChain();
   /// Verify the information in the .debug_info section.
   ///
   /// Any errors are reported to the stream that was this object was
   /// constructed with.
   ///
   /// \returns true if the .debug_info verifies successfully, false otherwise.
   bool handleDebugInfo();
 

lib/DebugInfo/DWARF/DWARFContext.cpp

DWARFDie DWARFContext::getDIEForOffset(uint32_t Offset) {
   if (auto *CU = CUs.getUnitForOffset(Offset))
     return CU->getDIEForOffset(Offset);
   return DWARFDie();
 }
 
 bool DWARFContext::verify(raw_ostream &OS, DIDumpType DumpType) {
   bool Success = true;
   DWARFVerifier verifier(OS, *this);
+  if (!verifier.handleDebugInfoUnitHeaderChain())
+    Success = false;
   if (DumpType == DIDT_All || DumpType == DIDT_Info) {
     if (!verifier.handleDebugInfo())
       Success = false;
   }
   if (DumpType == DIDT_All || DumpType == DIDT_Line) {
     if (!verifier.handleDebugLine())
       Success = false;
   }
   if (DumpType == DIDT_All || DumpType == DIDT_AppleNames) {
     if (!verifier.handleAppleNames())
       Success = false;
   }
   return Success;
 }
 
 const DWARFUnitIndex &DWARFContext::getCUIndex() {
   if (CUIndex)
     return *CUIndex;
 

lib/DebugInfo/DWARF/DWARFVerifier.cpp

 #include <map>
 #include <set>
 #include <vector>
 
 using namespace llvm;
 using namespace dwarf;
 using namespace object;
 
+bool DWARFVerifier::verifyUnit(const DWARFDataExtractor DebugInfoData,
+                               uint32_t *Offset, unsigned UnitIndex) {
+  uint32_t AbbrOffset, Length;
+  uint8_t AddrSize = 0, UnitType = 0;
+  uint16_t Version;
+  bool Success = true;
+  uint32_t HeaderSize =
+      11; // means that we have only compile units in .debug_info
+
+  bool ValidLength = false;
+  bool ValidVersion = false;
+  bool ValidAddrSize = false;
+  bool ValidType = true;
+  bool ValidAbbrevOffset = true;
+
+  uint32_t OffsetStart = *Offset;
+  Length = DebugInfoData.getU32(Offset);
+  if (Length == UINT32_MAX) {
+    isUnitDWARF64 = true;
+    OS << format(
+        "Unit[%d] is in 64-bit DWARF format; cannot verify from this point.\n",
+        UnitIndex);
+    return false;
+  }
+  Version = DebugInfoData.getU16(Offset);
+
+  if (Version >= 5) {
+    UnitType = DebugInfoData.getU8(Offset);
+    AddrSize = DebugInfoData.getU8(Offset);
+    AbbrOffset = DebugInfoData.getU32(Offset);
+    ValidType = DWARFUnit::isValidUnitType(UnitType);
+    if (ValidType)
+      HeaderSize = DWARFUnit::getDWARF5HeaderSize(UnitType);
+  } else {
+    AbbrOffset = DebugInfoData.getU32(Offset);
+    AddrSize = DebugInfoData.getU8(Offset);
+  }
+
+  if (!DCtx.getDebugAbbrev()->getAbbreviationDeclarationSet(AbbrOffset))
+    ValidAbbrevOffset = false;
+
+  ValidLength = DebugInfoData.isValidOffset(OffsetStart + Length + 3);
+  ValidVersion = DWARFContext::isSupportedVersion(Version);
+  ValidAddrSize = AddrSize == 4 || AddrSize == 8;
+  if (!ValidLength || !ValidVersion || !ValidAddrSize || !ValidAbbrevOffset ||
+      !ValidType) {
+    Success = false;
+    OS << format("Units[%d] - start offset: 0x%08x \n", UnitIndex, OffsetStart);
+    if (!ValidLength)
+      OS << "\tError: The length for this unit is too "
+            "large for the .debug_info provided.\n";
+    if (!ValidVersion)
+      OS << "\tError: The 16 bit unit header version is not valid.\n";
+    if (!ValidType)
+      OS << "\tError: The unit type encoding is not valid.\n";
+    if (!ValidAbbrevOffset)
+      OS << "\tError: The offset into the .debug_abbrev section is "
+            "not valid.\n";
+    if (!ValidAddrSize)
+      OS << "\tError: The address size is unsupported.\n";
+  }
+  *Offset = OffsetStart + Length + 4;
+  return Success;
+}
+
+bool DWARFVerifier::handleDebugInfoUnitHeaderChain() {
+  OS << "Verifying .debug_info Unit Header Chain...\n";
+  bool Success = true;
+  DWARFDataExtractor DebugInfoData(DCtx.getInfoSection(), DCtx.isLittleEndian(),
+                                   0);
+  uint32_t OffsetStart, Offset = 0, UnitIdx = 0;
+  bool hasDIE = DebugInfoData.isValidOffset(Offset);
+  while (hasDIE) {
+    OffsetStart = Offset;
+    if (!verifyUnit(DebugInfoData, &Offset, UnitIdx)) {
+      Success = false;
+      if (isUnitDWARF64)
+        break;
+    }
+    hasDIE = DebugInfoData.isValidOffset(Offset);
+    ++UnitIdx;
+  }
+  if (UnitIdx == 0 && !hasDIE) {
+    OS << "Warning: .debug_info is empty.\n";
+    Success = true;
+  }
+  return Success;
+}
+
 void DWARFVerifier::verifyDebugInfoAttribute(const DWARFDie &Die,
                                              DWARFAttribute &AttrValue) {
   const auto Attr = AttrValue.Attr;
   switch (Attr) {
   case DW_AT_ranges:
     // Make sure the offset in the DW_AT_ranges attribute is valid.
     if (auto SectionOffset = AttrValue.Value.getAsSectionOffset()) {
       if (*SectionOffset >= DCtx.getRangeSection().Data.size()) {

test/tools/llvm-dwarfdump/X86/verify_unit_header_chain.s

+# RUN: llvm-mc %s -filetype obj -triple x86_64-apple-darwin -o - \
+# RUN: | not llvm-dwarfdump -verify - \
+# RUN: | FileCheck %s
+
+# CHECK: Verifying .debug_info Unit Header Chain...
+# CHECK-NEXT: Units[1] - start offset: 0x0000000d 
+# CHECK-NEXT:   Error: The unit type encoding is not valid.
+# CHECK-NEXT:   Error: The address size is unsupported.
+# CHECK-NEXT: Units[2] - start offset: 0x00000026 
+# CHECK-NEXT:   Error: The 16 bit unit header version is not valid.
+# CHECK-NEXT:   Error: The offset into the .debug_abbrev section is not valid.
+# CHECK-NEXT: Units[4] - start offset: 0x00000041 
+# CHECK-NEXT:   Error: The length for this unit is too large for the .debug_info provided.
+
+  .section  __TEXT,__text,regular,pure_instructions
+  .file 1 "basic.c"
+  .comm _i,4,2                  ## @i
+  .comm _j,4,2                  ## @j
+  .section  __DWARF,__debug_str,regular,debug
+Linfo_string:
+  .asciz  "clang version 5.0.0 (trunk 307232) (llvm/trunk 307042)" ## string offset=0
+  .asciz  "basic.c"               ## string offset=55
+  .asciz  "/Users/sgravani/Development/tests" ## string offset=63
+  .asciz  "i"                     ## string offset=97
+  .asciz  "int"                   ## string offset=99
+  .asciz  "j"                     ## string offset=103
+  .section  __DWARF,__debug_abbrev,regular,debug
+Lsection_abbrev:
+  .byte 1                       ## Abbreviation Code
+  .byte 17                      ## DW_TAG_compile_unit
+  .byte 0                       ## EOM(1)
+  .byte 0                       ## EOM(2)
+  .byte 0                       ## EOM(3)
+  .section  __DWARF,__debug_info,regular,debug
+Lsection_info:
+Lcu_begin0:
+  .long 9                      ## Length of Unit
+  .short  4                       ## DWARF version number
+Lset0 = Lsection_abbrev-Lsection_abbrev ## Offset Into Abbrev. Section
+  .long Lset0
+  .byte 4                       ## Address Size (in bytes)
+  .byte 1                       ## Abbrev [1] 0xc:0x45 DW_TAG_compile_unit
+  .byte 0                       ## End Of Children Mark
+Ltu_begin0:
+  .long 21                      ## Length of Unit
+  .short  5                       ## DWARF version number
+  .byte 0                       ## DWARF Unit Type -- Error: The unit type encoding is not valid.
+  .byte 3                       ## Address Size (in bytes) -- Error: The address size is unsupported.
+  .long 0
+  .quad 0
+  .long   0
+  .byte   0
+Lcu_begin1:
+  .long 10                      ## Length of Unit
+  .short  6                       ## DWARF version number -- Error: The 16 bit unit header version is not valid.
+  .byte 1                       ## DWARF Unit Type
+  .byte 4                       ## Address Size (in bytes) -- The offset into the .debug_abbrev section is not valid.
+  .long Lline_table_start0
+  .byte 1                       ## Abbrev [1] 0xc:0x45 DW_TAG_compile_unit
+  .byte 0                       ## End Of Children Mark
+Lcu_begin2:
+  .long 9                      ## Length of Unit
+  .short  5                       ## DWARF version number
+  .byte 1                       ## DWARF Unit Type
+  .byte 4                       ## Address Size (in bytes)
+  .long 0           ## Abbrev offset
+  .byte   0
+Ltu_begin1:
+  .long 26                      ## Length of Unit -- Error: The length for this unit is too large for the .debug_info provided.
+  .short  5                       ## DWARF version number
+  .byte 2                       ## DWARF Unit Type
+  .byte 4                       ## Address Size (in bytes)
+  .long 0
+  .quad 0
+  .long   0
+  .byte   0   
+
+.subsections_via_symbols
+  .section  __DWARF,__debug_line,regular,debug
+Lsection_line:
+Lline_table_start0: