[llvm-objcopy] Add verification of added .note section format

Also add a --no-verify-note-sections flag to make it possible to add
invalid sections if needs be.

Pull Request: #90458

Author: serge-sans-paille

llvm/docs/CommandGuide/llvm-objcopy.rst

@@ -426,6 +426,10 @@ them.
 
  The default is `default`.
 
+.. option:: --no-verify-note-sections
+
+ When adding note sections, do not verify if the section format is valid.
+
 .. option:: --output-target <format>, -O
 
  Write the output as the specified format. See `SUPPORTED FORMATS`_ for a list
@@ -516,6 +520,11 @@ them.
  specified format. See `SUPPORTED FORMATS`_ for a list of valid ``<format>``
  values.
 
+.. option:: --verify-note-sections
+
+ When adding note sections, verify if the section format is valid. On by
+ default.
+
 .. option:: --weaken-symbol <symbol>, -W
 
  Mark global symbols named ``<symbol>`` as weak symbols in the output. Can

llvm/docs/ReleaseNotes.rst

@@ -379,6 +379,10 @@ Changes to the LLVM tools
   jumping in reverse direction with shift+L/R/B). (`#95662
   <https://github.com/llvm/llvm-project/pull/95662>`_).
 
+* llvm-objcopy now verifies format of ``.note`` sections for ELF input. This can
+  be disabled by ``--no-verify-note-sections``. (`#90458
+  <https://github.com/llvm/llvm-project/pull/90458>`).
+
 Changes to LLDB
 ---------------------------------
 

llvm/include/llvm/ObjCopy/ELF/ELFConfig.h

@@ -30,6 +30,7 @@ struct ELFConfig {
   bool AllowBrokenLinks = false;
   bool KeepFileSymbols = false;
   bool LocalizeHidden = false;
+  bool VerifyNoteSections = true;
 };
 
 } // namespace objcopy

llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp

@@ -623,6 +623,54 @@ handleUserSection(const NewSectionInfo &NewSection,
   return F(NewSection.SectionName, Data);
 }
 
+static Error verifyNoteSection(StringRef Name, endianness Endianness,
+                               ArrayRef<uint8_t> Data) {
+  // An ELF note has 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<uint8_t> NameSize = Data.slice(0, 4);
+  ArrayRef<uint8_t> DescSize = Data.slice(4, 4);
+
+  uint32_t NameSizeValue = support::endian::read32(NameSize.data(), Endianness);
+  uint32_t DescSizeValue = support::endian::read32(DescSize.data(), Endianness);
+
+  uint64_t ExpectedDataSize =
+      /*NameSize=*/4 + /*DescSize=*/4 + /*Type=*/4 +
+      /*Name=*/alignTo(NameSizeValue, 4) +
+      /*Desc=*/alignTo(DescSizeValue, 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 +679,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;
@@ -702,12 +750,19 @@ 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<uint8_t> Data) {
+    auto AddSection = [&](StringRef Name, ArrayRef<uint8_t> Data) -> Error {
       OwnedDataSection &NewSection =
           Obj.addSection<OwnedDataSection>(Name, Data);
-      if (Name.starts_with(".note") && Name != ".note.GNU-stack")
+      if (Name.starts_with(".note") && Name != ".note.GNU-stack") {
         NewSection.Type = SHT_NOTE;
+        if (ELFConfig.VerifyNoteSections)
+          return verifyNoteSection(Name, E, Data);
+      }
       return Error::success();
     };
     if (Error E = handleUserSection(AddedSection, AddSection))
@@ -840,7 +895,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);
 }
@@ -858,7 +913,7 @@ Error objcopy::elf::executeObjcopyOnRawBinary(const CommonConfig &Config,
   // (-B<arch>).
   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);
 }
@@ -877,7 +932,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))

llvm/test/tools/llvm-objcopy/ELF/add-invalid-note.test

