[llvm-objdump] Error with relevant message when adding invalid notes

[llvmbot]

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

Author: None (serge-sans-paille)

Changes

--- Full diff: https://github.com/llvm/llvm-project/pull/90458.diff 2 Files Affected: - (modified) llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp (+64-6) - (added) llvm/test/tools/llvm-objcopy/ELF/add-invalid-note.test (+35) ``````````diff diff --git a/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp b/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp index f343d1447e0554..90e59a5228d6fe 100644 --- a/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp +++ b/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp @@ -623,6 +623,58 @@ handleUserSection(const NewSectionInfo &NewSection, return F(NewSection.SectionName, Data); } +static Error verifyNoteSection(StringRef Name, llvm::endianness E, + ArrayRef Data) { + +// An ELF note have the following structure: +// Name Size: 4 bytes (integer) +// Desc Size: 4 bytes (integer) +// Type : 4 bytes +// Name : variable size, padded to a 4 byte boundary +// Desc : variable size, padded to a 4 byte boundary + + if (Data.empty()) + return Error::success(); + + if (Data.size() < 12) { + std::string msg; + raw_string_ostream(msg) + << Name << " data must be either empty or at least 12 bytes long."; + return createStringError(errc::invalid_argument, msg); + } + if (Data.size() % 4 != 0) { + std::string msg; + raw_string_ostream(msg) + << Name << " data size must be a multiple of 4 bytes."; + return createStringError(errc::invalid_argument, msg); + } + ArrayRef NameSize = Data.slice(0, 4); + ArrayRef DescSize = Data.slice(4, 4); + + uint32_t NameSizeValue, DescSizeValue; + std::memcpy(&NameSizeValue, NameSize.data(), 4); + std::memcpy(&DescSizeValue, DescSize.data(), 4); + + if (llvm::endianness::native != E) { + NameSizeValue = byteswap(NameSizeValue); + DescSizeValue = byteswap(DescSizeValue); + } + uint64_t ExpectedDataSize = + 4 + 4 + 4 + (NameSizeValue + 3) / 4 * 4 + (DescSizeValue + 3) / 4 * 4; + uint64_t ActualDataSize = Data.size(); + if (ActualDataSize != ExpectedDataSize) { + std::string msg; + raw_string_ostream(msg) << Name + << " data size is incompatible with the content of " + "the name and description size fields:" + << " expecting " << ExpectedDataSize << ", found " + << ActualDataSize << "."; + return createStringError(errc::invalid_argument, msg); + } + + return Error::success(); +} + // This function handles the high level operations of GNU objcopy including // handling command line options. It's important to outline certain properties // we expect to hold of the command line operations. Any operation that "keeps" @@ -631,7 +683,7 @@ handleUserSection(const NewSectionInfo &NewSection, // depend a) on the order the options occur in or b) on some opaque priority // system. The only priority is that keeps/copies overrule removes. static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig, - Object &Obj) { + ElfType OutputElfType, Object &Obj) { if (Config.OutputArch) { Obj.Machine = Config.OutputArch->EMachine; Obj.OSABI = Config.OutputArch->OSABI; @@ -675,12 +727,18 @@ static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig, if (Sec.Flags & SHF_ALLOC && Sec.Type != SHT_NOTE) Sec.Type = SHT_NOBITS; + endianness E = OutputElfType == ELFT_ELF32LE || OutputElfType == ELFT_ELF64LE + ? endianness::little + : endianness::big; + for (const NewSectionInfo &AddedSection : Config.AddSection) { - auto AddSection = [&](StringRef Name, ArrayRef Data) { + auto AddSection = [&](StringRef Name, ArrayRef Data) -> Error { OwnedDataSection &NewSection = Obj.addSection(Name, Data); - if (Name.starts_with(".note") && Name != ".note.GNU-stack") + if (Name.starts_with(".note") && Name != ".note.GNU-stack") { NewSection.Type = SHT_NOTE; + return verifyNoteSection(Name, E, Data); + } return Error::success(); }; if (Error E = handleUserSection(AddedSection, AddSection)) @@ -813,7 +871,7 @@ Error objcopy::elf::executeObjcopyOnIHex(const CommonConfig &Config, const ElfType OutputElfType = getOutputElfType(Config.OutputArch.value_or(MachineInfo())); - if (Error E = handleArgs(Config, ELFConfig, **Obj)) + if (Error E = handleArgs(Config, ELFConfig, OutputElfType, **Obj)) return E; return writeOutput(Config, **Obj, Out, OutputElfType); } @@ -831,7 +889,7 @@ Error objcopy::elf::executeObjcopyOnRawBinary(const CommonConfig &Config, // (-B). const ElfType OutputElfType = getOutputElfType(Config.OutputArch.value_or(MachineInfo())); - if (Error E = handleArgs(Config, ELFConfig, **Obj)) + if (Error E = handleArgs(Config, ELFConfig, OutputElfType, **Obj)) return E; return writeOutput(Config, **Obj, Out, OutputElfType); } @@ -850,7 +908,7 @@ Error objcopy::elf::executeObjcopyOnBinary(const CommonConfig &Config, ? getOutputElfType(*Config.OutputArch) : getOutputElfType(In); - if (Error E = handleArgs(Config, ELFConfig, **Obj)) + if (Error E = handleArgs(Config, ELFConfig, OutputElfType, **Obj)) return createFileError(Config.InputFilename, std::move(E)); if (Error E = writeOutput(Config, **Obj, Out, OutputElfType)) diff --git a/llvm/test/tools/llvm-objcopy/ELF/add-invalid-note.test b/llvm/test/tools/llvm-objcopy/ELF/add-invalid-note.test new file mode 100644 index 00000000000000..396b6ebaaaf4e5 --- /dev/null +++ b/llvm/test/tools/llvm-objcopy/ELF/add-invalid-note.test @@ -0,0 +1,35 @@ +# Verify that --add-section can be used to add a note section which is +# successfully interpreted by tools that read notes. + +# Add [namesz, descsz, type, name, desc] for a build id. +# +# RUN: echo -e -n "\x04\x00\x00\x00" > %t-miss-padding-note.bin +# RUN: echo -e -n "\x07\x00\x00\x00" >> %t-miss-padding-note.bin +# RUN: echo -e -n "\x03\x00\x00\x00" >> %t-miss-padding-note.bin +# RUN: echo -e -n "GNU\x00" >> %t-miss-padding-note.bin +# RUN: echo -e -n "\x0c\x0d\x0e" >> %t-miss-padding-note.bin +# +# RUN: echo -e -n "\x08\x00\x00\x00" > %t-invalid-size-note.bin +# RUN: echo -e -n "\x07\x00\x00\x00" >> %t-invalid-size-note.bin +# RUN: echo -e -n "\x03\x00\x00\x00" >> %t-invalid-size-note.bin +# RUN: echo -e -n "GNU\x00" >> %t-invalid-size-note.bin +# RUN: echo -e -n "\x0c\x0d\x0e\x00" >> %t-invalid-size-note.bin + +# RUN: echo -e -n "\x08\x00\x00\x00" > %t-short-note.bin +# RUN: echo -e -n "\x07\x00\x00\x00" >> %t-short-note.bin + +# RUN: yaml2obj %s -o %t.o +# RUN: not llvm-objcopy --add-section=.note.miss.padding=%t-miss-padding-note.bin %t.o %t-with-note.o 2>&1 | FileCheck --check-prefix=CHECK-MISS-PADDING %s +# RUN: not llvm-objcopy --add-section=.note.invalid.size=%t-invalid-size-note.bin %t.o %t-with-note.o 2>&1 | FileCheck --check-prefix=CHECK-INVALID-SIZE %s +# RUN: not llvm-objcopy --add-section=.note.short=%t-short-note.bin %t.o %t-with-note.o 2>&1 | FileCheck --check-prefix=CHECK-SHORT %s + +!ELF +FileHeader: + Class: ELFCLASS64 + Data: ELFDATA2LSB + Type: ET_REL + Machine: EM_X86_64 + +# CHECK-MISS-PADDING: .note.miss.padding data size must be a multiple of 4 bytes. +# CHECK-INVALID-SIZE: .note.invalid.size data size is incompatible with the content of the name and description size fields: expecting 28, found 20. +# CHECK-SHORT: .note.short data must be either empty or at least 12 bytes long. ``````````

[github-actions[bot]]

:white_check_mark: With the latest revision this PR passed the C/C++ code formatter.

[serge-sans-paille]

the buildkite failure seems unrelated

[MaskRay]

I am on the fence whether there should be an error (which will set the exit code to 1 and suppress the output).

One use case for --add-section is to create intentionally invalid object files to test consumers. While we can check SHT_NOTE, it's not practical to check other section types. In addition, GNU objcopy doesn't verify the added .note.

[serge-sans-paille]

Thanks for the early comments. My use case was very pragamatic: trying to add a note, forgetting about the required padding and then wondering how to improve the situation. I like the idea of the very explicit --add-note-section that would quite naturally imply a check. @nickclifton do you think we could see that in binutils' objcopy too? That way we would avoid having (too much) diverging CLI.