[llvm-dwp] Fix a possible out of bound access.

[ikudrin]

Summary

llvm-dwp did not check section identifiers read from input files. In the case of an unexpected identifier, the calculated index for Contributions[] pointed outside the array. This fix avoids the issue by skipping unsupported identifiers.

[dblaikie]

Thanks!

Does the test need the debug_info section? (Probably needs them for some reason, might be worth mentioning)
Seems like the test should also have a tu_index to test the change there?

[ikudrin]

• The test was extended to add a TU index case.
• Added a comment to the test that the additional sections are required to reach the test points in the code.

[jhenderson]

llvm/tools/llvm-dwp/llvm-dwp.cpp

220

You've got test cases for Kind < DW_SEC_INFO, but you probably also need cases for Kind > DW_SECT_MACRO.

250–252

I've not really looked around this code, so this might not make sense, but is it also worth checking that we don't emit data for these sections?

[ikudrin]

• Extended the test by adding an unknown ID which is greater than DW_SECT_MACRO.

llvm/tools/llvm-dwp/llvm-dwp.cpp

250–252

llvm-dwp does not emit anything for unknown sections and the patch is not changing that.

[jhenderson]

Thanks. Looks good from my point of view, but @dblaikie should confirm he's happy.

[dblaikie]

llvm/test/tools/llvm-dwp/X86/unknown-section-id.s

2–9

What does llvm-dwp do insteand of out of bounds access? Testing for "does anything other than crash" is usually a bad sign - presumably there's some specific behavior that we'd like llvm-dwp to have in these cases and should be testing for it here.

[ikudrin]

llvm/test/tools/llvm-dwp/X86/unknown-section-id.s

2–9

It just ignores the unknown sections because there is little it can do with them anyway.

[jhenderson]

llvm/test/tools/llvm-dwp/X86/unknown-section-id.s

2–9

Could we demonstrate that it does something else after the failure point and also that it doesn't try to dump the unknown sections?

[ikudrin]

llvm/test/tools/llvm-dwp/X86/unknown-section-id.s

2–9

Nothing comes to my mind. llvm-dwp creates the output, as usual. It ignores unknown sections, as usual. The only change is that it does not write to random memory locations, but that is not something that is easy to demonstrate; only UBSan catches that. Do you have any particular suggestions?

[jhenderson]

llvm/test/tools/llvm-dwp/X86/unknown-section-id.s

2–9

Perhaps at least show that the output contains only the expected sections maybe (not their contents)?

[ikudrin]

• Extended the test.

[jhenderson]

Looks good, though probably should get someone else to confirm.

[dblaikie]

Looks good - thanks!

llvm/test/tools/llvm-dwp/X86/unknown-section-id.s

