[WIP] Introduce aarch64-unknown-linux-pauthtest target#154759
[WIP] Introduce aarch64-unknown-linux-pauthtest target#154759jchlanda wants to merge 41 commits intorust-lang:mainfrom
Conversation
rustbot
added
A-compiletest
This comment has been minimized.
This comment has been minimized.
jchlanda
force-pushed
the
jakub/pauthtest
branch
from
88b623e to
3b3fcce
Compare
This comment has been minimized.
This comment has been minimized.
jchlanda
force-pushed
the
jakub/pauthtest
branch
from
3b3fcce to
e33dbf3
Compare
This comment has been minimized.
This comment has been minimized.
jchlanda
force-pushed
the
jakub/pauthtest
branch
from
e33dbf3 to
9e48aaa
Compare
This comment has been minimized.
This comment has been minimized.
jchlanda
force-pushed
the
jakub/pauthtest
branch
from
9e48aaa to
4468c36
Compare
This comment has been minimized.
This comment has been minimized.
jchlanda
force-pushed
the
jakub/pauthtest
branch
from
4468c36 to
c9fe7d6
Compare
This comment has been minimized.
This comment has been minimized.
jchlanda
force-pushed
the
jakub/pauthtest
branch
from
c9fe7d6 to
7ecdaa6
Compare
This comment has been minimized.
This comment has been minimized.
jchlanda
force-pushed
the
jakub/pauthtest
branch
from
7ecdaa6 to
65007e0
Compare
This comment has been minimized.
This comment has been minimized.
jchlanda
force-pushed
the
jakub/pauthtest
branch
from
65007e0 to
566b1b6
Compare
This comment has been minimized.
This comment has been minimized.
jchlanda
force-pushed
the
jakub/pauthtest
branch
3 times, most recently
from
0db30a1 to
5bc3e48
Compare
jchlanda
force-pushed
the
jakub/pauthtest
branch
from
63e45fd to
0606d27
Compare
This comment has been minimized.
This comment has been minimized.
| if (!C) | ||
| return Ptr; | ||
| if (!C->getType()->isPointerTy()) | ||
| return Ptr; | ||
| if (isa<UndefValue>(C) || isa<ConstantPointerNull>(C)) | ||
| return Ptr; |
There was a problem hiding this comment.
Do we expect values non-conforming to these conditions being passed to this function? Locally, I've commented out these lines, and nothing seems to be broken.
So, can we safely convert these to assertions? Or, maybe, some checks which would be present in release mode as well (and would panic when mismatch is detected)? Please just explain which contract do we have, who is responsible for these checks and whether the checks need to be just assertions or if we need to make them panicking or smth.
If there's a reason why we need to keep the current behavior, it's totally fine. But if so, can we somehow rename the function? Now it's name might make one think that we always wrap the underlying constant pointer value to ptrauth constant. But we also have this chunk of logic returning the exact input value w/o any change, and this is not clear from the function name.
There was a problem hiding this comment.
Fair.
With it now being wrapped in const_ptr_auth and only two call sites we can't violate the contract.
However it is still a symbol that can be accessed freely. I'm going to change it to asserts.
There was a problem hiding this comment.
Thanks! Just in case it was already intended to be changed to asserts - the checks are still present and nothing was changed. Are you about to submit this update or was there a reason why we want to keep the actual checks?
This comment has been minimized.
This comment has been minimized.
Looks like there is no bug in there, it's an expected behaviour. I'm guessing that when you compiled for You can verify that by either changing the arch to Will disable the test and add a comment. |
What I'm worried about is that in compiler/rustc_target/src/spec/targets/aarch64_unknown_linux_pauthtest.rs we have And my understanding was that if we add |
This comment has been minimized.
This comment has been minimized.
jchlanda
force-pushed
the
jakub/pauthtest
branch
from
6ee8cab to
9f2ac73
Compare
| in Rust can be found at | ||
| [#148640](https://github.com/rust-lang/rust/issues/148640). | ||
|
|
||
| Existing compiler options such as `-mbranch-protection` provide limited pointer |
There was a problem hiding this comment.
Is it -mbranch-protection for Rust? My understanding was that it's -Z branch-protection (while its indeed -mbranch-protection for clang). See also src/doc/unstable-book/src/compiler-flags/branch-protection.md.
Also, I'm not sure if it's worth talking about BTI - I doubt that anyone would mess BTI with pauthtest. But for pac-ret and pauthtest - it's non-obvious for new-comers because both these are based on the same PAC extension for aarch64 CPUs
| Clang-based toolchain. In this case, no wrapper script is required, | ||
| `<toolchain_root>/bin/aarch64-linux-pauthtest-clang` can be used directly. | ||
|
|
||
| ## Building the target |
There was a problem hiding this comment.
Removing the second one, both paragraphs should belong to the same header.
Is this update already present in this PR? Like, I'm still seeing the same header at lines 94 and 173.
| `aarch64-unknown-linux-pauthtest` target enabled. | ||
|
|
||
| For a comprehensive example of how to interact between C and Rust programs | ||
| withing the testing framework please consult |
There was a problem hiding this comment.
Typo: within
| * non-ABI-affecting indirect control flow hardening features included in | ||
| pauthtest ABI (corresponding to `-faarch64-jump-table-hardening`, | ||
| `-fptrauth-indirect-gotos`) | ||
| * signed ELF GOT entries (gated behind `-Z ptrauth-elf-got` off by default) |
There was a problem hiding this comment.
Nit: probably a comma missed before 'off'?
There was a problem hiding this comment.
@jchlanda Regarding test failures you've mentioned today - it looks like that for tests we need to manually specify additional compile args, otherwise stage1-tools-bin/compiletest uses the defaults.
Particularly, when running ./x test, it looks like we need to add --test-args --target-rustcflags --test-args "-Clink-arg=-Wl,--dynamic-linker=/path/to/aarch64-unknown-linux-pauthtest/usr/lib/libc.so".
This way of handling the issue is described in src/doc/rustc/src/platform-support/fuchsia.md
Thank you @kovdan01. This indeed helps with a group of tests, but, still when we are in subprocess scenarios I see failures. I'll DM you. |
|
|
||
| * `Cargo.toml` | ||
|
|
||
| ```markdown |
There was a problem hiding this comment.
Should it be toml instead of markdown?
| * `c_src/plugin.c` | ||
|
|
||
| ```c | ||
| #include <stdio.h> |
There was a problem hiding this comment.
Is this needed?
| ## Cross-compilation toolchains and C code | ||
|
|
||
| This target supports interoperability with C code. Use the PAC-enabled LLVM | ||
| sysroot, described in building the sysroot section of this document. C code must |
There was a problem hiding this comment.
described in building the sysroot section of this document
The section is now gone, could you please change wording here to reflect the current state of the document?
There was a problem hiding this comment.
yeah, changed to: building the toolchain
| // PAuth core info section of the resulting ELF, which the linker uses to enforce | ||
| // binary compatibility. | ||
| // | ||
| // We intentionally do not emit this flags now, since only a subset of pointer |
There was a problem hiding this comment.
Typo: this->these
| // binary compatibility. | ||
| // | ||
| // We intentionally do not emit this flags now, since only a subset of pointer | ||
| // authentication features is currently supported. By default, the absence of this |
There was a problem hiding this comment.
Maybe "subset of features included in pauthtest ABI" would be more clear?
| // Please note, that this would cause compatibility issues when linking against | ||
| // fully PAuth-enabled C/C++ binaries. | ||
| // | ||
| // Link to PAuth core info: |
There was a problem hiding this comment.
Nit: maybe "... info documentation:"? Or just "see also:"
I'm not a language expert, but I suppose this is not a link to the (platform, version) tuple (which is the definition of pauth core info) :)
| // authentication features is currently supported. By default, the absence of this | ||
| // info is treated as compatible with any binary. | ||
| // | ||
| // Please note, that this would cause compatibility issues when linking against |
There was a problem hiding this comment.
I suppose it's worth rephrasing this highlighting the following details:
- When talking about "compatibility issues", we are talking about runtime crashes due to auth failures while silently compiling and linking w/o problems
- I would not generalize this to "fully pauth-enabled c/c++ binaries". We are OK with interop when only pointers signed using supported ptrauth feature subset are crossing c/rust boundary (at this point, meaning only free function pointers signed w/o type discrimination). So we need to highlight that if pointers signed with other features cross c/rust or cxx/rust boundary, we result in runtime failures. Maybe even provide a list of such pointers: member function pointers, virtual function pointers, virtual table pointers, maybe smth else I forgot to mention (free function pointers with non-zero discr are technically not a part of pauthtest ABI, so not mentioning here)
There was a problem hiding this comment.
OK, tried to incorporate this.
There was a problem hiding this comment.
// (for example member function pointers, virtual function pointers, virtual table
// pointers).
Probably "for example, signing of C++ member ..."
| * the call is performed indirectly via a signed pointer, | ||
| * the `ptrauth` operand bundle enforces authentication at call time. | ||
|
|
||
| ## Cross-compilation |
There was a problem hiding this comment.
There are no other targets which define both "Cross-compilation" and "Cross-compilation toolchains and C code" sections. Probably we should stick to the TEMPLATE.md and only leave the latter section, moving contents from here if needed.
| must be consistent across Rust and C components. The target only supports | ||
| dynamic linking with the custom interpreter. | ||
|
|
||
| ## Limitation |
There was a problem hiding this comment.
Nit: probably an empty line after this would make things more consistent across the file
|
|
||
| * `src/main.rs` | ||
|
|
||
| ```rust,ignore (rustc will have no core crate for pauthtest) |
There was a problem hiding this comment.
What does this ignore (rustc will have no core crate for pauthtest) mean and which purpose does it serve?
|
|
||
| * `build.rs` | ||
|
|
||
| ```rust,no_run |
There was a problem hiding this comment.
What does no_run mean? I'm just not very familiar with this stuff, my understanding was it's just used for syntax highlighting
| ```toml | ||
| [package] | ||
| name = "rust_c_indirect" | ||
| version = "0.1.0" |
There was a problem hiding this comment.
As far as I understand from rust docs, version and edition are not mandatory. Could these be removed?
| must be compiled with the pauthtest aware compiler. Mixed Rust/C programs are | ||
| supported and tested (e.g. quicksort examples). Pointer authentication semantics | ||
| must be consistent across Rust and C components. The target only supports | ||
| dynamic linking with the custom interpreter. |
There was a problem hiding this comment.
I suppose that info about support only for dynamic linking should be somewhere in the overall target description, probably close to the top of the document - it's not cross-compilation specific, it's just a very important piece of info.
And I would also not say "custom interpreter" - it's not clear what does it mean. Maybe smth like "... dynamic linking with a pauthtest-enabled dynamic linker serving as ELF interpreter capable of resolving pauth relocations and respecting pauthtest ABI nuances" (maybe it could be somehow rephrased shorter, I'm just trying to make it clear which kind of "custom" dynamic loader we need)
| must be consistent across Rust and C components. The target only supports | ||
| dynamic linking with the custom interpreter. | ||
|
|
||
| ## Limitation |
There was a problem hiding this comment.
It's probably worth to make this plural, not singular, and add info about C++ interop and other currently unsupported pauthtest ABI features. It looks like that from the current version of the doc it's not very clear that these would be "by design" broken with runtime auth failures.
| named for convenience): | ||
|
|
||
| ```sh | ||
| x.py test --target aarch64-unknown-linux-pauthtest --force-rerun \ |
There was a problem hiding this comment.
Does it work this way or do we need to add --test-arg ... stuff I've mentioned previously in my comments to make rust test runner propagate correct dyn linker for rustc calls?
| * UI error reporting (pauthtest does not support `+crt-static`) | ||
| * crt-static-pauthtest.rs | ||
|
|
||
| All tests from `assembly-llvm`, `codegen-llvm`, `codegen-units`, `coverage`, |
There was a problem hiding this comment.
Is library (core, alloc, std) missed here?
| The following categories are supported (all present in tree): | ||
| * Assembly tests | ||
| * targets-aarch64_unknown_linux_pauthtest.rs | ||
| * LLVM IR/codegen tests |
There was a problem hiding this comment.
The extern-weak test looks missing in this list
| * UI error reporting (pauthtest does not support `+crt-static`) | ||
| * crt-static-pauthtest.rs | ||
|
|
||
| All tests from `assembly-llvm`, `codegen-llvm`, `codegen-units`, `coverage`, |
There was a problem hiding this comment.
Thanks for mentioning extra coverage here! BTW, it actually looks like that while some test suites like mir-opt, incremental, crashes indeed execute for the pauthtest target, some other ones are not (at least for me some suites show things like Testing stage1 with compiletest suite=XXX mode=XXX (x86_64-unknown-linux-gnu)
These are the suites which seem to only be tested against host (even though I'm running ./x test with explicit pauthtest target specified):
- coverage-run-rustdoc
- pretty
- rustdoc-html
- rustdoc-js
- rustdoc-js-std
- rustdoc-json
- rustdoc-ui
- ui-fulldeps
I might be running tests somehow incorrectly and maybe you actually observe them really running and passing for the pauthtest target. But if not and they only run for the host target - worth excluding from the list
| // is provided by libunwind. | ||
| #[cfg(target_env = "pauthtest")] | ||
| cfg_select! { | ||
| target_feature = "crt-static" => { |
| // and we tend to encounter interesting bugs in AArch64 unwinding code if we do not | ||
| frame_pointer: FramePointer::NonLeaf, | ||
| mcount: "\u{1}_mcount".into(), | ||
| ..base::linux::opts() |
There was a problem hiding this comment.
Resolved, thanks - looks reasonable, thanks for explanation.
| } | ||
|
|
||
| if cx.sess().target.env == Env::Pauthtest { | ||
| // FIXME(jchlanda): In LLVM/Clang, there also `aarch64-elf-pauthabi-platform` and |
| ## Cross-compilation toolchains and C code | ||
|
|
||
| This target supports interoperability with C code. Use the PAC-enabled LLVM | ||
| sysroot, described in building the sysroot section of this document. C code must |
| // PAuth core info section of the resulting ELF, which the linker uses to enforce | ||
| // binary compatibility. | ||
| // | ||
| // We intentionally do not emit this flags now, since only a subset of pointer |
| // binary compatibility. | ||
| // | ||
| // We intentionally do not emit this flags now, since only a subset of pointer | ||
| // authentication features is currently supported. By default, the absence of this |
| // Please note, that this would cause compatibility issues when linking against | ||
| // fully PAuth-enabled C/C++ binaries. | ||
| // | ||
| // Link to PAuth core info: |
| // authentication features is currently supported. By default, the absence of this | ||
| // info is treated as compatible with any binary. | ||
| // | ||
| // Please note, that this would cause compatibility issues when linking against |
There was a problem hiding this comment.
// (for example member function pointers, virtual function pointers, virtual table
// pointers).
Probably "for example, signing of C++ member ..."
|
|
||
| #[inline] | ||
| pub(crate) fn pauth_fn_attrs() -> &'static [&'static str] { | ||
| // FIXME(jchlanda) This is not an exhaustive list of all `pauthtest`-related attributes, but |
There was a problem hiding this comment.
| // FIXME(jchlanda) This is not an exhaustive list of all `pauthtest`-related attributes, but | |
| // FIXME(jchlanda) This is not an exhaustive list of all `ptrauth`-related attributes, but |
| let address_space = cx.tcx.global_alloc(prov.alloc_id()).address_space(cx); | ||
|
|
||
| llvals.push(cx.scalar_to_backend( | ||
| // For aarch64-unknown-linux-pauthtest function pointers stored in init/fini arrays need |
There was a problem hiding this comment.
I'd rather say something like "Under pointer authentication function pointers stored in init/fini arrays need special handling"
5 participants
View all comments
This PR introduces
aarch64-unknown-linux-pauthtesttarget. The target enablesPointer Authentication Code (PAC) support in Rust on AArch64 ELF based Linux
systems using a pauthtest ABI (provided by LLVM) and pauthtest-enabled sysroot
with custom musl, serving as a reference libc implementation.
Supported features include:
(corresponds to
-fptrauth-callsincluded in pauthtest ABI as defined inLLVM)
address after restoring from stack for non-leaf functions (corresponds to
-fptrauth-returns)(corresponds to
-fptrauth-auth-traps)ABI (corresponding to
-fptrauth-init-fini,-fptrauth-init-fini-address-discrimination)pauthtest ABI (corresponding to
-faarch64-jump-table-hardening,-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.
Please note that efforts were made to split the work into individual commits
that encapsulate different areas of the code; however, the commits are not
atomic and cannot be built or tested in isolation.
Useful links: