Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

serge-sans-paille
Copy link
Collaborator

No description provided.

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 29, 2024

@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 --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<uint8_t> 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<uint8_t> NameSize = Data.slice(0, 4);
+  ArrayRef<uint8_t> 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<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;
+        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<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);
 }
@@ -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.

Copy link

github-actions bot commented Apr 29, 2024

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

@serge-sans-paille
Copy link
Collaborator Author

the buildkite failure seems unrelated

uint64_t ActualDataSize = Data.size();
if (ActualDataSize != ExpectedDataSize) {
std::string msg;
raw_string_ostream(msg) << Name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bulbazord's #80493 (reverted due to compile time increase) could simplify this.

@MaskRay MaskRay requested a review from jh7370 April 29, 2024 21:33
@MaskRay
Copy link
Member

MaskRay commented Apr 29, 2024

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.

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @MaskRay that validating added note sections specifically and unconditionally is suboptimal. However, I also can see the benefit of being able to validate .note sections (and possibly other sections, although not as part of this patch). Did you have a specific use-case in mind @serge-sans-paille (e.g. adding an invalid section caused some misery somewhere)?

I have a few competing ideas that would achieve the goal, whilst still maintaining the ability to add invalid sections:

  1. Add a new --validate-notes option that validates all .note sections in the final output (so after adding and stripping etc). Possibly "all" is unnecessary and this could be restricted to just the added ones. Pros: opt-in behaviour means users can continue to add invalid notes deliberately. Cons: existing and new users accidentally adding invalid notes will continue to do so.
  2. Same as above, but make the option the default and provide a "don't validate" option instead. Bascially the same benefits as 1, without the con of people forgetting to add the option, but inconveniences existing users who are deliberately adding an invalid section for whatever reason.
  3. Add a new --add-note-section that is the same as --add-section but specifically for notes, and would do the validation unconditionally. The existing --add-section would continue to be usable for note sections, for backwards and GNU compatibility, but wouldn't do the validation.

Various variations on the above would cover non-note sections too, e.g. --verify-sections,
or even only a specific named section (e.g. --verify-section=... or --verify-note-section=...).

llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp Outdated Show resolved Hide resolved
llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp Outdated Show resolved Hide resolved
llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp Outdated Show resolved Hide resolved
llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp Outdated Show resolved Hide resolved
llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp Outdated Show resolved Hide resolved
llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp Outdated Show resolved Hide resolved
llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp Outdated Show resolved Hide resolved
llvm/test/tools/llvm-objcopy/ELF/add-invalid-note.test Outdated Show resolved Hide resolved
llvm/test/tools/llvm-objcopy/ELF/add-invalid-note.test Outdated Show resolved Hide resolved
@serge-sans-paille
Copy link
Collaborator Author

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.

@serge-sans-paille
Copy link
Collaborator Author

@jh7370 : I ended up going to the --verify-note-sections path, as it seems to me that the most common usage is to add a valid note. I debated whether -w (as for disable warning) would be the right approach, not erroring, warning by default with a way to silent warning

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the PR title/description for the revised behaviour.

Please also update the llvm-objcopy docs and add a release note for the new option.

Finally, please don't force push to active PRs: it makes it difficult to view the difference since the last review (this is why it's against the current LLVM policy). Instead, prefer a merge from main if rebasing is needed.

@@ -623,6 +623,58 @@ 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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Random thought: I wonder if there's a way to leverage existing code in the Object library or llvm-readobj to verify the note, rather than duplicating things? (There might not be a sensible way of doing this, I haven't looked).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems possible to hook in llvm::objcopy::elf::Object::addSection, but then it would also be a good opportunity to move the conversion to SHT_NOTE based on the section name there too... Let's investigate that in a follow-up PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: one note section may contain multiple notes.
The practical example is .note.gnu.property.

There is some alignment complexity. See https://reviews.llvm.org/D150022

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always get confused about .note and alignment behaviours. Are you suggesting @MaskRay that some action needs taking here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some note sections (.note.gnu.property) have 8-byte alignment. Perhaps add a test that they are supported. @serge-sans-paille

llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp Outdated Show resolved Hide resolved
llvm/test/tools/llvm-objcopy/ELF/add-invalid-note.test Outdated Show resolved Hide resolved
llvm/tools/llvm-objcopy/ObjcopyOpts.td Outdated Show resolved Hide resolved
@serge-sans-paille serge-sans-paille changed the title [llvm-objdump] Error with relevant message when adding invalid notes [llvm-objdump] Add possibility to verify .note section format Jun 3, 2024
llvm/docs/CommandGuide/llvm-objcopy.rst Show resolved Hide resolved
llvm/docs/CommandGuide/llvm-objcopy.rst Outdated Show resolved Hide resolved
llvm/test/tools/llvm-objcopy/ELF/add-invalid-note.test Outdated Show resolved Hide resolved
llvm/test/tools/llvm-objcopy/ELF/add-invalid-note.test Outdated Show resolved Hide resolved
llvm/test/tools/llvm-objcopy/ELF/add-invalid-note.test Outdated Show resolved Hide resolved
llvm/test/tools/llvm-objcopy/ELF/add-invalid-note.test Outdated Show resolved Hide resolved
llvm/tools/llvm-objcopy/ObjcopyOptions.cpp Outdated Show resolved Hide resolved
llvm/tools/llvm-objcopy/ObjcopyOpts.td Show resolved Hide resolved
Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need adding to the release notes.

