akirilov-arm edited PR #3606 from pauth
to main
:
This pull request is meant to illustrate the RFC proposal to improve control flow integrity for compiled WebAssembly code by using the Pointer Authentication extension to the Arm instruction set architecture (bytecodealliance/rfcs#17), so it is not in a shape to be merged yet. The generation of the unwinding information is a hack to make things work (i.e. pass the tests) before there is a resolution to gimli-rs/gimli#130 and gimli-rs/gimli#608.
P.S. Actually I apply another hack to test the code - I change the processor feature detection logic in
cranelift/native/src/lib.rs
, so that the availability of PAuth is hardcoded (and there is no need to use a nightly toolchain). What you see here and what CI is testing is the clean code, though.
akirilov-arm edited PR #3606 from pauth
to main
:
This pull request is meant to illustrate the RFC proposal to improve control flow integrity for compiled WebAssembly code by using the Pointer Authentication extension to the Arm instruction set architecture (bytecodealliance/rfcs#17), so it is not in a shape to be merged yet. The generation of the unwinding information is a hack to make things work (i.e. pass the tests) before there is a resolution to gimli-rs/gimli#130 and gimli-rs/gimli#608.
P.S. Actually I apply another hack to test the code - I change the processor feature detection logic in
cranelift/native/src/lib.rs
, so that the availability of PAuth is hardcoded (and there is no need to use a nightly toolchain). What you see here and what CI is testing is the clean code, though.P.P.S. The RFC proposal has now been merged, and the changes in this PR have been updated the reflect the final version of the proposal, so they are now ready. No hacks are necessary (and have been removed from the code), but on Linux unwinding through functions with signed return addresses will result in crashes unless the unwinder includes the fix discussed in issue #3183 (note that return address signing is not enabled by default).
akirilov-arm requested bnjbvr for a review on PR #3606.
akirilov-arm requested cfallin for a review on PR #3606.
cfallin created PR review comment:
This feels a little too inefficient to me to put into the hotpath of codegen, even if it's used only in prologues/epilogues. I think the reason we pass around
Vec<settings::Flags>
rather than the backend-specific type is to avoid another type parameter, but I suspect we could do a lot better here with a little more thought. Could you look into whether we could (e.g.) add the ISA-specificFlags
type as a projected type onABIMachineSpec
and carry it that way, or maybe even instantiate anABIMachineSpec
with specific flags in the backend and then pass it to theABICallee
, and add&self
to all its methods? In other words, let's make flag resolution static if we can.
cfallin created PR review comment:
Together with the
unimplemented
above, is this going to create issues by default on macOS/aarch64? Should we avoid turning on PAuth on this platform until we implement that?
cfallin submitted PR review.
cfallin created PR review comment:
Comment here to note which instruction this is, since it's not the actual
ret
?
cfallin submitted PR review.
cfallin created PR review comment:
Could we add "as opposed to the default use of the A key" here, and maybe also a little bit about why one would want to do this ("some operating systems or ABIs may require it instead" or somesuch)?
akirilov-arm updated PR #3606 from pauth
to main
.
akirilov-arm submitted PR review.
akirilov-arm created PR review comment:
@cfallin My thinking was that since macOS hardcoded the use of the B key, the additional metadata specifying the key was unnecessary, hence this logic was safe. Now that we have our custom unwinder, this issue might be moot anyway, except perhaps for debugging/profiling purposes.
Also, note that back-edge CFI is disabled by default, since
sign_return_address
is the setting that makes the real change. Actually, I had a thought that since pointer authentication was always supported on Apple Silicon, we might treat that platform differently, i.e. by adding a line here:isa_builder.enable("sign_return_address").unwrap();
akirilov-arm requested cfallin for a review on PR #3606.
cfallin submitted PR review.
cfallin created PR review comment:
Ah, I had missed that it's still off by default. My real concern was just whether we were going to hit
unimplemented
bits immediately on macOS but it sounds like that's not the case.... and I just tested on my M1 machine and all tests pass, so this indeed is fine.
I'm personally fine with enabling it by default; @bnjbvr do you have any opinions on this, given you did the initial M1 porting work? Flipping the switch can happen in a followup PR if we decide to do so; we don't need to delay this PR I think.
cfallin submitted PR review.
cfallin merged PR #3606.
akirilov-arm submitted PR review.
akirilov-arm created PR review comment:
As a further clarification, since we are using the
HINT
encodings by default, it should actually be safe to enable the mechanism everywhere (modulo the unwinder issues discussed in #3183, which, as I said before, might be moot by now), but at least on Apple Silicon we have a guarantee that we are not filling up the processor pipeline with no-ops.
bnjbvr submitted PR review.
bnjbvr created PR review comment:
If this passes all tests with this setting, I don't see any downside in enabling it by default, and only benefits, so I'd be in favor of doing this. Perhaps we'd need some basic documentation explaining that "if you run into this particular kind of SIG* failure, this might be because of pointer authentication", and make sure it's still possible to disable it, would we run into a bug that wasn't caught by the test suite.
akirilov-arm submitted PR review.
akirilov-arm created PR review comment:
I can do a candidate PR for this that one of you can test - of course, it is going to pass the CI checks trivially.
akirilov-arm edited PR review comment.
Last updated: Dec 23 2024 at 12:05 UTC