Introduce aarch64-unknown-linux-pauthtest target#155722
Introduce aarch64-unknown-linux-pauthtest target#155722jchlanda wants to merge 9 commits intorust-lang:mainfrom
Conversation
Co-authored-by: Daniil Kovalev <dkovalev@accesssoftek.com>
`const_ptr_auth` wraps aroudn `LLVMRustConstPtrAuth`, which provides a way to decorate a function pointer in `ConstPtrAuth`.
Allow PAC metadata to be passed to `get_fn_addr` and related API changes.
The set of supported attributes is: function * "aarch64-jump-table-hardening" * "ptrauth-auth-traps" * "ptrauth-calls" * "ptrauth-indirect-gotos" * "ptrauth-returns" module * "ptrauth-elf-got" * "ptrauth-sign-personality"
Also add flag for ELF-GOT signing.
Also: * update tests to force dynamic library when targetting pauthtest * various test fixes * introduce end-to-end tests for pauthtest (in run-make)
| } | ||
| } | ||
|
|
||
| if sess.target.env == Env::Pauthtest { |
There was a problem hiding this comment.
For a full context I'm going to bring comments from a draft PR into this one:
@asl
Note that pauthtest in clang is an interim thing. How we can enable pauth on, say, bare-metal platform? Or on some downstream platform?
@jchlanda
While there is already code gated by Env in codegen llvm, pauthtest is an outlier, such that all its functionality is behind the environment checks. Handling it earlier in the pipeline would make for a better design, one that decouples target/triple specifics from pauth logic. Maybe Session would be a good candidate to hold that info?
Clang does just that by solving platform/environment specifics on the driver level and later on basing the logic on language flags that are there regardless of the platform. Finally the flags are resolved to a concrete signing schema.
Note that
pauthtestin clang is an interim thing. How we can enable pauth on, say, bare-metal platform? Or on some downstream platform?
Specifically for this bit, I suggested on libc that env could be set to musl since that's what most libraries will care about, and then use the TargetOptions.cfg_abi field for pauthtest. Which means that you can cfg(target_abi = "pauthtest") regardless of whether it's musl or bare metal.
Either way, it could also be encapsulated in an .should_emit_pauth() -> bool function that can do the relevant checks in one place.
There was a problem hiding this comment.
I've created a follow up task to unify handling of pointer authentication features.
There was a problem hiding this comment.
For a full context I'm going to bring comments from a draft PR into this one:
@tgross35
Mentioned this elsewhere but the target should land as no-std first. The initial support should only be the bare minimum to get core building via --target, everything else can come as a followup.
(Fine to keep this PR around as a WIP of course)
There was a problem hiding this comment.
Landing as no-std first makes sense - smaller incremental step are always welcome :)
There was a problem hiding this comment.
For a full context I'm going to bring comments from a draft PR into this one:
@tgross35
Could pauth-quicksort-*-driver be combined into one run-make test that builds/runs two different things in main? Since they can probably share some docs or reuse some code, and it saves the per-test overhead.
There was a problem hiding this comment.
@tgross35, not without some serious code juggling; conceptually those two tests perform identical tasks. However, the problem is that pauth-quicksort-c-driver creates a C executable that links against a Rust library. While pauth-quicksort-rust-driver does the opposite, creating a Rust executable that is linked against a C library.
I could put all the sources in one directory and handle compilations from one build script, but am not sure if that would make for a clear, easy to follow test?
|
r? compiler |
|
cc: @davidtwco |
| arch: Arch::AArch64, | ||
|
|
||
| options: TargetOptions { | ||
| env: Env::Pauthtest, |
There was a problem hiding this comment.
I tend to think this should set target_abi = "pauth" instead of target_env = "pauth"? Or maybe an entirely new cfg(target_has_pointer_authentication)?
My motivation is that arm64e Apple targets have pointer authentication as well, but there target_env is used for things like Mac Catalyst too (target_env = "macabi").
There was a problem hiding this comment.
Target triple, aarch64-unknown-linux-pauthtest, mimics exactly what LLVM defines. With pauthtest being llvm::Triple::EnvironmentType.
What I don't like about explicitly using target_abi is that it might set an incorrect expectation. While it is true, that different authentication features, encoded in the signing schema, are not ABI-compatible with one another, pauthtest is not a new ABI, it follows C/C++ language ABI, with pointer authentication features. Also, while LLVM allows for -mabi=pauthtest (i.e.: calng --target=aarch64-linux -mabi=pauthtest) it is then normalized to environment part of the triple effective re-creating aarch64-unknown-linux-pauthtest.
WRT other targets supporting pointer authentication features. I have a follow up task to abstract that away. Clang handles it gracefully, where platform/environment is normalized to a set of language features, that are then used to create a concrete signing schema.
There was a problem hiding this comment.
What I don't like about explicitly using
target_abiis that it might set an incorrect expectation. While it is true, that different authentication features, encoded in the signing schema, are not ABI-compatible with one another,pauthtestis not a new ABI, it follows C/C++ language ABI, with pointer authentication features.
That still feels like it can fit under the definition of "new ABI" IMO? At least if you take the view "if it's not compatible, it's a new ABI".
Anyhow, my primary argument here comes after reading the libc PR, it feels like being able to set target_env = "musl" would make things a lot simpler in most user code?
EDIT: I see now that you've also discussed that with @tgross35.
(If we do that, I'd probably lean towards this target being called aarch64-unknown-linux-muslpauth and it setting target_env = "musl" and target_abi = "pauth", because that would make cc-rs' parsing easier too).
There was a problem hiding this comment.
Currently musl is just an implementation detail and we do not want to set this in the stone. It is just a reference proof-of-concept libc implementation that was done on top of musl in the downstream patch, however, this does not imply that:
- Upstream musl will support pauth anytime soon (and frankly speaking, the real-world implementation should be done a bit differently to ensure e.g. there are no exploitable signing oracles)
- There might be other standard libraries supporting pauth (I am thinking about bare-metal world here mostly)
- Still, some requirements would likely hold for any standard library implementations (e.g. while static link is in theory possible, in reality it would require lots of weird things especially when address discrimination is involved).
To add more on top of it – we're currently discussing ways how we can modify pauthtest approach in Clang/LLVM. So the target triple with environment is an interim solution (this was the motivation of test in the name) that will go away but for now we'd need it in some form :)
There was a problem hiding this comment.
My gut instincts with regards to the target name match @madsmtm's. It's relatively easy for us to change, remove or add new targets with different triples for other standard libraries or environments, so we can make this target as descriptive as possible for the environment is corresponds to.
There was a problem hiding this comment.
From a libs perspective I'd rather have target_env = "musl" as well if that tells us the API we are allowed to use. Otherwise this target is always going to be playing catch up when something gets gated on target_env = "musl" but doesn't account for pauthtest. I also don't know that we need to mirror LLVM if we could do something that's an improvement. (It's fine IMO to not say "musl" in the target triple but still specify that in the env, if that's one of the concerns.)
Perhaps worth a Zulip thread?
|
|
||
| pub(crate) fn target() -> Target { | ||
| Target { | ||
| llvm_target: "aarch64-unknown-linux-pauthtest".into(), |
There was a problem hiding this comment.
I feel like the motivation for having "test" in the name is somewhat weak? If the intention is that the target is unstable, we have better ways of enforcing that (such as a check that you're on the nightly channel before using the target).
There was a problem hiding this comment.
pauthtest mimics what LLVM uses: llvm::Triple::EnvironmentType.
There was a problem hiding this comment.
Yeah, I understand that, but I'm not convinced we should carry that forwards? Do you have a link to the original motivation for that name in LLVM?
I guess it depends on how widely you expect to see usage of this target? If there's any point where libraries are going to do target_env = "pauthtest", where it would need to be updated to target_env = "pauth" or smth in the future, then I think it's a bad idea?
There was a problem hiding this comment.
There is an ongoing discussion as to how to move forward with the naming on the LLVM side (see the round table report). My preference would be to stick with pauthtest and wait for LLVM to come up with their new name, which I'll be happy to adopt here.
| #[cfg_attr(target_env = "pauthtest", link(name = "rust_test_helpers", kind = "dylib"))] | ||
| #[cfg_attr(not(target_env = "pauthtest"), link(name = "rust_test_helpers", kind = "static"))] |
There was a problem hiding this comment.
Dynamic linking is required, with a dynamic linker acting as the ELF interpreter that can resolve pauth relocations and enforce pointer authentication constraints.
Maybe we could translate link(name = "xyz", kind = "static") to kind = "dynamic" on this target then in the meantime? It seems like the test suite is gonna break all the time otherwise when people assume that static linking works.
|
r? madsmtm |
|
|
|
Some changes occurred in compiler/rustc_codegen_gcc This PR modifies cc @jieyouxu This PR modifies If appropriate, please update These commits modify compiler targets.
Some changes occurred in src/tools/compiletest cc @jieyouxu Some changes occurred in cc @Amanieu, @folkertdev, @sayantn Some changes occurred in src/doc/rustc/src/platform-support cc @Noratrieb |
| valid_range: WrappingRange::full(pointer_size), | ||
| }, | ||
| cx.type_ptr_ext(address_space), | ||
| pac_metadata, |
There was a problem hiding this comment.
| pac_metadata, | |
| Some(pac_metadata), |
and just have let pac_metadata not be wrapped in an option?
There was a problem hiding this comment.
I'm not sure, ideally we should only be signing function pointers (addresses, not values), so having the metadata wrapped works as a binary switch. It is true that now, for codegen llvm, we sign everything, but that is a workaround (more info here: #152532).
| // Pauthtest musl does not support 128-bit floating point math. | ||
| (Arch::AArch64, _) if *target_env == Env::Pauthtest => false, |
There was a problem hiding this comment.
Is there an issue for this? What's the problem here?
There was a problem hiding this comment.
This refers to the fork of musl that is used for this target, as it is a proof of concept implementation it has not yet been added.
| /// Strategy for incorporating address-based diversity into PAC computation. | ||
| pub enum AddressDiversity { | ||
| /// No address diversity is applied. | ||
| None, |
There was a problem hiding this comment.
| /// Strategy for incorporating address-based diversity into PAC computation. | |
| pub enum AddressDiversity { | |
| /// No address diversity is applied. | |
| None, | |
| /// Strategy for incorporating address-based diversity into PAC computation. | |
| #[derive(Default)] | |
| pub enum AddressDiversity { | |
| /// No address diversity is applied. | |
| #[default] | |
| None, |
Unless I'm missing something the default instances can just be derived?
| pub(crate) fn LLVMRustConstPtrAuth( | ||
| ptr: *mut Value, | ||
| key: u32, | ||
| disc: u64, | ||
| addr_diversity: *mut Value, |
There was a problem hiding this comment.
Are these actually *mut Value or in reality *const Value? Whether to use const or mut in extern signatures is really more for documentation, using the correct pointer type provides no safety and they can be cast back and forth arbitrarily.
You do cast from *const to *mut when calling this function below, so either that is UB (if the value is in fact mutated) or this signature should just use *const
There was a problem hiding this comment.
You are right, const * it is. Changed now.
|
|
||
| fn get_fn_addr(&self, instance: Instance<'tcx>) -> &'ll Value { | ||
| get_fn(self, instance) | ||
| fn get_fn_addr(&self, instance: Instance<'tcx>, pac: Option<PacMetadata>) -> &'ll Value { |
There was a problem hiding this comment.
This function should document the difference between using None versus Some(PacMetadata::default()). When is None allowed, versus when should the default be used?
| #[inline(never)] | ||
| pub extern "C" fn add(a: i32, b: i32) -> i32 { | ||
| a + b | ||
| } |
There was a problem hiding this comment.
if you adjust this slightly you can use minicore here too.
maybe something like
fn select(a: bool, b: i32, c: i32) -> i32 {
if a { b } else { c }
}There was a problem hiding this comment.
I've modified it to use minicore, we have to keep the extern "C" function though, only those functions take part in pointer authentication.
| } | ||
| } | ||
|
|
||
| // CHECK-LABE: test_direct_call |
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes, it should be CHECK-LABEL. Fixed.
| // Make sure that direct extern "C" calls are not handled by pointer authentication operand bundle | ||
| // logic. | ||
| use std::ffi::c_void; | ||
| use std::hint::black_box; |
There was a problem hiding this comment.
This is so close to also being a minicore test. black_box can just be added to it (much like ptr::write_volatile).
There was a problem hiding this comment.
can use minicore
View all comments
This target enables Pointer Authentication Code (PAC) support in Rust on AArch64
ELF-based Linux systems. It uses the
aarch64-unknown-linux-pauthtestLLVMtarget and a pointer-authentication-enabled sysroot with a custom musl as a
reference libc implementation. Dynamic linking is required, with a dynamic
linker acting as the ELF interpreter that can resolve pauth relocations and
enforce pointer authentication constraints.
Supported features include:
to LLVM's
-fptrauth-calls)after restoring for non-leaf functions (corresponds to
-fptrauth-returns)(corresponds to
-fptrauth-auth-traps)authentication scheme (corresponds to
-fptrauth-init-finiand-fptrauth-init-fini-address-discrimination)LLVM (corresponds to
-faarch64-jump-table-hardeningand-fptrauth-indirect-gotos)-Z ptrauth-elf-got, off by default)Existing compiler support, such as enabling branch authentication instructions
(i.e.:
-Z branch-protection) provide limited functionality, mainly signingreturn addresses (
pac-ret). The new target goes further by enabling ABI-levelpointer authentication support.
This target does not define a new ABI; it builds on the existing C/C++ language
ABI with pointer authentication support added. However, different authentication
features, encoded in the signing schema, are not ABI-compatible with one
another.
Useful links:
https://clang.llvm.org/docs/PointerAuthentication.html
https://llvm.org/docs/PointerAuth.html
https://github.com/ARM-software/abi-aa/blob/main/pauthabielf64/pauthabielf64.rst
Tier 3 check list
I pledge to do my best maintaining it.
The name chosen for the target is
aarch64-unknown-linux-pauthtestwhichmirrors the LLVM target naming.
There should be no confusion, the name follows naming convention and is
descriptive.
Letters, numbers and dashes only.
The target requires system
clangandlldavailable as well as custom libc(musl based) and sysroot, provided through the build scripts.
There are no license implications.
Understood.
There are no new dependencies or requirements.
The target only relies on open source tools.
No such terms present.
Understood.
Understood.
aarch64-unknown-linux-pauthtest targethas std library support, moreover alllibrarytests pass for the target.Platform support document covers building instructions.
Understood.
Understood.
Understood.
Understood.
It is expected that the target should be able to compile binaries on any systems
that are capable of compiling
aarch64code.