Wasmtime GitHub notifications bot (Jun 23 2022 at 17:46): Wasmtime GitHub notifications bot (Jun 23 2022 at 17:46):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.
Wasmtime GitHub notifications bot (Jun 23 2022 at 17:52):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).
Wasmtime GitHub notifications bot (Jun 30 2022 at 11:10):akirilov-arm requested bnjbvr for a review on PR #3606.
Wasmtime GitHub notifications bot (Jun 30 2022 at 11:10):akirilov-arm requested cfallin for a review on PR #3606.
Wasmtime GitHub notifications bot (Jun 30 2022 at 18:13):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-specificFlagstype as a projected type onABIMachineSpecand carry it that way, or maybe even instantiate anABIMachineSpecwith specific flags in the backend and then pass it to theABICallee, and add&selfto all its methods? In other words, let's make flag resolution static if we can.
Wasmtime GitHub notifications bot (Jun 30 2022 at 18:13):cfallin created PR review comment:
Together with the
unimplementedabove, is this going to create issues by default on macOS/aarch64? Should we avoid turning on PAuth on this platform until we implement that?
Wasmtime GitHub notifications bot (Jun 30 2022 at 18:13):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jun 30 2022 at 18:13):cfallin created PR review comment:
Comment here to note which instruction this is, since it's not the actual
ret?
Wasmtime GitHub notifications bot (Jun 30 2022 at 18:13):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jun 30 2022 at 18:13):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)?
Wasmtime GitHub notifications bot (Aug 03 2022 at 17:24):akirilov-arm updated PR #3606 from pauth to main.
Wasmtime GitHub notifications bot (Aug 03 2022 at 17:38):akirilov-arm submitted PR review.
Wasmtime GitHub notifications bot (Aug 03 2022 at 17:38):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_addressis 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();
Wasmtime GitHub notifications bot (Aug 03 2022 at 17:39):akirilov-arm requested cfallin for a review on PR #3606.
Wasmtime GitHub notifications bot (Aug 03 2022 at 18:07):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 03 2022 at 18:07):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
unimplementedbits 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.
Wasmtime GitHub notifications bot (Aug 03 2022 at 18:08):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 03 2022 at 18:08):cfallin merged PR #3606.
Wasmtime GitHub notifications bot (Aug 03 2022 at 18:26):akirilov-arm submitted PR review.
Wasmtime GitHub notifications bot (Aug 03 2022 at 18:26):akirilov-arm created PR review comment:
As a further clarification, since we are using the
HINTencodings 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.
Wasmtime GitHub notifications bot (Aug 04 2022 at 14:32):bnjbvr submitted PR review.
Wasmtime GitHub notifications bot (Aug 04 2022 at 14:32):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.
Wasmtime GitHub notifications bot (Aug 04 2022 at 14:35):akirilov-arm submitted PR review.
Wasmtime GitHub notifications bot (Aug 04 2022 at 14:35):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.
Wasmtime GitHub notifications bot (Aug 04 2022 at 14:38):akirilov-arm edited PR review comment.
Last updated: Nov 22 2024 at 17:03 UTC