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

[lld][AArch64] Add support for GCS #90732

Merged
merged 6 commits into from
May 21, 2024
Merged

Conversation

john-brawn-arm
Copy link
Collaborator

@john-brawn-arm john-brawn-arm commented May 1, 2024

This adds the -z gcs and -z gcs-report options, which behave similarly to -z shtk and -z cet-report, except that -z gcs accepts a parameter:

  • -z gcs=implicit is the default behaviour, where the GCS bit is inferred from the input objects.
  • -z gcs=never clears the GCS bit, ignoring the input objects.
  • -z gcs=always sets the GCS bit, ignoring the input objects.

This is so that there's a means of explicitly disabling GCS even when all input objects have the GCS bit set.

This adds the -z gcs and -z gcs-report options, which behave similarly
to -z shtk and -z cet-report, except that -z gcs accepts a parameter:
 * -z gcs=implicit is the default behaviour, where the GCS bit is
   inferred from the input objects.
 * -z gcs=never clears the GCS bit, ignoring the input objects.
 * -z gcs=always sets the GCS bit, ignoring the input objects.
 * -z gcs is the same as -z gcs=always.

This is so that there's a means of explicitly disabling GCS even when
all input objects have the GCS bit set.
@llvmbot
Copy link
Collaborator

llvmbot commented May 1, 2024

@llvm/pr-subscribers-lld-elf

@llvm/pr-subscribers-lld

Author: John Brawn (john-brawn-arm)

Changes

This adds the -z gcs and -z gcs-report options, which behave similarly to -z shtk and -z cet-report, except that -z gcs accepts a parameter:

  • -z gcs=implicit is the default behaviour, where the GCS bit is inferred from the input objects.
  • -z gcs=never clears the GCS bit, ignoring the input objects.
  • -z gcs=always sets the GCS bit, ignoring the input objects.
  • -z gcs is the same as -z gcs=always.

This is so that there's a means of explicitly disabling GCS even when all input objects have the GCS bit set.


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

5 Files Affected:

  • (modified) lld/ELF/Config.h (+2)
  • (modified) lld/ELF/Driver.cpp (+37-1)
  • (added) lld/test/ELF/Inputs/aarch64-func2-gcs.s (+19)
  • (added) lld/test/ELF/Inputs/aarch64-func3-gcs.s (+16)
  • (added) lld/test/ELF/aarch64-feature-gcs.s (+66)
diff --git a/lld/ELF/Config.h b/lld/ELF/Config.h
index 33bfa42b0fcbf0..8279cd4aafc16f 100644
--- a/lld/ELF/Config.h
+++ b/lld/ELF/Config.h
@@ -188,6 +188,8 @@ struct Config {
   StringRef zBtiReport = "none";
   StringRef zCetReport = "none";
   StringRef zPauthReport = "none";
+  StringRef zGcsReport = "none";
+  StringRef zGcs = "implicit";
   bool ltoBBAddrMap;
   llvm::StringRef ltoBasicBlockSections;
   std::pair<llvm::StringRef, llvm::StringRef> thinLTOObjectSuffixReplace;
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index a5b47f020f8726..b5dab029f110e0 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -466,6 +466,10 @@ static void checkOptions() {
       error("-z bti-report only supported on AArch64");
     if (config->zPauthReport != "none")
       error("-z pauth-report only supported on AArch64");
+    if (config->zGcsReport != "none")
+      error("-z gcs-report only supported on AArch64");
+    if (config->zGcs != "implicit")
+      error("-z gcs only supported on AArch64");
   }
 
   if (config->emachine != EM_386 && config->emachine != EM_X86_64 &&
@@ -560,6 +564,25 @@ static uint8_t getZStartStopVisibility(opt::InputArgList &args) {
   return ret;
 }
 
+static StringRef getZGcs(opt::InputArgList &args) {
+  StringRef ret = "implicit";
+  for (auto *arg : args.filtered(OPT_z)) {
+    std::pair<StringRef, StringRef> kv = StringRef(arg->getValue()).split('=');
+    if (kv.first == "gcs") {
+      arg->claim();
+      if (kv.second == "implicit" || kv.second == "always" ||
+          kv.second == "never")
+        ret = kv.second;
+      else if (StringRef(arg->getValue()) == "gcs")
+        // -z gcs is the same as -z gcs=always
+        ret = "always";
+      else
+        error("unknown -z gcs= value: " + StringRef(kv.second));
+    }
+  }
+  return ret;
+}
+
 // Report a warning for an unknown -z option.
 static void checkZOptions(opt::InputArgList &args) {
   // This function is called before getTarget(), when certain options are not
@@ -1436,6 +1459,7 @@ static void readConfigs(opt::InputArgList &args) {
   config->zCopyreloc = getZFlag(args, "copyreloc", "nocopyreloc", true);
   config->zForceBti = hasZOption(args, "force-bti");
   config->zForceIbt = hasZOption(args, "force-ibt");
+  config->zGcs = getZGcs(args);
   config->zGlobal = hasZOption(args, "global");
   config->zGnustack = getZGnuStack(args);
   config->zHazardplt = hasZOption(args, "hazardplt");
@@ -1508,7 +1532,8 @@ static void readConfigs(opt::InputArgList &args) {
 
   auto reports = {std::make_pair("bti-report", &config->zBtiReport),
                   std::make_pair("cet-report", &config->zCetReport),
-                  std::make_pair("pauth-report", &config->zPauthReport)};
+                  std::make_pair("pauth-report", &config->zPauthReport),
+                  std::make_pair("gcs-report", &config->zGcsReport)};
   for (opt::Arg *arg : args.filtered(OPT_z)) {
     std::pair<StringRef, StringRef> option =
         StringRef(arg->getValue()).split('=');
@@ -2667,6 +2692,11 @@ static void readSecurityNotes() {
         toString(f) + ": -z bti-report: file does not have "
                       "GNU_PROPERTY_AARCH64_FEATURE_1_BTI property");
 
+    checkAndReportMissingFeature(
+        config->zGcsReport, features, GNU_PROPERTY_AARCH64_FEATURE_1_GCS,
+        toString(f) + ": -z gcs-report: file does not have "
+                      "GNU_PROPERTY_AARCH64_FEATURE_1_GCS property");
+
     checkAndReportMissingFeature(
         config->zCetReport, features, GNU_PROPERTY_X86_FEATURE_1_IBT,
         toString(f) + ": -z cet-report: file does not have "
@@ -2719,6 +2749,12 @@ static void readSecurityNotes() {
   // Force enable Shadow Stack.
   if (config->zShstk)
     config->andFeatures |= GNU_PROPERTY_X86_FEATURE_1_SHSTK;
+
+  // Force enable/disable GCS
+  if (config->zGcs == "always")
+    config->andFeatures |= GNU_PROPERTY_AARCH64_FEATURE_1_GCS;
+  else if (config->zGcs == "never")
+    config->andFeatures &= ~GNU_PROPERTY_AARCH64_FEATURE_1_GCS;
 }
 
 static void initSectionsAndLocalSyms(ELFFileBase *file, bool ignoreComdats) {
diff --git a/lld/test/ELF/Inputs/aarch64-func2-gcs.s b/lld/test/ELF/Inputs/aarch64-func2-gcs.s
new file mode 100644
index 00000000000000..4e7e95dcaff7cc
--- /dev/null
+++ b/lld/test/ELF/Inputs/aarch64-func2-gcs.s
@@ -0,0 +1,19 @@
+.section ".note.gnu.property", "a"
+.long 4
+.long 0x10
+.long 0x5
+.asciz "GNU"
+
+.long 0xc0000000 // GNU_PROPERTY_AARCH64_FEATURE_1_AND
+.long 4
+.long 4          // GNU_PROPERTY_AARCH64_FEATURE_1_GCS
+.long 0
+
+.text
+.globl func2
+.type func2,@function
+func2:
+  .globl func3
+  .type func3, @function
+  bl func3
+  ret
diff --git a/lld/test/ELF/Inputs/aarch64-func3-gcs.s b/lld/test/ELF/Inputs/aarch64-func3-gcs.s
new file mode 100644
index 00000000000000..28460338822cdb
--- /dev/null
+++ b/lld/test/ELF/Inputs/aarch64-func3-gcs.s
@@ -0,0 +1,16 @@
+.section ".note.gnu.property", "a"
+.long 4
+.long 0x10
+.long 0x5
+.asciz "GNU"
+
+.long 0xc0000000 // GNU_PROPERTY_AARCH64_FEATURE_1_AND
+.long 4
+.long 4          // GNU_PROPERTY_AARCH64_FEATURE_1_GCS
+.long 0
+
+.text
+.globl func3
+.type func3,@function
+func3:
+  ret
diff --git a/lld/test/ELF/aarch64-feature-gcs.s b/lld/test/ELF/aarch64-feature-gcs.s
new file mode 100644
index 00000000000000..31cb3b4cdee9c8
--- /dev/null
+++ b/lld/test/ELF/aarch64-feature-gcs.s
@@ -0,0 +1,66 @@
+# REQUIRES: aarch64
+# RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu %s -o %t1.o
+# RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu %p/Inputs/aarch64-func2-gcs.s -o %t2.o
+# RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu %p/Inputs/aarch64-func3-gcs.s -o %t3.o
+# RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu %p/Inputs/aarch64-func2.s -o %t2no.o
+# RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu %p/Inputs/aarch64-func3.s -o %t3no.o
+
+# GCS should be enabled when it's enabled in all inputs or when it's forced on.
+
+# RUN: ld.lld %t1.o %t2.o %t3.o --shared -o %t.exe
+# RUN: llvm-readelf -n %t.exe | FileCheck --check-prefix GCS %s
+# RUN: ld.lld %t1.o %t3.o --shared -o %t.so
+# RUN: llvm-readelf -n %t.so | FileCheck --check-prefix GCS %s
+# RUN: ld.lld %t1.o %t2no.o %t3.o --shared -o %tforce.exe -z gcs
+# RUN: llvm-readelf -n %tforce.exe | FileCheck --allow-empty --check-prefix GCS %s
+# RUN: ld.lld %t2.o %t3no.o --shared -o %tforce.so -z gcs=always
+# RUN: llvm-readelf -n %tforce.so | FileCheck --allow-empty --check-prefix GCS %s
+# RUN: ld.lld %t2.o %t3no.o --shared -o %tforce2.so -z gcs=never -z gcs=always
+# RUN: llvm-readelf -n %tforce2.so | FileCheck --allow-empty --check-prefix GCS %s
+
+# GCS: Properties:    aarch64 feature: GCS
+
+# GCS should not be enabled if it's not enabled in at least one input, and we
+# should warn or error when using the report option.
+
+# RUN: ld.lld %t1.o %t2no.o %t3.o --shared -o %tno.exe
+# RUN: llvm-readelf -n %tno.exe | FileCheck --allow-empty --check-prefix NOGCS %s
+# RUN: ld.lld %t2.o %t3no.o --shared -o %tno.so
+# RUN: llvm-readelf -n %tno.so | FileCheck --allow-empty --check-prefix NOGCS %s
+# RUN: ld.lld %t1.o %t2no.o %t3.o --shared -o %tno.exe -z gcs-report=warning 2>&1 | FileCheck --check-prefix=REPORT-WARN %s
+# RUN: llvm-readelf -n %tno.exe | FileCheck --allow-empty --check-prefix NOGCS %s
+# RUN: not ld.lld %t2.o %t3no.o --shared -o %tno.so -z gcs-report=error 2>&1 | FileCheck --check-prefix=REPORT-ERROR %s
+
+# NOGCS-NOT: Properties
+# REPORT-WARN: warning: {{.*}}tmp2no.o: -z gcs-report: file does not have GNU_PROPERTY_AARCH64_FEATURE_1_GCS property
+# REPORT-ERROR: error: {{.*}}tmp3no.o: -z gcs-report: file does not have GNU_PROPERTY_AARCH64_FEATURE_1_GCS property
+
+# GCS should be disabled with gcs=never, even if GCS is present in all inputs.
+
+# RUN: ld.lld %t1.o %t2.o %t3.o -z gcs=never --shared -o %tnever.exe
+# RUN: llvm-readelf -n %tnever.exe | FileCheck --allow-empty --check-prefix NOGCS %s
+# RUN: ld.lld %t1.o %t2.o %t3.o -z gcs=always -z gcs=never --shared -o %tnever2.exe
+# RUN: llvm-readelf -n %tnever2.exe | FileCheck --allow-empty --check-prefix NOGCS %s
+
+# An invalid gcs option should give an error
+# RUN: not ld.lld %t1.o %t2.o %t3.o -z gcs=nonsense -o %tinvalid.exe 2>&1 | FileCheck --check-prefix=INVALID %s
+
+# INVALID: error: unknown -z gcs= value: nonsense
+
+.section ".note.gnu.property", "a"
+.long 4
+.long 0x10
+.long 0x5
+.asciz "GNU"
+
+.long 0xc0000000 // GNU_PROPERTY_AARCH64_FEATURE_1_AND
+.long 4
+.long 4          // GNU_PROPERTY_AARCH64_FEATURE_1_GCS
+.long 0
+
+.text
+.globl _start
+.type func1,%function
+func1:
+  bl func2
+  ret

@MaskRay
Copy link
Member

MaskRay commented May 1, 2024

Thanks for the patch. Looks quite good.

I've a question about the alias: -z gcs is the same as -z gcs=always.

Is there a strong need for it? It's quite uncommon to have -z option aliases and I'd avoid if there is no compatibility concern.

@john-brawn-arm
Copy link
Collaborator Author

Thanks for the patch. Looks quite good.

I've a question about the alias: -z gcs is the same as -z gcs=always.

Is there a strong need for it? It's quite uncommon to have -z option aliases and I'd avoid if there is no compatibility concern.

-z gcs is there to be consistent will all the other similar options to do with the GNU_PROPERTY bits, where there's a -z thing option which forces the GNU_PROPERTY_THING bit to be set.

Copy link
Member

@DanielKristofKiss DanielKristofKiss left a comment

Choose a reason for hiding this comment

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

LGMT

@MaskRay
Copy link
Member

MaskRay commented May 3, 2024

Thanks for the patch. Looks quite good.
I've a question about the alias: -z gcs is the same as -z gcs=always.
Is there a strong need for it? It's quite uncommon to have -z option aliases and I'd avoid if there is no compatibility concern.

-z gcs is there to be consistent will all the other similar options to do with the GNU_PROPERTY bits, where there's a -z thing option which forces the GNU_PROPERTY_THING bit to be set.

Other options setting GNU_PROPERTY are quite different in names, -z force-bti, -z force-ibt.
For -z xxx without =, there is typically a -z noxxx, but I don't think we want to add -z nogcs.

To the best of my knowledge,

  • There is no -z alias options
  • The few aliases linkers have are for -z vs non-z options, which might be related to SunOS/Solaris heritage. In newer options there is very few (if any) alias.

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.

.

lld/test/ELF/aarch64-feature-gcs.s Outdated Show resolved Hide resolved
lld/test/ELF/aarch64-feature-gcs.s Outdated Show resolved Hide resolved
lld/ELF/Driver.cpp Outdated Show resolved Hide resolved
lld/ELF/Driver.cpp Outdated Show resolved Hide resolved
lld/ELF/Driver.cpp Outdated Show resolved Hide resolved
lld/test/ELF/aarch64-feature-gcs.s Outdated Show resolved Hide resolved
@john-brawn-arm
Copy link
Collaborator Author

Thanks for the patch. Looks quite good.
I've a question about the alias: -z gcs is the same as -z gcs=always.
Is there a strong need for it? It's quite uncommon to have -z option aliases and I'd avoid if there is no compatibility concern.

-z gcs is there to be consistent will all the other similar options to do with the GNU_PROPERTY bits, where there's a -z thing option which forces the GNU_PROPERTY_THING bit to be set.

Other options setting GNU_PROPERTY are quite different in names, -z force-bti, -z force-ibt. For -z xxx without =, there is typically a -z noxxx, but I don't think we want to add -z nogcs.

It looks like there's a mix:

  • With force-: force-bti, force-ibt
  • Without force-: pac-plt, shstk

The current name is what I've agreed with the people working on the GNU support for this (https://sourceware.org/git/?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/ARM/gcs-binutils-gdb-master, though that currently has experimental- in the option name which we've agreed should be removed), I'll ask them what they think about not having -z gcs mean the same as -z gcs=always.

@MaskRay
Copy link
Member

MaskRay commented May 10, 2024

Thanks for the patch. Looks quite good.
I've a question about the alias: -z gcs is the same as -z gcs=always.
Is there a strong need for it? It's quite uncommon to have -z option aliases and I'd avoid if there is no compatibility concern.

-z gcs is there to be consistent will all the other similar options to do with the GNU_PROPERTY bits, where there's a -z thing option which forces the GNU_PROPERTY_THING bit to be set.

Other options setting GNU_PROPERTY are quite different in names, -z force-bti, -z force-ibt. For -z xxx without =, there is typically a -z noxxx, but I don't think we want to add -z nogcs.

It looks like there's a mix:

  • With force-: force-bti, force-ibt
  • Without force-: pac-plt, shstk

The current name is what I've agreed with the people working on the GNU support for this (sourceware.org/git/?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/ARM/gcs-binutils-gdb-master, though that currently has experimental- in the option name which we've agreed should be removed), I'll ask them what they think about not having -z gcs mean the same as -z gcs=always.

Thanks for the additional context. Please forward my remark about the alias to them :)

The existence of an alias -z gcs could make people wonder whether it is always or implicit.

@john-brawn-arm
Copy link
Collaborator Author

There were no objections to removing the alias, so I've done so.

# RUN: not ld.lld func2-gcs.o func3.o --shared -o /dev/null -z gcs-report=error -z gcs=always 2>&1 | FileCheck --check-prefix=REPORT-ERROR %s
# RUN: not ld.lld func2-gcs.o func3.o --shared -o /dev/null -z gcs-report=error -z gcs=never 2>&1 | FileCheck --check-prefix=REPORT-ERROR %s
# RUN: ld.lld func1-gcs.o func2-gcs.o func3-gcs.o -o /dev/null -z gcs-report=warning | FileCheck --allow-empty --check-prefix=REPORT-NONE %s
# RUN: ld.lld func1-gcs.o func2-gcs.o func3-gcs.o -o /dev/null -z gcs-report=warning -z gcs=always | FileCheck --allow-empty --check-prefix=REPORT-NONE %s
Copy link
Member

Choose a reason for hiding this comment

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

2>&1 | count 0 instead of --allow-empty

@john-brawn-arm john-brawn-arm merged commit cfeb25c into llvm:main May 21, 2024
3 of 4 checks passed
@john-brawn-arm john-brawn-arm deleted the lld_gcs branch May 21, 2024 16:34
mub-at-arm pushed a commit to mub-at-arm/llvm-project that referenced this pull request May 22, 2024
This adds the -z gcs and -z gcs-report options, which behave similarly
to -z shtk and -z cet-report, except that -z gcs accepts a parameter:
* -z gcs=implicit is the default behaviour, where the GCS bit is
inferred from the input objects.
 * -z gcs=never clears the GCS bit, ignoring the input objects.
 * -z gcs=always sets the GCS bit, ignoring the input objects.

This is so that there's a means of explicitly disabling GCS even when
all input objects have the GCS bit set.
VyacheslavLevytskyy pushed a commit to VyacheslavLevytskyy/llvm-project that referenced this pull request May 22, 2024
This adds the -z gcs and -z gcs-report options, which behave similarly
to -z shtk and -z cet-report, except that -z gcs accepts a parameter:
* -z gcs=implicit is the default behaviour, where the GCS bit is
inferred from the input objects.
 * -z gcs=never clears the GCS bit, ignoring the input objects.
 * -z gcs=always sets the GCS bit, ignoring the input objects.

This is so that there's a means of explicitly disabling GCS even when
all input objects have the GCS bit set.
jameshu15869 pushed a commit to jameshu15869/llvm-project that referenced this pull request May 31, 2024
This adds the -z gcs and -z gcs-report options, which behave similarly
to -z shtk and -z cet-report, except that -z gcs accepts a parameter:
* -z gcs=implicit is the default behaviour, where the GCS bit is
inferred from the input objects.
 * -z gcs=never clears the GCS bit, ignoring the input objects.
 * -z gcs=always sets the GCS bit, ignoring the input objects.

This is so that there's a means of explicitly disabling GCS even when
all input objects have the GCS bit set.
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.

None yet

4 participants