From 689715f335aeffc0e9583ac1b2a5629b6dd47876 Mon Sep 17 00:00:00 2001 From: Fangrui Song Date: Wed, 10 May 2023 09:36:58 -0700 Subject: [PATCH] [Object] Fix handling of Elf_Nhdr with sh_addralign=8 The generic ABI says: > Padding is present, if necessary, to ensure 8 or 4-byte alignment for the next note entry (depending on whether the file is a 64-bit or 32-bit object). Such padding is not included in descsz. Our parsing code currently aligns n_namesz. Fix the bug by aligning the start offset of the descriptor instead. This issue has been benign because the primary uses of sh_addralign=8 notes are `.note.gnu.property`, where `sizeof(Elf_Nhdr) + sizeof("GNU") = 16` (already aligned by 8). In practice, many 64-bit systems incorrectly use sh_addralign=4 notes. We can use sh_addralign (= p_align) to decide the descriptor padding. Treat an alignment of 0 and 1 as 4. This approach matches modern GNU readelf (since 2018). We have a few tests incorrectly using sh_addralign=0. We may make our behavior stricter after fixing these tests. Linux kernel dumped core files use `p_align=0` notes, so we need to support the case for compatibility. Reviewed By: jhenderson Differential Revision: https://reviews.llvm.org/D150022 --- lld/ELF/InputFiles.cpp | 9 +++-- llvm/include/llvm/Object/ELF.h | 21 +++++++++- llvm/include/llvm/Object/ELFTypes.h | 36 ++++++++--------- llvm/lib/Object/BuildID.cpp | 2 +- .../ELF/AArch64/aarch64-note-gnu-property.s | 1 - .../llvm-readobj/ELF/note-alignment-invalid.test | 21 ++++++++++ .../tools/llvm-readobj/ELF/note-gnu-property2.s | 7 ++-- llvm/test/tools/llvm-readobj/ELF/note-unknown.s | 46 +++++++++++++++++++++- llvm/tools/llvm-readobj/ELFDumper.cpp | 22 ++++++----- llvm/tools/obj2yaml/elf2yaml.cpp | 7 ++-- llvm/unittests/Object/ELFTypesTest.cpp | 14 +++---- 11 files changed, 136 insertions(+), 50 deletions(-) create mode 100644 llvm/test/tools/llvm-readobj/ELF/note-alignment-invalid.test diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp index b8291fb..311e2c9 100644 --- a/lld/ELF/InputFiles.cpp +++ b/lld/ELF/InputFiles.cpp @@ -885,12 +885,13 @@ template static uint32_t readAndFeatures(const InputSection &sec) { while (!data.empty()) { // Read one NOTE record. auto *nhdr = reinterpret_cast(data.data()); - if (data.size() < sizeof(Elf_Nhdr) || data.size() < nhdr->getSize()) + if (data.size() < sizeof(Elf_Nhdr) || + data.size() < nhdr->getSize(sec.addralign)) reportFatal(data.data(), "data is too short"); Elf_Note note(*nhdr); if (nhdr->n_type != NT_GNU_PROPERTY_TYPE_0 || note.getName() != "GNU") { - data = data.slice(nhdr->getSize()); + data = data.slice(nhdr->getSize(sec.addralign)); continue; } @@ -899,7 +900,7 @@ template static uint32_t readAndFeatures(const InputSection &sec) { : GNU_PROPERTY_X86_FEATURE_1_AND; // Read a body of a NOTE record, which consists of type-length-value fields. - ArrayRef desc = note.getDesc(); + ArrayRef desc = note.getDesc(sec.addralign); while (!desc.empty()) { const uint8_t *place = desc.data(); if (desc.size() < 8) @@ -924,7 +925,7 @@ template static uint32_t readAndFeatures(const InputSection &sec) { } // Go to next NOTE record to look for more FEATURE_1_AND descriptions. - data = data.slice(nhdr->getSize()); + data = data.slice(nhdr->getSize(sec.addralign)); } return featuresSet; diff --git a/llvm/include/llvm/Object/ELF.h b/llvm/include/llvm/Object/ELF.h index 2ae2e9f..a1cf47a1 100644 --- a/llvm/include/llvm/Object/ELF.h +++ b/llvm/include/llvm/Object/ELF.h @@ -316,7 +316,16 @@ public: ") or size (0x" + Twine::utohexstr(Phdr.p_filesz) + ")"); return Elf_Note_Iterator(Err); } - return Elf_Note_Iterator(base() + Phdr.p_offset, Phdr.p_filesz, Err); + // Allow 4, 8, and (for Linux core dumps) 0. + // TODO: Disallow 1 after all tests are fixed. + if (Phdr.p_align != 0 && Phdr.p_align != 1 && Phdr.p_align != 4 && + Phdr.p_align != 8) { + Err = + createError("alignment (" + Twine(Phdr.p_align) + ") is not 4 or 8"); + return Elf_Note_Iterator(Err); + } + return Elf_Note_Iterator(base() + Phdr.p_offset, Phdr.p_filesz, + std::max(Phdr.p_align, 4), Err); } /// Get an iterator over notes in a section. @@ -335,7 +344,15 @@ public: ") or size (0x" + Twine::utohexstr(Shdr.sh_size) + ")"); return Elf_Note_Iterator(Err); } - return Elf_Note_Iterator(base() + Shdr.sh_offset, Shdr.sh_size, Err); + // TODO: Allow just 4 and 8 after all tests are fixed. + if (Shdr.sh_addralign != 0 && Shdr.sh_addralign != 1 && + Shdr.sh_addralign != 4 && Shdr.sh_addralign != 8) { + Err = createError("alignment (" + Twine(Shdr.sh_addralign) + + ") is not 4 or 8"); + return Elf_Note_Iterator(Err); + } + return Elf_Note_Iterator(base() + Shdr.sh_offset, Shdr.sh_size, + std::max(Shdr.sh_addralign, 4), Err); } /// Get the end iterator for notes. diff --git a/llvm/include/llvm/Object/ELFTypes.h b/llvm/include/llvm/Object/ELFTypes.h index 62c251e..608b15b 100644 --- a/llvm/include/llvm/Object/ELFTypes.h +++ b/llvm/include/llvm/Object/ELFTypes.h @@ -599,15 +599,13 @@ struct Elf_Nhdr_Impl { Elf_Word n_descsz; Elf_Word n_type; - /// The alignment of the name and descriptor. - /// - /// Implementations differ from the specification here: in practice all - /// variants align both the name and descriptor to 4-bytes. - static const unsigned int Align = 4; - - /// Get the size of the note, including name, descriptor, and padding. - size_t getSize() const { - return sizeof(*this) + alignTo(n_namesz) + alignTo(n_descsz); + /// Get the size of the note, including name, descriptor, and padding. Both + /// the start and the end of the descriptor are aligned by the section + /// alignment. In practice many 64-bit systems deviate from the generic ABI by + /// using sh_addralign=4. + size_t getSize(size_t Align) const { + return alignToPowerOf2(sizeof(*this) + n_namesz, Align) + + alignToPowerOf2(n_descsz, Align); } }; @@ -635,18 +633,18 @@ public: } /// Get the note's descriptor. - ArrayRef getDesc() const { + ArrayRef getDesc(size_t Align) const { if (!Nhdr.n_descsz) return ArrayRef(); return ArrayRef( - reinterpret_cast(&Nhdr) + sizeof(Nhdr) + - alignTo::Align>(Nhdr.n_namesz), + reinterpret_cast(&Nhdr) + + alignToPowerOf2(sizeof(Nhdr) + Nhdr.n_namesz, Align), Nhdr.n_descsz); } /// Get the note's descriptor as StringRef - StringRef getDescAsStringRef() const { - ArrayRef Desc = getDesc(); + StringRef getDescAsStringRef(size_t Align) const { + ArrayRef Desc = getDesc(Align); return StringRef(reinterpret_cast(Desc.data()), Desc.size()); } @@ -666,6 +664,7 @@ private: // Nhdr being a nullptr marks the end of iteration. const Elf_Nhdr_Impl *Nhdr = nullptr; size_t RemainingSize = 0u; + size_t Align = 0; Error *Err = nullptr; template friend class ELFFile; @@ -693,7 +692,7 @@ private: stopWithOverflowError(); else { Nhdr = reinterpret_cast *>(NhdrPos + NoteSize); - if (Nhdr->getSize() > RemainingSize) + if (Nhdr->getSize(Align) > RemainingSize) stopWithOverflowError(); else *Err = Error::success(); @@ -702,8 +701,9 @@ private: Elf_Note_Iterator_Impl() = default; explicit Elf_Note_Iterator_Impl(Error &Err) : Err(&Err) {} - Elf_Note_Iterator_Impl(const uint8_t *Start, size_t Size, Error &Err) - : RemainingSize(Size), Err(&Err) { + Elf_Note_Iterator_Impl(const uint8_t *Start, size_t Size, size_t Align, + Error &Err) + : RemainingSize(Size), Align(Align), Err(&Err) { consumeError(std::move(Err)); assert(Start && "ELF note iterator starting at NULL"); advanceNhdr(Start, 0u); @@ -713,7 +713,7 @@ public: Elf_Note_Iterator_Impl &operator++() { assert(Nhdr && "incremented ELF note end iterator"); const uint8_t *NhdrPos = reinterpret_cast(Nhdr); - size_t NoteSize = Nhdr->getSize(); + size_t NoteSize = Nhdr->getSize(Align); advanceNhdr(NhdrPos, NoteSize); return *this; } diff --git a/llvm/lib/Object/BuildID.cpp b/llvm/lib/Object/BuildID.cpp index 887798a..ef21458 100644 --- a/llvm/lib/Object/BuildID.cpp +++ b/llvm/lib/Object/BuildID.cpp @@ -36,7 +36,7 @@ template BuildIDRef getBuildID(const ELFFile &Obj) { for (auto N : Obj.notes(P, Err)) if (N.getType() == ELF::NT_GNU_BUILD_ID && N.getName() == ELF::ELF_NOTE_GNU) - return N.getDesc(); + return N.getDesc(P.p_align); consumeError(std::move(Err)); } return {}; diff --git a/llvm/test/tools/llvm-readobj/ELF/AArch64/aarch64-note-gnu-property.s b/llvm/test/tools/llvm-readobj/ELF/AArch64/aarch64-note-gnu-property.s index 4f280ff..872a3f1 100644 --- a/llvm/test/tools/llvm-readobj/ELF/AArch64/aarch64-note-gnu-property.s +++ b/llvm/test/tools/llvm-readobj/ELF/AArch64/aarch64-note-gnu-property.s @@ -24,7 +24,6 @@ // LLVM-NEXT: ] .section ".note.gnu.property", "a" -.align 4 .long 4 /* Name length is always 4 ("GNU") */ .long end - begin /* Data length */ .long 5 /* Type: NT_GNU_PROPERTY_TYPE_0 */ diff --git a/llvm/test/tools/llvm-readobj/ELF/note-alignment-invalid.test b/llvm/test/tools/llvm-readobj/ELF/note-alignment-invalid.test new file mode 100644 index 0000000..2b7221c --- /dev/null +++ b/llvm/test/tools/llvm-readobj/ELF/note-alignment-invalid.test @@ -0,0 +1,21 @@ +# RUN: yaml2obj --docnum=1 %s -o %t1.so +# RUN: llvm-readelf --notes %t1.so 2>&1 | FileCheck %s -DFILE=%t1.so +# RUN: llvm-readobj --notes %t1.so 2>&1 | FileCheck %s -DFILE=%t1.so + +# CHECK: warning: '[[FILE]]': unable to read notes from the SHT_NOTE section with index 1: alignment (6) is not 4 or 8 + +--- !ELF +FileHeader: + Class: ELFCLASS64 + Data: ELFDATA2LSB + Type: ET_EXEC +Sections: + - Name: .note.invalid + Type: SHT_NOTE + AddressAlign: 0x0000000000000006 + Content: 0400000004000000cdab0000474E550061626364 +ProgramHeaders: + - Type: PT_NOTE + FileSize: 0x20 + FirstSec: .note.invalid + LastSec: .note.invalid diff --git a/llvm/test/tools/llvm-readobj/ELF/note-gnu-property2.s b/llvm/test/tools/llvm-readobj/ELF/note-gnu-property2.s index b6c0104..5ac35d4 100644 --- a/llvm/test/tools/llvm-readobj/ELF/note-gnu-property2.s +++ b/llvm/test/tools/llvm-readobj/ELF/note-gnu-property2.s @@ -6,13 +6,13 @@ // GNU: Displaying notes found in: .note.gnu.property // GNU-NEXT: Owner Data size Description // GNU-NEXT: GNU 0x00000004 NT_GNU_PROPERTY_TYPE_0 (property note) -// GNU-NEXT: Properties: +// GNU-NEXT: Properties: // LLVM: Notes [ // LLVM-NEXT: NoteSection { // LLVM-NEXT: Name: .note.gnu.property // LLVM-NEXT: Offset: 0x40 -// LLVM-NEXT: Size: 0x14 +// LLVM-NEXT: Size: 0x18 // LLVM-NEXT: Note { // LLVM-NEXT: Owner: GNU // LLVM-NEXT: Data size: 0x4 @@ -29,10 +29,11 @@ .section ".note.gnu.property", "a" .align 4 .long 4 /* Name length is always 4 ("GNU") */ - .long end - begin /* Data length */ + .long 4 /* Data length (corrupted) */ .long 5 /* Type: NT_GNU_PROPERTY_TYPE_0 */ .asciz "GNU" /* Name */ .p2align 3 begin: .long 1 /* Type: GNU_PROPERTY_STACK_SIZE */ + .p2align 3 end: diff --git a/llvm/test/tools/llvm-readobj/ELF/note-unknown.s b/llvm/test/tools/llvm-readobj/ELF/note-unknown.s index 27f8f8e..aa74b51 100644 --- a/llvm/test/tools/llvm-readobj/ELF/note-unknown.s +++ b/llvm/test/tools/llvm-readobj/ELF/note-unknown.s @@ -39,21 +39,63 @@ // LLVM-NEXT: ) // LLVM-NEXT: } // LLVM-NEXT: } +// LLVM-NEXT: NoteSection { +// LLVM-NEXT: Name: .note.8 +// LLVM-NEXT: Offset: 0x80 +// LLVM-NEXT: Size: 0x40 +// LLVM-NEXT: Note { +// LLVM-NEXT: Owner: WXYZ +// LLVM-NEXT: Data size: 0x8 +// LLVM-NEXT: Type: Unknown (0x00000006) +// LLVM-NEXT: Description data ( +// LLVM-NEXT: 0000: 4C6F7265 6D000000 |Lorem...| +// LLVM-NEXT: ) +// LLVM-NEXT: } +// LLVM-NEXT: Note { +// LLVM-NEXT: Owner: VWXYZ +// LLVM-NEXT: Data size: 0x8 +// LLVM-NEXT: Type: Unknown (0x00000006) +// LLVM-NEXT: Description data ( +// LLVM-NEXT: 0000: 78787800 00000000 |xxx.....| +// LLVM-NEXT: ) +// LLVM-NEXT: } +// LLVM-NEXT: } // LLVM-NEXT: ] .section ".note.foo", "a" - .align 4 .long 4 /* namesz */ .long 0 /* descsz */ .long 3 /* type */ .asciz "XYZ" + .align 4 .section ".note.bar", "a" - .align 4 .long 4 /* namesz */ .long end - begin /* descsz */ .long 3 /* type */ .asciz "XYZ" + .align 4 begin: .asciz "Lorem ipsum dolor sit amet" .align 4 end: + +.section ".note.8", "a" + .long 5 /* namesz */ + .long 2f - 1f /* descsz */ + .long 6 /* type */ + .asciz "WXYZ" + .align 8 +1: + .asciz "Lorem" + .align 8 +2: + + .long 6 /* namesz */ + .long 2f - 1f /* descsz */ + .long 6 /* type */ + .asciz "VWXYZ" + .align 8 +1: + .asciz "xxx" + .align 8 +2: diff --git a/llvm/tools/llvm-readobj/ELFDumper.cpp b/llvm/tools/llvm-readobj/ELFDumper.cpp index eddffa0..b2b835d 100644 --- a/llvm/tools/llvm-readobj/ELFDumper.cpp +++ b/llvm/tools/llvm-readobj/ELFDumper.cpp @@ -5828,7 +5828,7 @@ template static void processNotesHelper( const ELFDumper &Dumper, llvm::function_ref, typename ELFT::Off, - typename ELFT::Addr)> + typename ELFT::Addr, size_t)> StartNotesFn, llvm::function_ref ProcessNoteFn, llvm::function_ref FinishNotesFn) { @@ -5841,7 +5841,7 @@ static void processNotesHelper( if (S.sh_type != SHT_NOTE) continue; StartNotesFn(expectedToStdOptional(Obj.getSectionName(S)), S.sh_offset, - S.sh_size); + S.sh_size, S.sh_addralign); Error Err = Error::success(); size_t I = 0; for (const typename ELFT::Note Note : Obj.notes(S, Err)) { @@ -5872,7 +5872,7 @@ static void processNotesHelper( const typename ELFT::Phdr &P = (*PhdrsOrErr)[I]; if (P.p_type != PT_NOTE) continue; - StartNotesFn(/*SecName=*/std::nullopt, P.p_offset, P.p_filesz); + StartNotesFn(/*SecName=*/std::nullopt, P.p_offset, P.p_filesz, P.p_align); Error Err = Error::success(); size_t Index = 0; for (const typename ELFT::Note Note : Obj.notes(P, Err)) { @@ -5892,10 +5892,12 @@ static void processNotesHelper( } template void GNUELFDumper::printNotes() { + size_t Align = 0; bool IsFirstHeader = true; auto PrintHeader = [&](std::optional SecName, const typename ELFT::Off Offset, - const typename ELFT::Addr Size) { + const typename ELFT::Addr Size, size_t Al) { + Align = std::max(Al, 4); // Print a newline between notes sections to match GNU readelf. if (!IsFirstHeader) { OS << '\n'; @@ -5916,7 +5918,7 @@ template void GNUELFDumper::printNotes() { auto ProcessNote = [&](const Elf_Note &Note, bool IsCore) -> Error { StringRef Name = Note.getName(); - ArrayRef Descriptor = Note.getDesc(); + ArrayRef Descriptor = Note.getDesc(Align); Elf_Word Type = Note.getType(); // Print the note owner/type. @@ -6048,7 +6050,7 @@ template void ELFDumper::printMemtag() { auto FindAndroidNote = [&](const Elf_Note &Note, bool IsCore) -> Error { if (Note.getName() == "Android" && Note.getType() == ELF::NT_ANDROID_TYPE_MEMTAG) - AndroidNoteDesc = Note.getDesc(); + AndroidNoteDesc = Note.getDesc(4); return Error::success(); }; @@ -6056,7 +6058,7 @@ template void ELFDumper::printMemtag() { *this, /*StartNotesFn=*/ [](std::optional, const typename ELFT::Off, - const typename ELFT::Addr) {}, + const typename ELFT::Addr, size_t) {}, /*ProcessNoteFn=*/FindAndroidNote, /*FinishNotesFn=*/[]() {}); ArrayRef Contents = getMemtagGlobalsSectionContents(MemtagGlobals); @@ -7590,9 +7592,11 @@ template void LLVMELFDumper::printNotes() { ListScope L(W, "Notes"); std::unique_ptr NoteScope; + size_t Align = 0; auto StartNotes = [&](std::optional SecName, const typename ELFT::Off Offset, - const typename ELFT::Addr Size) { + const typename ELFT::Addr Size, size_t Al) { + Align = std::max(Al, 4); NoteScope = std::make_unique(W, "NoteSection"); W.printString("Name", SecName ? *SecName : ""); W.printHex("Offset", Offset); @@ -7604,7 +7608,7 @@ template void LLVMELFDumper::printNotes() { auto ProcessNote = [&](const Elf_Note &Note, bool IsCore) -> Error { DictScope D2(W, "Note"); StringRef Name = Note.getName(); - ArrayRef Descriptor = Note.getDesc(); + ArrayRef Descriptor = Note.getDesc(Align); Elf_Word Type = Note.getType(); // Print the note owner/type. diff --git a/llvm/tools/obj2yaml/elf2yaml.cpp b/llvm/tools/obj2yaml/elf2yaml.cpp index 9df483b..b261b9d 100644 --- a/llvm/tools/obj2yaml/elf2yaml.cpp +++ b/llvm/tools/obj2yaml/elf2yaml.cpp @@ -1221,6 +1221,7 @@ ELFDumper::dumpNoteSection(const Elf_Shdr *Shdr) { std::vector Entries; ArrayRef Content = *ContentOrErr; + size_t Align = std::max(Shdr->sh_addralign, 4); while (!Content.empty()) { if (Content.size() < sizeof(Elf_Nhdr)) { S->Content = yaml::BinaryRef(*ContentOrErr); @@ -1228,16 +1229,16 @@ ELFDumper::dumpNoteSection(const Elf_Shdr *Shdr) { } const Elf_Nhdr *Header = reinterpret_cast(Content.data()); - if (Content.size() < Header->getSize()) { + if (Content.size() < Header->getSize(Align)) { S->Content = yaml::BinaryRef(*ContentOrErr); return S.release(); } Elf_Note Note(*Header); Entries.push_back( - {Note.getName(), Note.getDesc(), (ELFYAML::ELF_NT)Note.getType()}); + {Note.getName(), Note.getDesc(Align), (ELFYAML::ELF_NT)Note.getType()}); - Content = Content.drop_front(Header->getSize()); + Content = Content.drop_front(Header->getSize(Align)); } S->Notes = std::move(Entries); diff --git a/llvm/unittests/Object/ELFTypesTest.cpp b/llvm/unittests/Object/ELFTypesTest.cpp index 265f8a7..aae71ef 100644 --- a/llvm/unittests/Object/ELFTypesTest.cpp +++ b/llvm/unittests/Object/ELFTypesTest.cpp @@ -19,9 +19,9 @@ template struct NoteTestData { const Elf_Note_Impl getElfNote(StringRef Name, uint32_t Type, ArrayRef Desc) { - Data.resize(sizeof(Elf_Nhdr_Impl) + - alignTo::Align>(Name.size()) + - alignTo::Align>(Desc.size()), + constexpr uint64_t Align = 4; + Data.resize(alignTo(sizeof(Elf_Nhdr_Impl) + Name.size(), Align) + + alignTo(Desc.size(), Align), 0); Elf_Nhdr_Impl *Nhdr = @@ -34,7 +34,7 @@ template struct NoteTestData { std::copy(Name.begin(), Name.end(), NameOffset); auto DescOffset = - NameOffset + alignTo::Align>(Nhdr->n_namesz); + Data.begin() + alignTo(sizeof(*Nhdr) + Nhdr->n_namesz, Align); std::copy(Desc.begin(), Desc.end(), DescOffset); return Elf_Note_Impl(*Nhdr); @@ -50,8 +50,8 @@ TEST(ELFTypesTest, NoteTest) { RandomData); EXPECT_EQ(Note1.getName(), "AMD"); EXPECT_EQ(Note1.getType(), ELF::NT_AMDGPU_METADATA); - EXPECT_EQ(Note1.getDesc(), RandomData); - EXPECT_EQ(Note1.getDescAsStringRef(), + EXPECT_EQ(Note1.getDesc(4), RandomData); + EXPECT_EQ(Note1.getDescAsStringRef(4), StringRef(reinterpret_cast(Random), sizeof(Random))); auto Note2 = TestData.getElfNote("", ELF::NT_AMDGPU_METADATA, RandomData); @@ -59,5 +59,5 @@ TEST(ELFTypesTest, NoteTest) { auto Note3 = TestData.getElfNote("AMD", ELF::NT_AMDGPU_METADATA, ArrayRef()); - EXPECT_EQ(Note3.getDescAsStringRef(), StringRef("")); + EXPECT_EQ(Note3.getDescAsStringRef(4), StringRef("")); } -- 2.7.4