+## The test checks that llvm-dwp avoids an out of bound access when there is
+## an unknown section identifier in an index section. Without the fix, the test
+## failed when LLVM is built with UBSan.
+## Note that additional sections (.debug_abbrev.dwo, .debug_info.dwo, and
+## .debug_types.dwo) are required to reach the test points in the code.
+
+# RUN: llvm-mc -triple x86_64-unknown-linux-gnu %s -filetype=obj -o %t.dwp
+# RUN: llvm-dwp %t.dwp -o -
+
+.section .debug_abbrev.dwo, "e", @progbits
+.LCUAbbrevBegin:
+    .uleb128 1                      # Abbreviation Code
+    .uleb128 0x11                   # DW_TAG_compile_unit
+    .byte 1                         # DW_CHILDREN_no
+    .uleb128 0x2131                 # DW_AT_GNU_dwo_id
+    .uleb128 7                      # DW_FORM_data8
+    .byte 0                         # EOM(1)
+    .byte 0                         # EOM(2)
+    .byte 0                         # EOM(3)
+.LCUAbbrevEnd:
+
+.LTUAbbrevBegin:
+    .uleb128 1                      # Abbreviation Code
+    .uleb128 0x41                   # DW_TAG_type_unit
+    .byte 1                         # DW_CHILDREN_yes
+    .byte 0                         # EOM(1)
+    .byte 0                         # EOM(2)
+    .uleb128 2                      # Abbreviation Code
+    .uleb128 0x13                   # DW_TAG_structure_type
+    .byte 0                         # DW_CHILDREN_no
+    .byte 0                         # EOM(1)
+    .byte 0                         # EOM(2)
+    .byte 0                         # EOM(3)
+.LTUAbbrevEnd:
+
+    .section .debug_info.dwo, "e", @progbits
+.LCUBegin:
+    .long .LCUEnd-.LCUVersion       # Length of Unit
+.LCUVersion:
+    .short 4                        # Version
+    .long 0                         # Abbrev offset
+    .byte 8                         # Address size
+    .uleb128 1                      # Abbrev [1] DW_TAG_compile_unit
+    .quad 0x1100002222222222        # DW_AT_GNU_dwo_id
+.LCUEnd:
+
+    .section .debug_types.dwo, "e", @progbits
+.LTUBegin:
+    .long .LTUEnd-.LTUVersion       # Length of Unit
+.LTUVersion:
+    .short 4                        # Version
+    .long 0                         # Abbrev offset
+    .byte 8                         # Address size
+    .quad 0x1100003333333333        # Type signature
+    .long .LTUType-.LTUBegin        # Type offset
+    .uleb128 1                      # Abbrev [1] DW_TAG_type_unit
+.LTUType:
+    .uleb128 2                      # Abbrev [2] DW_TAG_structure_type
+.LTUEnd:
+
+    .section .debug_cu_index, "", @progbits
+## Header:
+    .long 2                         # Version
+    .long 3                         # Section count
+    .long 1                         # Unit count
+    .long 2                         # Slot count
+## Hash Table of Signatures:
+    .quad 0x1100002222222222
+    .quad 0
+## Parallel Table of Indexes:
+    .long 1
+    .long 0
+## Table of Section Offsets:
+## Row 0:
+    .long 1                         # DW_SECT_INFO
+    .long 3                         # DW_SECT_ABBREV
+    .long 0                         # Invalid (unknown) section identifier
+## Row 1:
+    .long 0                         # Offset in .debug_info.dwo
+    .long 0                         # Offset in .debug_abbrev.dwo
+    .long 0
+## Table of Section Sizes:
+    .long .LCUEnd-.LCUBegin         # Size in .debug_info.dwo
+    .long .LCUAbbrevEnd-.LCUAbbrevBegin # Size in .debug_abbrev.dwo
+    .long 1
+
+    .section .debug_tu_index, "", @progbits
+## Header:
+    .long 2                         # Version
+    .long 3                         # Section count
+    .long 1                         # Unit count
+    .long 2                         # Slot count
+## Hash Table of Signatures:
+    .quad 0x1100003333333333
+    .quad 0
+## Parallel Table of Indexes:
+    .long 1
+    .long 0
+## Table of Section Offsets:
+## Row 0:
+    .long 2                         # DW_SECT_TYPES
+    .long 3                         # DW_SECT_ABBREV
+    .long 0                         # Invalid (unknown) section identifier
+## Row 1:
+    .long 0                         # Offset in .debug_types.dwo
+    .long 0                         # Offset in .debug_abbrev.dwo
+    .long 0
+## Table of Section Sizes:
+    .long .LTUEnd-.LTUBegin         # Size in .debug_types.dwo
+    .long .LTUAbbrevEnd-.LTUAbbrevBegin # Size in .debug_abbrev.dwo
+    .long 1

llvm/tools/llvm-dwp/llvm-dwp.cpp

 
 struct UnitIndexEntry {
   DWARFUnitIndex::Entry::SectionContribution Contributions[8];
   std::string Name;
   std::string DWOName;
   StringRef DWPName;
 };
 
+static bool isSupportedSectionKind(DWARFSectionKind Kind) {
+  return Kind >= DW_SECT_INFO && Kind <= DW_SECT_MACRO;
+}
+
 static StringRef getSubsection(StringRef Section,
                                const DWARFUnitIndex::Entry &Entry,
                                DWARFSectionKind Kind) {
   const auto *Off = Entry.getOffset(Kind);
   if (!Off)
     return StringRef();
   return Section.substr(Off->Offset, Off->Length);
 }
if (!I)
       continue;
     auto P = TypeIndexEntries.insert(std::make_pair(E.getSignature(), TUEntry));
     if (!P.second)
       continue;
     auto &Entry = P.first->second;
     // Zero out the debug_info contribution
     Entry.Contributions[0] = {};
     for (auto Kind : TUIndex.getColumnKinds()) {
+      if (!isSupportedSectionKind(Kind))
+        continue;
       auto &C = Entry.Contributions[Kind - DW_SECT_INFO];
       C.Offset += I->Offset;
       C.Length = I->Length;
       ++I;
     }
     auto &C = Entry.Contributions[DW_SECT_TYPES - DW_SECT_INFO];
     Out.emitBytes(Types.substr(
         C.Offset - TUEntry.Contributions[DW_SECT_TYPES - DW_SECT_INFO].Offset,
         C.Length));
     C.Offset = TypesOffset;
     TypesOffset += C.Length;
for (const DWARFUnitIndex::Entry &E : CUIndex.getRows()) {
       const auto &ID = *EID;
       if (!P.second)
         return buildDuplicateError(*P.first, ID, Input);
       auto &NewEntry = P.first->second;
       NewEntry.Name = ID.Name;
       NewEntry.DWOName = ID.DWOName;
       NewEntry.DWPName = Input;
       for (auto Kind : CUIndex.getColumnKinds()) {
+        if (!isSupportedSectionKind(Kind))
+          continue;
         auto &C = NewEntry.Contributions[Kind - DW_SECT_INFO];
         C.Offset += I->Offset;
         C.Length = I->Length;
         ++I;
       }
     }
 
     if (!CurTypesSection.empty()) {