Stream: git-wasmtime

Topic: wasmtime / PR #3693 Initial forward-edge CFI implementation

view this post on Zulip Wasmtime GitHub notifications bot (May 04 2022 at 13:25):

akirilov-arm edited PR #3693 from bti to main:

This pull request is meant to illustrate the RFC proposal to improve control flow integrity for compiled WebAssembly code by using the Branch Target Identification extension to the Arm instruction set architecture (bytecodealliance/rfcs#17), so it is not ready to be merged yet.

view this post on Zulip Wasmtime GitHub notifications bot (May 04 2022 at 13:27):

akirilov-arm edited PR #3693 from bti to main:

This pull request is meant to illustrate the RFC proposal to improve control flow integrity for compiled WebAssembly code by using the Branch Target Identification extension to the Arm instruction set architecture (bytecodealliance/rfcs#17), so it is not ready to be merged yet.

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.

view this post on Zulip Wasmtime GitHub notifications bot (May 04 2022 at 17:30):

akirilov-arm edited PR #3693 from bti to main:

This pull request is meant to illustrate the RFC proposal to improve control flow integrity for compiled WebAssembly code by using the Branch Target Identification (BTI) extension to the Arm instruction set architecture (bytecodealliance/rfcs#17), so it is not ready to be merged yet.

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.

view this post on Zulip Wasmtime GitHub notifications bot (May 10 2022 at 11:53):

akirilov-arm updated PR #3693 from bti to main.

view this post on Zulip Wasmtime GitHub notifications bot (May 13 2022 at 15:38):

akirilov-arm updated PR #3693 from bti to main.

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2022 at 14:27):

akirilov-arm updated PR #3693 from bti to main.

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2022 at 18:20):

akirilov-arm updated PR #3693 from bti to main.

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2022 at 16:05):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2022 at 16:05):

bjorn3 created PR review comment:

The bti feature being detected doesn't necessarily mean that the kernel supports it, right? Maybe you could try using PROT_BTI and if that fails try again without?

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2022 at 16:07):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2022 at 16:07):

bjorn3 created PR review comment:

This is somewhat unfortunate code duplication.

view this post on Zulip Wasmtime GitHub notifications bot (May 30 2022 at 12:32):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 30 2022 at 12:32):

akirilov-arm created PR review comment:

No, that's exactly what it means. The reason is that the 64-bit Arm architecture does not provide any mechanism to unprivileged software, i.e. userspace, to detect the features supported by the processor while bypassing the kernel (or more privileged software); indeed, it does not provide such a mechanism at all (irrespective of the kernel involvement), leaving it to the software environment to define one.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 22 2022 at 11:32):

akirilov-arm updated PR #3693 from bti to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 22 2022 at 11:47):

akirilov-arm updated PR #3693 from bti to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 22 2022 at 13:08):

akirilov-arm updated PR #3693 from bti to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2022 at 09:39):

akirilov-arm requested alexcrichton for a review on PR #3693.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2022 at 09:39):

akirilov-arm requested cfallin for a review on PR #3693.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2022 at 14:49):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2022 at 14:49):

alexcrichton created PR review comment:

Could this use io::Error::last_os_error() as well to forward along the original error message?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2022 at 14:49):

alexcrichton created PR review comment:

Could this perhaps move to a method on Engine?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2022 at 14:49):

alexcrichton created PR review comment:

Instead of ferrying around this bool as a return value from this function could the is_branch_protection_enabled testing go as a method on Engine? Most contexts that are being given this bool already have the Engine so they should be able to query the engine itself for this

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2022 at 14:49):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2022 at 14:49):

alexcrichton created PR review comment:

Could this perhaps be unconditionally done? I would expect that the detection of the bti feature would happen at a higher level than this (e.g. at the Wasmtime Config layer) where enable_branch_protection is only true if the machine actually has bti.

Although I think it's ok to still have cfg!(target_arch = "aarch64") in the condition since this is still arm64-specific.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 12:20):

akirilov-arm updated PR #3693 from bti to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 13:49):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 13:49):

akirilov-arm created PR review comment:

Could this perhaps be unconditionally done? I would expect that the detection of the bti feature would happen at a higher level than this (e.g. at the Wasmtime Config layer) where enable_branch_protection is only true if the machine actually has bti.

My other comment provides a bit of context, but I didn't touch the higher-level layers on purpose, so that we support as much flexibility as possible, e.g. compile ahead-of-time a module with BTI on a machine that does not support BTI, or load a module that has been compiled ahead-of-time without BTI, while the engine is set to use BTI. I think that we will need to be more careful if we are making these decisions at a higher level - for instance, distinguish between wasmtime compile and wasmtime run.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 13:49):

akirilov-arm requested alexcrichton for a review on PR #3693.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 14:28):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 14:28):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 14:28):

alexcrichton created PR review comment:

Given that isa_flags always come from the Engine itself could this parameter be dropped and internally this uses self.compiler().isa_flags()?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 14:28):

alexcrichton created PR review comment:

Could this be dropped here since it can be inferred from the &Engine argument?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 14:28):

alexcrichton created PR review comment:

Could this perhaps still fall through to rustix below but use MprotectFlags::from_u32 until there's a constant for PROT_BTI?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 14:28):

alexcrichton created PR review comment:

Since this an be inferred from &Engine-the-argument could this return value be dropped?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 14:28):

alexcrichton created PR review comment:

Also if it's a perf concern it might be prudent to add engine.compiler().is_branch_protection_enabled(..) since isa_flags() returns a whole new BTreeMap each time it's called I think. Or otherwise watch it get enabled on Config and ferry through that bool to here or similar.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 14:36):

akirilov-arm updated PR #3693 from bti to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 19:14):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 19:14):

akirilov-arm created PR review comment:

There is one case in which the flags come from a previously compiled module - line 230 in crates/wasmtime/src/module/serialization.rs. However, I can create a wrapper function, say is_branch_protection_enabled_globally(), that simply calls this one with a fixed parameter, &engine.compiler().isa_flags(), and change most call sites to invoke it instead.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 19:17):

akirilov-arm created PR review comment:

See my comment about line 540 in crates/wasmtime/src/engine.rs.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 19:17):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 19:31):

akirilov-arm created PR review comment:

This is a bit tricky, but the reason is the cache implementation on line 291, and in particular the call to SerializedModule::into_parts() on line 314, which, given the declaration of ModuleCacheEntry::get_data_raw(), enforces a particular function signature for Module::build_artifacts(). I tried changing the closure on line 311, so that it didn't return whatever SerializedModule::into_parts()'s return value was directly by defining a mutable flag enable_branch_protection inside Module::from_binary(), but then the Rust compiler complained that it could not accept a closure that captured variables in that location.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 19:31):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 19 2022 at 11:48):

akirilov-arm updated PR #3693 from bti to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 19 2022 at 12:06):

akirilov-arm updated PR #3693 from bti to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 19 2022 at 12:24):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 19 2022 at 12:24):

akirilov-arm created PR review comment:

@alexcrichton I am not too happy about this part, but I tried separating the parsing of the ELF image and the changing of the memory protection done by CodeMemory::publish() and I ran into various borrow checker and lifetime issues that seemed beyond my Rust skills, so I went ahead with this suboptimal, but nevertheless correct solution.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 19 2022 at 19:30):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 19 2022 at 19:30):

alexcrichton created PR review comment:

Oh dear, I'll work on improving this with some refactorings but otherwise this PR looks good to me, thanks for the updates!

view this post on Zulip Wasmtime GitHub notifications bot (Aug 19 2022 at 19:30):

alexcrichton edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 19 2022 at 19:44):

alexcrichton created PR review comment:

Ah I ended up with https://github.com/akirilov-arm/wasmtime/pull/1 which was easier than I thought

view this post on Zulip Wasmtime GitHub notifications bot (Aug 19 2022 at 19:44):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2022 at 11:38):

akirilov-arm updated PR #3693 from bti to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2022 at 11:53):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2022 at 11:53):

akirilov-arm created PR review comment:

Yeah, parsing the ELF header twice definitely helps.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2022 at 12:01):

akirilov-arm updated PR #3693 from bti to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2022 at 12:02):

akirilov-arm updated PR #3693 from bti to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2022 at 12:05):

akirilov-arm edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2022 at 21:05):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2022 at 21:05):

cfallin created PR review comment:

Rather than take a single bool here, can we take an options struct? It may seem overkill for one flag but enable_branch_protection would not be my first guess for the meaning of a bare false hanging out in a Memory::new call :-)

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2022 at 21:05):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2022 at 21:05):

cfallin created PR review comment:

This logic feels a little bit brittle to me. If I understand correctly, it is determining what the "real" CLIF-level block for this target is meant to be, even if we have inserted an edge block, so that we can tell if any indirect branch might reach it; is that right?

If so, I think what we really should have is a set of lowered block indices reachable via an indirect branch, computed by the lowering-order code. We can compute this when we traverse the lowered-block's successors if we know that we're visiting out-edges of an indirect branch, I think.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2022 at 21:05):

cfallin created PR review comment:

Can we add a comment here noting what the bool arg to the closure is for?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 07 2022 at 09:17):

akirilov-arm updated PR #3693 from bti to main.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 07 2022 at 09:25):

akirilov-arm submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 07 2022 at 09:25):

akirilov-arm created PR review comment:

I decided to define a new enum and pass it instead. I noticed that Windows defined some memory protection flags that were related to Control Flow Guard, its own CFI implementation, and this approach seems like a good way to leave the possibility of adding support for them in the future (if we decide to do so), while keeping the current implementation simple.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 07 2022 at 10:01):

akirilov-arm updated PR #3693 from bti to main.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 08 2022 at 00:07):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 08 2022 at 14:36):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 08 2022 at 14:36):

alexcrichton merged PR #3693.

Last updated: Oct 23 2024 at 20:03 UTC