@MaskRay: what's your opinion on whether this should be enabled by default? Enabling by default means that those who are accidentally creating invalid notes will be warned about it. On the other hand we'll break people's build, if they are deliberately creating invalid ones (I don't know why you would outside of testing). My inclination is on by default, since there's an easy way to get out of the situation with the new --no-verify-note-sections option.

llvm/docs/CommandGuide/llvm-objcopy.rst Outdated Show resolved Hide resolved
llvm/docs/CommandGuide/llvm-objcopy.rst Outdated Show resolved Hide resolved
llvm/test/tools/llvm-objcopy/ELF/add-invalid-note.test Outdated Show resolved Hide resolved
llvm/test/tools/llvm-objcopy/ELF/add-invalid-note.test Outdated Show resolved Hide resolved
@MaskRay
Copy link
Member

MaskRay commented Jun 6, 2024

This will need adding to the release notes.

@MaskRay: what's your opinion on whether this should be enabled by default? Enabling by default means that those who are accidentally creating invalid notes will be warned about it. On the other hand we'll break people's build, if they are deliberately creating invalid ones (I don't know why you would outside of testing). My inclination is on by default, since there's an easy way to get out of the situation with the new --no-verify-note-sections option.

objcopy's section/symbol removal/modification features can easily break linking or cause runtime failures, but it is pretty difficult for --add-* to cause failures.

The most probably --add-section .notexxx=xxx users are distributors.
Adding another option for verification could be perceived as annoyance.
The other users use --add-section .notexxx=xxx to deliberately create invalid files to another tool, which is probably niche.
These folks are likely more amenable to additional complexity.

I agree with you that verification can be on by default.

@MaskRay
Copy link
Member

MaskRay commented Jun 6, 2024

[llvm-objdump] Add possibility to verify .note section format

The subject line is still wrong. It should be llvm-objcopy

@serge-sans-paille serge-sans-paille force-pushed the feature/objcopy-note branch 2 times, most recently from 398cd75 to c4afad6 Compare June 13, 2024 13:33
@serge-sans-paille
Copy link
Collaborator Author

(add to push --force to resolve conflict + change original commit title)

@serge-sans-paille serge-sans-paille changed the title [llvm-objdump] Add possibility to verify .note section format [llvm-objcopy] Add possibility to verify .note section format Jun 13, 2024
@serge-sans-paille
Copy link
Collaborator Author

@MaskRay : all green and comments taken into account.

@serge-sans-paille
Copy link
Collaborator Author

(rebased to fix merge conflict)

@jh7370
Copy link
Collaborator

jh7370 commented Jun 18, 2024

(rebased to fix merge conflict)

Please review the LLVM pull request guidelines: do not do a force push unless absolutely essential. You can use merge commits to merge in changes from main into your feature branch instead. Using a force push results in the old commits being wiped and it is no longer trivial to diff the change against the previous version that was reviewed.

llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp Outdated Show resolved Hide resolved
@@ -623,6 +623,58 @@ 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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always get confused about .note and alignment behaviours. Are you suggesting @MaskRay that some action needs taking here?

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't "resolve conversations" where I've instigated a change request comment. See https://discourse.llvm.org/t/rfc-github-pr-resolve-conversation-button for more context.

uint32_t NameSizeValue = *reinterpret_cast<const uint32_t *>(NameSize.data());
uint32_t DescSizeValue = *reinterpret_cast<const uint32_t *>(DescSize.data());

if (Endianness != endianness::native) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

endian::read32 should make byteswap unnecessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

@MaskRay MaskRay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but please give jh7370 time to verify

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but could you make some changes to the PR title and description as follows, please, before merging?

  1. Change the title to "[llvm-objcopy] Add verification of added .note section format". This makes it clearer that the new behaviour is on by default.
  2. Add a description outlining the new behaviour, in particular the new options, in more detail. Something like:

This commit adds a --verify-note-sections option to llvm-objcopy, which enables checking that added .note sections conform to the SHT_NOTE section format. This option is enabled by default, but can be disabled with --no-verify-note-sections.

@jh7370
Copy link
Collaborator

jh7370 commented Jul 11, 2024

P.S. It would be worth waiting before landing this to allow for the pre-merge checks to finish, so that you can make sure any failures are unrelated to your change.

@serge-sans-paille serge-sans-paille changed the title [llvm-objcopy] Add possibility to verify .note section format [llvm-objcopy] Add verification of added .note section format Jul 11, 2024
serge-sans-paille added a commit that referenced this pull request Jul 11, 2024
Also add a --no-verify-note-sections flag to make it possible to add
invalid sections if needs be.

Pull Request: #90458
@serge-sans-paille
Copy link
Collaborator Author

Commited as fb5a38b

@serge-sans-paille
Copy link
Collaborator Author

Thanks @MaskRay and @jh7370 for the throughful review.

aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
Also add a --no-verify-note-sections flag to make it possible to add
invalid sections if needs be.

Pull Request: llvm#90458
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants