[PAC][clang] Add signed GOT cc1 flag #96160
Conversation
kovdan01
requested review from
damyanp,
MaskRay,
sudonatalie,
asl and
smithp35
and removed request for
damyanp and
MaskRay
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Daniil Kovalev (kovdan01) ChangesDepends on #96159 Add Full diff: https://github.com/llvm/llvm-project/pull/96160.diff 8 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index 1ca2cb85565a1..28667b1eb239e 100644
--- a/clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -742,6 +742,10 @@ def warn_drv_fjmc_for_elf_only : Warning<
"-fjmc works only for ELF; option ignored">,
InGroup<OptionIgnored>;
+def warn_drv_ptrauth_elf_got_for_elf_only : Warning<
+ "-fptrauth-elf-got works only for ELF; option ignored">,
+ InGroup<OptionIgnored>;
+
def warn_target_override_arm64ec : Warning<
"/arm64EC has been overridden by specified target: %0; option ignored">,
InGroup<OptionIgnored>;
diff --git a/clang/include/clang/Basic/Features.def b/clang/include/clang/Basic/Features.def
index 53f410d3cb4bd..569f4e1715af5 100644
--- a/clang/include/clang/Basic/Features.def
+++ b/clang/include/clang/Basic/Features.def
@@ -110,6 +110,7 @@ FEATURE(ptrauth_vtable_pointer_address_discrimination, LangOpts.PointerAuthVTPtr
FEATURE(ptrauth_vtable_pointer_type_discrimination, LangOpts.PointerAuthVTPtrTypeDiscrimination)
FEATURE(ptrauth_member_function_pointer_type_discrimination, LangOpts.PointerAuthCalls)
FEATURE(ptrauth_init_fini, LangOpts.PointerAuthInitFini)
+FEATURE(ptrauth_elf_got, LangOpts.PointerAuthELFGOT)
EXTENSION(swiftcc,
PP.getTargetInfo().checkCallingConvention(CC_Swift) ==
clang::TargetInfo::CCCR_OK)
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 112eb286eb075..e16c1a0d06a1b 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -4222,6 +4222,7 @@ defm ptrauth_vtable_pointer_address_discrimination :
defm ptrauth_vtable_pointer_type_discrimination :
OptInCC1FFlag<"ptrauth-vtable-pointer-type-discrimination", "Enable type discrimination of vtable pointers">;
defm ptrauth_init_fini : OptInCC1FFlag<"ptrauth-init-fini", "Enable signing of function pointers in init/fini arrays">;
+defm ptrauth_elf_got : OptInCC1FFlag<"ptrauth-elf-got", "Enable authentication of pointers from GOT (ELF only)">;
}
def fenable_matrix : Flag<["-"], "fenable-matrix">, Group<f_Group>,
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 331cf6e713d89..5f55e79ec206b 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -1788,6 +1788,13 @@ void Clang::AddAArch64TargetArgs(const ArgList &Args,
options::OPT_fno_ptrauth_vtable_pointer_type_discrimination);
Args.addOptInFlag(CmdArgs, options::OPT_fptrauth_init_fini,
options::OPT_fno_ptrauth_init_fini);
+
+ Args.addOptInFlag(CmdArgs, options::OPT_fptrauth_elf_got,
+ options::OPT_fno_ptrauth_elf_got);
+
+ if (Args.hasArg(options::OPT_fptrauth_elf_got))
+ getToolChain().getDriver().Diag(
+ diag::warn_drv_ptrauth_elf_got_for_elf_only);
}
void Clang::AddLoongArchTargetArgs(const ArgList &Args,
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 58694e5399d58..97a5408a4c1e0 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -3361,6 +3361,8 @@ static void GeneratePointerAuthArgs(const LangOptions &Opts,
GenerateArg(Consumer, OPT_fptrauth_vtable_pointer_type_discrimination);
if (Opts.PointerAuthInitFini)
GenerateArg(Consumer, OPT_fptrauth_init_fini);
+ if (Opts.PointerAuthELFGOT)
+ GenerateArg(Consumer, OPT_fptrauth_elf_got);
}
static void ParsePointerAuthArgs(LangOptions &Opts, ArgList &Args,
@@ -3374,6 +3376,7 @@ static void ParsePointerAuthArgs(LangOptions &Opts, ArgList &Args,
Opts.PointerAuthVTPtrTypeDiscrimination =
Args.hasArg(OPT_fptrauth_vtable_pointer_type_discrimination);
Opts.PointerAuthInitFini = Args.hasArg(OPT_fptrauth_init_fini);
+ Opts.PointerAuthELFGOT = Args.hasArg(OPT_fptrauth_elf_got);
}
/// Check if input file kind and language standard are compatible.
@@ -4728,6 +4731,7 @@ bool CompilerInvocation::CreateFromArgsImpl(
ParseAPINotesArgs(Res.getAPINotesOpts(), Args, Diags);
ParsePointerAuthArgs(LangOpts, Args, Diags);
+ LangOpts.PointerAuthELFGOT &= T.isOSBinFormatELF();
ParseLangArgs(LangOpts, Args, DashX, T, Res.getPreprocessorOpts().Includes,
Diags);
diff --git a/clang/test/CodeGen/aarch64-elf-pauthabi.c b/clang/test/CodeGen/aarch64-elf-pauthabi.c
index aa83ee3e0d7b0..df675563b331f 100644
--- a/clang/test/CodeGen/aarch64-elf-pauthabi.c
+++ b/clang/test/CodeGen/aarch64-elf-pauthabi.c
@@ -5,7 +5,8 @@
// RUN: -fptrauth-auth-traps \
// RUN: -fptrauth-vtable-pointer-address-discrimination \
// RUN: -fptrauth-vtable-pointer-type-discrimination \
-// RUN: -fptrauth-init-fini %s | \
+// RUN: -fptrauth-init-fini \
+// RUN: -fptrauth-elf-got %s | \
// RUN: FileCheck %s --check-prefix=ALL
// RUN: %clang_cc1 -triple aarch64-linux -emit-llvm -o - \
@@ -32,8 +33,11 @@
// RUN: -fptrauth-calls -fptrauth-init-fini %s | \
// RUN: FileCheck %s --check-prefix=INITFINI
+// RUN: %clang --target=aarch64-linux -S -emit-llvm -o - \
+// RUN: -fptrauth-elf-got %s | FileCheck %s --check-prefix=ELFGOT
+
// ALL: !{i32 1, !"aarch64-elf-pauthabi-platform", i32 268435458}
-// ALL: !{i32 1, !"aarch64-elf-pauthabi-version", i32 127}
+// ALL: !{i32 1, !"aarch64-elf-pauthabi-version", i32 255}
// INTRIN: !{i32 1, !"aarch64-elf-pauthabi-platform", i32 268435458}
// INTRIN: !{i32 1, !"aarch64-elf-pauthabi-version", i32 1}
@@ -56,4 +60,7 @@
// INITFINI: !{i32 1, !"aarch64-elf-pauthabi-platform", i32 268435458}
// INITFINI: !{i32 1, !"aarch64-elf-pauthabi-version", i32 66}
+// ELFGOT: !{i32 1, !"aarch64-elf-pauthabi-platform", i32 268435458}
+// ELFGOT: !{i32 1, !"aarch64-elf-pauthabi-version", i32 128}
+
void foo() {}
diff --git a/clang/test/Driver/aarch64-ptrauth.c b/clang/test/Driver/aarch64-ptrauth.c
index fa0125f4b22a9..e568ea4843e9b 100644
--- a/clang/test/Driver/aarch64-ptrauth.c
+++ b/clang/test/Driver/aarch64-ptrauth.c
@@ -13,9 +13,15 @@
// RUN: %s 2>&1 | FileCheck %s --check-prefix=ALL
// ALL: "-cc1"{{.*}} "-fptrauth-intrinsics" "-fptrauth-calls" "-fptrauth-returns" "-fptrauth-auth-traps" "-fptrauth-vtable-pointer-address-discrimination" "-fptrauth-vtable-pointer-type-discrimination" "-fptrauth-init-fini"
+// RUN: %clang -### -c --target=aarch64-elf -fno-ptrauth-elf-got -fptrauth-elf-got %s 2>&1 | FileCheck %s --check-prefix=ELFGOT
+// ELFGOT: "-cc1"{{.*}} "-fptrauth-elf-got"
+
+// RUN: %clang -### -c --target=aarch64-darwin -fptrauth-elf-got %s 2>&1 | FileCheck %s --check-prefix=NOELFGOT
+// NOELFGOT: warning: -fptrauth-elf-got works only for ELF; option ignored [-Woption-ignored]
+
// RUN: not %clang -### -c --target=x86_64 -fptrauth-intrinsics -fptrauth-calls -fptrauth-returns -fptrauth-auth-traps \
// RUN: -fptrauth-vtable-pointer-address-discrimination -fptrauth-vtable-pointer-type-discrimination \
-// RUN: -fptrauth-init-fini %s 2>&1 | FileCheck %s --check-prefix=ERR
+// RUN: -fptrauth-init-fini -fptrauth-elf-got %s 2>&1 | FileCheck %s --check-prefix=ERR
// ERR: error: unsupported option '-fptrauth-intrinsics' for target '{{.*}}'
// ERR-NEXT: error: unsupported option '-fptrauth-calls' for target '{{.*}}'
// ERR-NEXT: error: unsupported option '-fptrauth-returns' for target '{{.*}}'
@@ -23,3 +29,4 @@
// ERR-NEXT: error: unsupported option '-fptrauth-vtable-pointer-address-discrimination' for target '{{.*}}'
// ERR-NEXT: error: unsupported option '-fptrauth-vtable-pointer-type-discrimination' for target '{{.*}}'
// ERR-NEXT: error: unsupported option '-fptrauth-init-fini' for target '{{.*}}'
+// ERR-NEXT: error: unsupported option '-fptrauth-elf-got' for target '{{.*}}'
diff --git a/clang/test/Preprocessor/ptrauth_feature.c b/clang/test/Preprocessor/ptrauth_feature.c
index 80e239110ffc7..ba256384199fe 100644
--- a/clang/test/Preprocessor/ptrauth_feature.c
+++ b/clang/test/Preprocessor/ptrauth_feature.c
@@ -4,56 +4,72 @@
// RUN: -fptrauth-returns \
// RUN: -fptrauth-vtable-pointer-address-discrimination \
// RUN: -fptrauth-vtable-pointer-type-discrimination \
-// RUN: -fptrauth-init-fini | \
-// RUN: FileCheck %s --check-prefixes=INTRIN,CALLS,RETS,VPTR_ADDR_DISCR,VPTR_TYPE_DISCR,INITFINI
+// RUN: -fptrauth-init-fini \
+// RUN: -fptrauth-elf-got | \
+// RUN: FileCheck %s --check-prefixes=INTRIN,CALLS,RETS,VPTR_ADDR_DISCR,VPTR_TYPE_DISCR,INITFINI,ELFGOT
// RUN: %clang_cc1 -E %s -triple=aarch64 \
// RUN: -fptrauth-calls \
// RUN: -fptrauth-returns \
// RUN: -fptrauth-vtable-pointer-address-discrimination \
// RUN: -fptrauth-vtable-pointer-type-discrimination \
-// RUN: -fptrauth-init-fini | \
-// RUN: FileCheck %s --check-prefixes=NOINTRIN,CALLS,RETS,VPTR_ADDR_DISCR,VPTR_TYPE_DISCR,INITFINI
+// RUN: -fptrauth-init-fini \
+// RUN: -fptrauth-elf-got | \
+// RUN: FileCheck %s --check-prefixes=NOINTRIN,CALLS,RETS,VPTR_ADDR_DISCR,VPTR_TYPE_DISCR,INITFINI,ELFGOT
// RUN: %clang_cc1 -E %s -triple=aarch64 \
// RUN: -fptrauth-intrinsics \
// RUN: -fptrauth-returns \
// RUN: -fptrauth-vtable-pointer-address-discrimination \
// RUN: -fptrauth-vtable-pointer-type-discrimination \
-// RUN: -fptrauth-init-fini | \
-// RUN: FileCheck %s --check-prefixes=INTRIN,NOCALLS,RETS,VPTR_ADDR_DISCR,VPTR_TYPE_DISCR,INITFINI
+// RUN: -fptrauth-init-fini \
+// RUN: -fptrauth-elf-got | \
+// RUN: FileCheck %s --check-prefixes=INTRIN,NOCALLS,RETS,VPTR_ADDR_DISCR,VPTR_TYPE_DISCR,INITFINI,ELFGOT
// RUN: %clang_cc1 -E %s -triple=aarch64 \
// RUN: -fptrauth-intrinsics \
// RUN: -fptrauth-calls \
// RUN: -fptrauth-vtable-pointer-address-discrimination \
// RUN: -fptrauth-vtable-pointer-type-discrimination \
-// RUN: -fptrauth-init-fini | \
-// RUN: FileCheck %s --check-prefixes=INTRIN,CALLS,NORETS,VPTR_ADDR_DISCR,VPTR_TYPE_DISCR,INITFINI
+// RUN: -fptrauth-init-fini \
+// RUN: -fptrauth-elf-got | \
+// RUN: FileCheck %s --check-prefixes=INTRIN,CALLS,NORETS,VPTR_ADDR_DISCR,VPTR_TYPE_DISCR,INITFINI,ELFGOT
// RUN: %clang_cc1 -E %s -triple=aarch64 \
// RUN: -fptrauth-intrinsics \
// RUN: -fptrauth-calls \
// RUN: -fptrauth-returns \
// RUN: -fptrauth-vtable-pointer-type-discrimination \
-// RUN: -fptrauth-init-fini | \
-// RUN: FileCheck %s --check-prefixes=INTRIN,CALLS,RETS,NOVPTR_ADDR_DISCR,VPTR_TYPE_DISCR,INITFINI
+// RUN: -fptrauth-init-fini \
+// RUN: -fptrauth-elf-got | \
+// RUN: FileCheck %s --check-prefixes=INTRIN,CALLS,RETS,NOVPTR_ADDR_DISCR,VPTR_TYPE_DISCR,INITFINI,ELFGOT
// RUN: %clang_cc1 -E %s -triple=aarch64 \
// RUN: -fptrauth-intrinsics \
// RUN: -fptrauth-calls \
// RUN: -fptrauth-returns \
// RUN: -fptrauth-vtable-pointer-address-discrimination \
-// RUN: -fptrauth-init-fini | \
-// RUN: FileCheck %s --check-prefixes=INTRIN,CALLS,RETS,VPTR_ADDR_DISCR,NOVPTR_TYPE_DISCR,INITFINI
+// RUN: -fptrauth-init-fini \
+// RUN: -fptrauth-elf-got | \
+// RUN: FileCheck %s --check-prefixes=INTRIN,CALLS,RETS,VPTR_ADDR_DISCR,NOVPTR_TYPE_DISCR,INITFINI,ELFGOT
+
+// RUN: %clang_cc1 -E %s -triple=aarch64 \
+// RUN: -fptrauth-intrinsics \
+// RUN: -fptrauth-calls \
+// RUN: -fptrauth-returns \
+// RUN: -fptrauth-vtable-pointer-address-discrimination \
+// RUN: -fptrauth-vtable-pointer-type-discrimination \
+// RUN: -fptrauth-elf-got | \
+// RUN: FileCheck %s --check-prefixes=INTRIN,CALLS,RETS,VPTR_ADDR_DISCR,VPTR_TYPE_DISCR,NOINITFINI,ELFGOT
// RUN: %clang_cc1 -E %s -triple=aarch64 \
// RUN: -fptrauth-intrinsics \
// RUN: -fptrauth-calls \
// RUN: -fptrauth-returns \
// RUN: -fptrauth-vtable-pointer-address-discrimination \
-// RUN: -fptrauth-vtable-pointer-type-discrimination | \
-// RUN: FileCheck %s --check-prefixes=INTRIN,CALLS,RETS,VPTR_ADDR_DISCR,VPTR_TYPE_DISCR,NOINITFINI
+// RUN: -fptrauth-vtable-pointer-type-discrimination \
+// RUN: -fptrauth-init-fini | \
+// RUN: FileCheck %s --check-prefixes=INTRIN,CALLS,RETS,VPTR_ADDR_DISCR,VPTR_TYPE_DISCR,INITFINI,NOELFGOT
#if __has_feature(ptrauth_intrinsics)
// INTRIN: has_ptrauth_intrinsics
@@ -111,3 +127,11 @@ void has_ptrauth_init_fini() {}
// NOINITFINI: no_ptrauth_init_fini
void no_ptrauth_init_fini() {}
#endif
+
+#if __has_feature(ptrauth_elf_got)
+// ELFGOT: has_ptrauth_elf_got
+void has_ptrauth_elf_got() {}
+#else
+// NOELFGOT: no_ptrauth_elf_got
+void no_ptrauth_elf_got() {}
+#endif
|
|
Do we want a lot of |
I would lean towards a single flag. However, I do not know how this would affect Apple downstream and what are preferences there. Tagging @ahmedbougacha @ahatanak @ojhunt |
Comma-separated flags generally seem less user-friendly (for, e.g., grepping, modifying, reading). To some extent we can mitigate that with the obvious cleverness (e.g., negatives, repetition), at the cost of a lot more driver logic, and even that would only go so far. Is there a compelling argument for them?
Yeah, we do already support the long spellings, but we can change these here if we have consensus, and it should be doable to support the long spellings on top of these. I should mention that in our world we generally don't expect these to be common (other than in cc1 invocations), and they're generally used for overriding default ABI behavior inferred from triples and deployment targets and whatnot. |
We also don't expect usage of these options to be common - instead, we propose I think that the main rationale for using comma-separated flags instead of a bunch of different flags is that it'll reduce unneeded
Regarding that: I'm not sure if it's a good idea to support both |
|
@MaskRay @ahmedbougacha @asl I suggest to move the discussion on comma-separated flags/current flags to an issue #97320. This PR is intended to introduce the signed GOT flag, and we already have a bunch of similar ptrauth flags, so, if we want to change the flags to a single comma-separated value, we need to do that as a separate refactoring PR but not here. As for this PR, I suggest to stick with old convention not to make this a blocker. @MaskRay please let me know if you have other issues with proposed changes except suggestion to use comma-separated flags. |
|
Ping: would be glad to see feedback on the changes from those who are interested. |
1 similar comment
|
Ping: would be glad to see feedback on the changes from those who are interested. |
|
Ping: would be glad to see feedback on the changes from those who are interested. |
There was a problem hiding this comment.
Does this need a clang command line option at this point? Would it be possible to live with a {{-mllvm}} option to turn on signed GOT? I would expect signed GOT (or not) would be part of a higher-level signing schema and not toggled at a low-level via clang.
I know that there are existing command-line options for other signing schema affecting options, but I'm not sure if we want to add more to them.
@smithp35 Theoretically, we can use However, as you correctly mentioned, we already have other similar flags and they are also not intended to be used manually and, instead, should be a part of high-level signing schema. As for me, it's better to keep things consistent at each moment - if we use clang driver flags, we use them everywhere, and if we want to refactor that and move to So, if it's not a very huge concern right now, I'd prefer to introduce |
|
Sorry, I haven't spent a lot of time on the review and the user interface. -mllvm and -Xclang options are for internal testing and might change names or disappear in the future. Driver options offer more stability for users. Given the complexity of the feature and lack of upstream runtime linker (rtld) support, it might be wise to delay exposing it through driver options until it's more mature. What do you think? |
@MaskRay We actually have proof-of-concept support for musl access-softek/musl@cbf25fb (probably not ready for meaningful production usage though), and the signed GOT implementation which is submitted to upstream via multiple PRs (including this one) is actually working: I'm able to run binaries compiled and linked with signed GOT (and also So, as for me we can expose the flag right now. |
Yes, I know there is a musl fork, but it's not the official upstream. musl project tends to be cautious when accepting hardening features. Additionally, deployment experience might be limited. There are many examples we don't expose driver options:
|
kovdan01
force-pushed
the
pauth-signed-got-clang-driver
branch
from
aeedd6b to
1ca6cf8
Compare
@MaskRay I suppose that if exposing this to driver options is considered undesirable at the moment, we can just add cc1 option. A cc1 option is actually needed to construct pauth-enabled environments - currently, it is done by adding a predefined set of If we only add cc1 flag and postpone adding driver flag, would it be OK? |
|
@MaskRay Would be glad to see your answer on the question above:
|
|
Sorry, I made a mistake by approving #85235, which added a lot of driver options. It's not clear who are the initial ELF ptrauth users, but I am not sure with a very small user base, there are a lot of demands to customize the protection in such a fine-grained manner. Using cc1 options looks fine. |
Thanks!
I personally am OK with deleting those driver flags and only leaving cc1 flags - I'll prepare a corresponding PR for that. Also tagging @ahmedbougacha |
kovdan01
changed the title
kovdan01
force-pushed
the
pauth-signed-got-clang-driver
branch
2 times, most recently
from
a97866b to
3a5ae01
Compare
|
Rebased on top of current main with fixing conflicts. |
Add `-fptrauth-elf-got` clang cc1 flag and set `ptrauth_elf_got` preprocessor feature and `PointerAuthELFGOT` LangOption correspondingly. No additional checks like ensuring OS binary format is ELF are performed: it should be done on clang driver level when a pauth-enabled environment implying signed GOT enabled is requested. If the cc1 flag is passed, "ptrauth-elf-got" IR module flag is set.
kovdan01
force-pushed
the
pauth-signed-got-clang-driver
branch
from
3a5ae01 to
fd3919e
Compare
Add `-fptrauth-elf-got` clang cc1 flag and set `ptrauth_elf_got` preprocessor feature and `PointerAuthELFGOT` LangOption correspondingly. No additional checks like ensuring OS binary format is ELF are performed: it should be done on clang driver level when a pauth-enabled environment implying signed GOT enabled is requested. If the cc1 flag is passed, "ptrauth-elf-got" IR module flag is set.
Add
-fptrauth-elf-gotclang cc1 flag and setptrauth_elf_gotpreprocessor feature andPointerAuthELFGOTLangOption correspondingly. No additional checks like ensuring OS binary format is ELF are performed: it should be done on clang driver level when a pauth-enabled environment implying signed GOT enabled is requested.If the cc1 flag is passed, "ptrauth-elf-got" IR module flag is set.