@@ -0,0 +1,54 @@
+## Verify that --add-section warns on invalid note sections.
+
+## Add [namesz, descsz, type, name, desc] for a build id.
+
+## Notes should be padded to 8 bytes.
+# RUN: printf "\x04\x00\x00\x00" >  %t-miss-padding-note.bin
+# RUN: printf "\x07\x00\x00\x00" >> %t-miss-padding-note.bin
+# RUN: printf "\x03\x00\x00\x00" >> %t-miss-padding-note.bin
+# RUN: printf "GNU\x00"          >> %t-miss-padding-note.bin
+# RUN: printf "\x0c\x0d\x0e"     >> %t-miss-padding-note.bin
+
+## The namesz field bit is incorrect.
+# RUN: printf "\x08\x00\x00\x00" >  %t-invalid-size-note.bin
+# RUN: printf "\x07\x00\x00\x00" >> %t-invalid-size-note.bin
+# RUN: printf "\x03\x00\x00\x00" >> %t-invalid-size-note.bin
+# RUN: printf "GNU\x00"          >> %t-invalid-size-note.bin
+# RUN: printf "\x0c\x0d\x0e\x00" >> %t-invalid-size-note.bin
+
+## Missing type field.
+# RUN: printf "\x08\x00\x00\x00" >  %t-short-note.bin
+# RUN: printf "\x07\x00\x00\x00" >> %t-short-note.bin
+
+# RUN: yaml2obj %s -o %t.o
+
+## Test each invalid note.
+# 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
+# CHECK-MISS-PADDING: .note.miss.padding data size must be a multiple of 4 bytes
+
+# 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
+# CHECK-INVALID-SIZE: .note.invalid.size data size is incompatible with the content of the name and description size fields: expecting 28, found 20
+
+# 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
+# CHECK-SHORT: .note.short data must be either empty or at least 12 bytes long
+
+## Test compatibility with .note.gnu.property, which has 8-byte alignment.
+# RUN: printf "\x04\x00\x00\x00\x40\x00\x00\x00\x05\x00\x00\x00\x47\x4e\x55\x00" >  %t-note-gnu-property.bin
+# RUN: printf "\x02\x00\x00\xc0\x04\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00" >> %t-note-gnu-property.bin
+# RUN: printf "\x01\x80\x01\xc0\x04\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00" >> %t-note-gnu-property.bin
+# RUN: printf "\x01\x00\x01\xc0\x04\x00\x00\x00\x0b\x00\x00\x00\x00\x00\x00\x00" >> %t-note-gnu-property.bin
+# RUN: printf "\x02\x00\x01\xc0\x04\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00" >> %t-note-gnu-property.bin
+# RUN: llvm-objcopy --add-section=.note.gnu.property=%t-note-gnu-property.bin %t.o %t-with-note-gnu-property.o
+
+## Test that verification can be disabled.
+# RUN: llvm-objcopy --add-section=.note.short=%t-short-note.bin --no-verify-note-sections %t.o %t-with-note.o
+
+## Test that last argument has precedence.
+# RUN: llvm-objcopy --add-section=.note.short=%t-short-note.bin --verify-note-sections --no-verify-note-sections %t.o %t-with-note.o
+# RUN: not llvm-objcopy --add-section=.note.short=%t-short-note.bin --no-verify-note-sections --verify-note-sections %t.o %t-with-note.o 2>&1 | FileCheck --check-prefix=CHECK-SHORT %s
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp

@@ -990,6 +990,10 @@ objcopy::parseObjcopyOptions(ArrayRef<const char *> RawArgsArr,
                              ? DiscardType::All
                              : DiscardType::Locals;
   }
+
+  ELFConfig.VerifyNoteSections = InputArgs.hasFlag(
+      OBJCOPY_verify_note_sections, OBJCOPY_no_verify_note_sections, true);
+
   Config.OnlyKeepDebug = InputArgs.hasArg(OBJCOPY_only_keep_debug);
   ELFConfig.KeepFileSymbols = InputArgs.hasArg(OBJCOPY_keep_file_symbols);
   MachOConfig.KeepUndefined = InputArgs.hasArg(OBJCOPY_keep_undefined);

llvm/tools/llvm-objcopy/ObjcopyOpts.td

@@ -1,5 +1,10 @@
 include "CommonOpts.td"
 
+multiclass B<string name, string help1, string help2> {
+  def NAME: Flag<["--"], name>, HelpText<help1>;
+  def no_ # NAME: Flag<["--"], "no-" # name>, HelpText<help2>;
+}
+
 defm binary_architecture
     : Eq<"binary-architecture", "Ignored for compatibility">;
 def B : JoinedOrSeparate<["-"], "B">,
@@ -176,6 +181,10 @@ def G : JoinedOrSeparate<["-"], "G">,
         Alias<keep_global_symbol>,
         HelpText<"Alias for --keep-global-symbol">;
 
+defm verify_note_sections : B<"verify-note-sections",
+                              "Validate note sections",
+                              "Don't validate note sections">;
+
 defm keep_global_symbols
     : Eq<"keep-global-symbols",
          "Read a list of symbols from <filename> and run as if "