Skip to content

[ObjectYAML][ELF] Report incorrect offset to generate notes #118741

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

Merged
merged 3 commits into from
Dec 23, 2024

Conversation

igorkudrin
Copy link
Collaborator

All notes in the note section must be correctly aligned, including the first. The tool should refuse to generate notes if the section offset is incorrect in this respect.

@llvmbot
Copy link
Member

llvmbot commented Dec 5, 2024

@llvm/pr-subscribers-objectyaml

Author: Igor Kudrin (igorkudrin)

Changes

All notes in the note section must be correctly aligned, including the first. The tool should refuse to generate notes if the section offset is incorrect in this respect.


Full diff: https://github.com/llvm/llvm-project/pull/118741.diff

2 Files Affected:

  • (modified) llvm/lib/ObjectYAML/ELFEmitter.cpp (+8-1)
  • (modified) llvm/test/tools/yaml2obj/ELF/note-section.yaml (+55)
diff --git a/llvm/lib/ObjectYAML/ELFEmitter.cpp b/llvm/lib/ObjectYAML/ELFEmitter.cpp
index 5daf6c32ec936a..cc41bbe6bbde24 100644
--- a/llvm/lib/ObjectYAML/ELFEmitter.cpp
+++ b/llvm/lib/ObjectYAML/ELFEmitter.cpp
@@ -1796,7 +1796,7 @@ template <class ELFT>
 void ELFState<ELFT>::writeSectionContent(Elf_Shdr &SHeader,
                                          const ELFYAML::NoteSection &Section,
                                          ContiguousBlobAccumulator &CBA) {
-  if (!Section.Notes)
+  if (!Section.Notes || Section.Notes->empty())
     return;
 
   unsigned Align;
@@ -1814,6 +1814,13 @@ void ELFState<ELFT>::writeSectionContent(Elf_Shdr &SHeader,
     return;
   }
 
+  if (CBA.getOffset() != alignTo(CBA.getOffset(), Align)) {
+    reportError(Section.Name + ": invalid offset of a note section: 0x" +
+                Twine::utohexstr(CBA.getOffset()) + ", should be aligned to " +
+                Twine(Align));
+    return;
+  }
+
   uint64_t Offset = CBA.tell();
   for (const ELFYAML::NoteEntry &NE : *Section.Notes) {
     // Write name size.
diff --git a/llvm/test/tools/yaml2obj/ELF/note-section.yaml b/llvm/test/tools/yaml2obj/ELF/note-section.yaml
index 10fd36fefeabfd..e48c323f594da6 100644
--- a/llvm/test/tools/yaml2obj/ELF/note-section.yaml
+++ b/llvm/test/tools/yaml2obj/ELF/note-section.yaml
@@ -514,3 +514,58 @@ Sections:
         Desc: 030405
       - Name: GNU
         Type: NT_GNU_BUILD_ID
+
+## Check that an incorrect offset for generating notes is reported.
+
+# RUN: not yaml2obj --docnum=19 %s 2>&1 | FileCheck %s --check-prefix=ERR_OFFSET
+# ERR_OFFSET: error: .note: invalid offset of a note section: 0x{{.*}}, should be aligned to 4
+
+--- !ELF
+FileHeader:
+  Class: ELFCLASS32
+  Data:  ELFDATA2LSB
+  Type:  ET_EXEC
+Sections:
+  - Name: .dummy
+    Type: SHT_PROGBITS
+    Size: 1
+  - Name: .note
+    Type: SHT_NOTE
+    Notes:
+      - Type: 0x1
+
+## Do not issue an error if the notes array is empty
+
+# RUN: yaml2obj --docnum=20 %s -o - | \
+# RUN:   llvm-readobj --sections --section-data - | \
+# RUN:   FileCheck %s --check-prefix=TEST20
+
+# TEST20:      Section {
+# TEST20:      Name: .note
+# TEST20-NEXT:   Type: SHT_NOTE
+# TEST20-NEXT:   Flags [ (0x0)
+# TEST20-NEXT:   ]
+# TEST20-NEXT:   Address:
+# TEST20-NEXT:   Offset:
+# TEST20-NEXT:   Size: 0
+# TEST20-NEXT:   Link:
+# TEST20-NEXT:   Info:
+# TEST20-NEXT:   AddressAlignment: 5
+# TEST20-NEXT:   EntrySize:
+# TEST20-NEXT:   SectionData (
+# TEST20-NEXT:   )
+# TEST20-NEXT: }
+
+--- !ELF
+FileHeader:
+  Class: ELFCLASS32
+  Data:  ELFDATA2LSB
+  Type:  ET_EXEC
+Sections:
+  - Name: .dummy
+    Type: SHT_PROGBITS
+    Size: 1
+  - Name: .note
+    Type: SHT_NOTE
+    AddressAlign: 5
+    Notes: []

All notes in the note section must be correctly aligned, including the
first. The tool should refuse to generate notes if the section offset is
incorrect in this respect.
@igorkudrin igorkudrin force-pushed the yaml2obj-notes-misalignment-fix branch from 729b634 to 3df90c7 Compare December 5, 2024 05:35
@MaskRay
Copy link
Member

MaskRay commented Dec 5, 2024

If the alignment is not 0 or a power of 2, it seems fine to report an error even if it is empty.

@igorkudrin
Copy link
Collaborator Author

igorkudrin commented Dec 5, 2024

If the alignment is not 0 or a power of 2, it seems fine to report an error even if it is empty.

As @jh7370 said, the tool should be permissive, so I believe we should only report an error if we cannot generate the notes in a reasonable way. There is at least one test (llvm/test/tools/llvm-size/elf-sysv.test) that would need to be updated if the check for the empty list was not added.

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'm not sure I understand what the benefit is of adding this error to yaml2obj? With the error added, how would somebody generate a note section that wasn't correctly aligned?

@igorkudrin
Copy link
Collaborator Author

I'm not sure I understand what the benefit is of adding this error to yaml2obj?

This helps to prevent the accidental generation of invalid note entries. If the current offset in CBA is not aligned as expected, the namesz, descsz, type, and name fields of the first entry would be aligned differently than its descriptor and all other note entries in the section, making the first entry malformed.

With the error added, how would somebody generate a note section that wasn't correctly aligned?

At least, they can still use Content: instead of Notes:, or an empty array for Notes:.

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.

Basically looks fine to me. I've suggested one small tweak to the test.

If we find the error message to get in the way of a genuine use case, we can look to modify/remove it in the future.

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, sorry for the delayed follow-up review.

@igorkudrin igorkudrin merged commit ba37309 into llvm:main Dec 23, 2024
8 checks passed
@igorkudrin igorkudrin deleted the yaml2obj-notes-misalignment-fix branch December 23, 2024 21:22
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