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

[X86] Do not end 'note.gnu.property' section with -fcf-protection #79360

Merged
merged 2 commits into from
Jan 26, 2024

Conversation

zatrazz
Copy link
Member

@zatrazz zatrazz commented Jan 24, 2024

The glibc now adds the required minimum ISA level for libc-nonshared.a (linked on all programs) and this is done with an inline asm along with .note.gnu.property and .pushsection/.popsection. However, the x86 backend always ends the 'note.gnu.property' section when building with -fcf-protection, leading to assert failure:

llvm/llvm-project-git/llvm/lib/MC/MCStreamer.cpp:1251: virtual void llvm::MCStreamer::switchSection(llvm::MCSection*, const llvm::MCExpr*): Assertion `!Section->hasEnded() && "Section already ended"' failed.

[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/x86/isa-level.c;h=3f1b269848a52f994275bab6f60dded3ded6b144;hb=HEAD

@llvmbot llvmbot added the mc Machine (object) code label Jan 24, 2024
@llvmbot
Copy link
Member

llvmbot commented Jan 24, 2024

@llvm/pr-subscribers-mc

@llvm/pr-subscribers-backend-x86

Author: Adhemerval Zanella (zatrazz)

Changes

The glibc now adds the required minimum ISA level for libc-nonshared.a (linked on all programs) and this is done with an inline asm along with .note.gnu.property and .pushsection/.popsection. However, the x86 backend always ends the 'note.gnu.property' section when building with -fcf-protection, leading to assert failure:

llvm/llvm-project-git/llvm/lib/MC/MCStreamer.cpp:1251: virtual void llvm::MCStreamer::switchSection(llvm::MCSection*, const llvm::MCExpr*): Assertion `!Section->hasEnded() && "Section already ended"' failed.

[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/x86/isa-level.c;h=3f1b269848a52f994275bab6f60dded3ded6b144;hb=HEAD


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

2 Files Affected:

  • (modified) llvm/lib/Target/X86/X86AsmPrinter.cpp (-1)
  • (added) llvm/test/MC/AsmParser/assembler-cf-protection-inlineasm.ll (+30)
diff --git a/llvm/lib/Target/X86/X86AsmPrinter.cpp b/llvm/lib/Target/X86/X86AsmPrinter.cpp
index 9f0fd4d0938e97f..87ec8aa23080e00 100644
--- a/llvm/lib/Target/X86/X86AsmPrinter.cpp
+++ b/llvm/lib/Target/X86/X86AsmPrinter.cpp
@@ -877,7 +877,6 @@ void X86AsmPrinter::emitStartOfAsmFile(Module &M) {
       OutStreamer->emitInt32(FeatureFlagsAnd);            // data
       emitAlignment(WordSize == 4 ? Align(4) : Align(8)); // padding
 
-      OutStreamer->endSection(Nt);
       OutStreamer->switchSection(Cur);
     }
   }
diff --git a/llvm/test/MC/AsmParser/assembler-cf-protection-inlineasm.ll b/llvm/test/MC/AsmParser/assembler-cf-protection-inlineasm.ll
new file mode 100644
index 000000000000000..a0e5b4add1b386e
--- /dev/null
+++ b/llvm/test/MC/AsmParser/assembler-cf-protection-inlineasm.ll
@@ -0,0 +1,30 @@
+; RUN: llc -mtriple x86_64-unknown-linux-gnu %s -o %t.o -filetype=obj
+; RUN: llvm-readobj -n %t.o | FileCheck %s
+
+module asm ".pushsection \22.note.gnu.property\22,\22a\22,@note"
+module asm "     .p2align 3"
+module asm "     .long 1f - 0f"
+module asm "     .long 4f - 1f"
+module asm "     .long 5"
+module asm "0:   .asciz \22GNU\22"
+module asm "1:   .p2align 3"
+module asm "     .long 0xc0008002"
+module asm "     .long 3f - 2f"
+module asm "2:   .long ((1U << 0) | 0 | 0 | 0)"
+module asm "3:   .p2align 3"
+module asm "4:"
+module asm " .popsection"
+
+!llvm.module.flags = !{!0, !1}
+
+!0 = !{i32 4, !"cf-protection-return", i32 1}
+!1 = !{i32 4, !"cf-protection-branch", i32 1}
+
+; CHECK:      Type: NT_GNU_PROPERTY_TYPE_0
+; CHECK-NEXT: Property [
+; CHECK-NEXT:   x86 feature: IBT, SHSTK
+; CHECK-NEXT: ]
+; CHECK:      Type: NT_GNU_PROPERTY_TYPE_0
+; CHECK-NEXT: Property [
+; CHECK-NEXT:   x86 ISA needed: x86-64-baseline
+; CHECK-NEXT: ]

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.

Thanks for the patch. LGTM. I think a better place for the test is llvm/test/CodeGen/X86/note-cet-property-*.ll

The problem is that codegen should not end the section, not on the MCAsmParser side.

The glibc now adds the required minimum ISA level for libc-nonshared.a
(linked on all programs) and this is done with a inline asm along with
.note.gnu.property an .pushsection/.popsection.  However, x86 backend
always end the 'note.gnu.property' section when building with
-fcf-protection, leading to assert failure:

llvm/llvm-project-git/llvm/lib/MC/MCStreamer.cpp:1251: virtual void
llvm::MCStreamer::switchSection(llvm::MCSection*, const llvm::MCExpr*):
Assertion `!Section->hasEnded() && "Section already ended"' failed.

[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/x86/isa-level.c;h=3f1b269848a52f994275bab6f60dded3ded6b144;hb=HEAD
@zatrazz zatrazz force-pushed the zatrazz/cet-property-mcasm-fix branch from 6b492b2 to 240ec1a Compare January 26, 2024 17:17
@MaskRay MaskRay merged commit a58c62f into llvm:main Jan 26, 2024
3 of 4 checks passed
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Jan 27, 2024
…vm#79360)

The glibc now adds the required minimum ISA level for libc-nonshared.a
(linked on all programs) and this is done with an inline asm along with
.note.gnu.property and .pushsection/.popsection. However, the x86
backend always ends the 'note.gnu.property' section when building with
-fcf-protection, leading to assert failure:

llvm/llvm-project-git/llvm/lib/MC/MCStreamer.cpp:1251: virtual void
llvm::MCStreamer::switchSection(llvm::MCSection*, const llvm::MCExpr*):
Assertion `!Section->hasEnded() && "Section already ended"' failed.

[1]
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/x86/isa-level.c;h=3f1b269848a52f994275bab6f60dded3ded6b144;hb=HEAD

(cherry picked from commit a58c62f)
tstellar pushed a commit that referenced this pull request Jan 27, 2024
…9360)

The glibc now adds the required minimum ISA level for libc-nonshared.a
(linked on all programs) and this is done with an inline asm along with
.note.gnu.property and .pushsection/.popsection. However, the x86
backend always ends the 'note.gnu.property' section when building with
-fcf-protection, leading to assert failure:

llvm/llvm-project-git/llvm/lib/MC/MCStreamer.cpp:1251: virtual void
llvm::MCStreamer::switchSection(llvm::MCSection*, const llvm::MCExpr*):
Assertion `!Section->hasEnded() && "Section already ended"' failed.

[1]
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/x86/isa-level.c;h=3f1b269848a52f994275bab6f60dded3ded6b144;hb=HEAD

(cherry picked from commit a58c62f)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
…vm#79360)

The glibc now adds the required minimum ISA level for libc-nonshared.a
(linked on all programs) and this is done with an inline asm along with
.note.gnu.property and .pushsection/.popsection. However, the x86
backend always ends the 'note.gnu.property' section when building with
-fcf-protection, leading to assert failure:

llvm/llvm-project-git/llvm/lib/MC/MCStreamer.cpp:1251: virtual void
llvm::MCStreamer::switchSection(llvm::MCSection*, const llvm::MCExpr*):
Assertion `!Section->hasEnded() && "Section already ended"' failed.

[1]
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/x86/isa-level.c;h=3f1b269848a52f994275bab6f60dded3ded6b144;hb=HEAD

(cherry picked from commit a58c62f)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
…vm#79360)

The glibc now adds the required minimum ISA level for libc-nonshared.a
(linked on all programs) and this is done with an inline asm along with
.note.gnu.property and .pushsection/.popsection. However, the x86
backend always ends the 'note.gnu.property' section when building with
-fcf-protection, leading to assert failure:

llvm/llvm-project-git/llvm/lib/MC/MCStreamer.cpp:1251: virtual void
llvm::MCStreamer::switchSection(llvm::MCSection*, const llvm::MCExpr*):
Assertion `!Section->hasEnded() && "Section already ended"' failed.

[1]
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/x86/isa-level.c;h=3f1b269848a52f994275bab6f60dded3ded6b144;hb=HEAD

(cherry picked from commit a58c62f)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
…vm#79360)

The glibc now adds the required minimum ISA level for libc-nonshared.a
(linked on all programs) and this is done with an inline asm along with
.note.gnu.property and .pushsection/.popsection. However, the x86
backend always ends the 'note.gnu.property' section when building with
-fcf-protection, leading to assert failure:

llvm/llvm-project-git/llvm/lib/MC/MCStreamer.cpp:1251: virtual void
llvm::MCStreamer::switchSection(llvm::MCSection*, const llvm::MCExpr*):
Assertion `!Section->hasEnded() && "Section already ended"' failed.

[1]
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/x86/isa-level.c;h=3f1b269848a52f994275bab6f60dded3ded6b144;hb=HEAD

(cherry picked from commit a58c62f)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
…vm#79360)

The glibc now adds the required minimum ISA level for libc-nonshared.a
(linked on all programs) and this is done with an inline asm along with
.note.gnu.property and .pushsection/.popsection. However, the x86
backend always ends the 'note.gnu.property' section when building with
-fcf-protection, leading to assert failure:

llvm/llvm-project-git/llvm/lib/MC/MCStreamer.cpp:1251: virtual void
llvm::MCStreamer::switchSection(llvm::MCSection*, const llvm::MCExpr*):
Assertion `!Section->hasEnded() && "Section already ended"' failed.

[1]
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/x86/isa-level.c;h=3f1b269848a52f994275bab6f60dded3ded6b144;hb=HEAD

(cherry picked from commit a58c62f)
@pointhex pointhex mentioned this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants