Initial forward-edge CFI implementation #3693

akirilov-arm · 2022-01-14T21:28:21Z

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.

github-actions · 2022-03-07T16:32:57Z

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "cranelift:meta", "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

github-actions · 2022-05-04T13:56:51Z

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "fuzzing"

Thus the following users have been cc'd because of the following labels:

fitzgen: fuzzing

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

akirilov-arm · 2022-05-04T17:37:22Z

Note that the region crate lacks support for BTI (reported in darfink/region-rs#21), so I have used a work-around - a direct call to libc::mprotect().

cranelift/jit/src/memory.rs

akirilov-arm · 2022-06-23T09:40:20Z

@alexcrichton I added you as reviewer for the Wasmtime bits.

alexcrichton

Seems reasonable enough to me! Sorry you've probably answered these questions before but I wanted to double-check something as well:

If a module is compiled with bti support and loaded into an engine that has bti support disabled, that's fine right? (since everything is a nop)
If a module is not compiled with bti support, but is loaded into an engine with bti support, that's bad right? (in that none of the branches are protected but the page permissions say that all branches need protection?)

Otherwise though I think this can cut down on the amount of is_branch_proetection_enabled calls and passing-of-the-bool by just making it a method on Engine to avoid some extra changes here.

crates/runtime/src/mmap.rs

crates/wasmtime/src/lib.rs

crates/wasmtime/src/module.rs

akirilov-arm · 2022-08-16T13:25:31Z

@alexcrichton I wanted to finish with the pointer authentication PR before coming back to this one:

* If a module is compiled with bti support and loaded into an engine that has bti support disabled, that's fine right? (since everything is a nop)

Yes, that is correct; we just have to make sure that we do not request the PROT_BTI memory protection mode from the kernel, since the system call is going to fail.

* If a module is not compiled with bti support, but is loaded into an engine with bti support, that's bad right? (in that none of the branches are protected but the page permissions say that all branches need protection?)

Yes, assuming that the engine applies PROT_BTI unconditionally to all executable memory pages.

The current implementation supports the full flexibility the BTI extension enables, so we have to decouple the following:

whether the user passed --cranelift-enable use_bti on the Wasmtime command line
whether the environment (i.e. CPU and OS) supports BTI
whether the module that the engine is trying to load has been compiled to use BTI instructions; also, if I am not mistaken, the engine could be trying to load several modules, which might not necessarily have been compiled with the same settings

afonso360 · 2022-08-16T14:25:03Z

We should add a target aarch64 use_bti to runtests/br_table.clif, we now use the cranelift-jit to run runtests so we can also test the cranelift-jit portion of this PR.

We don't yet support call's, but that's something I'm addressing in #4667, and can add some bti runtests there as well.

alexcrichton

Just a few minor nits but otherwise the all the wasmtime bits look reasonable to me

crates/runtime/src/mmap.rs

crates/wasmtime/src/engine.rs

crates/wasmtime/src/module.rs

alexcrichton · 2022-08-16T20:23:06Z

Ah sorry ok I see why this can't be an Engine-level thing. If that's the case though instead of ferrying a separate bool through the system could this get attached to CompiledModuleInfo somewhere internally?

Give the user the option to start all basic blocks that are targets of indirect branches with the BTI instruction introduced by the Branch Target Identification extension to the Arm instruction set architecture. Copyright (c) 2022, Arm Limited.

crates/jit/src/instantiate.rs

This involves "parsing" twice but this is parsing just the header of an ELF file so it's not a very intensive operation and should be ok to do twice.

akirilov-arm · 2022-08-22T12:08:01Z

@cfallin I think that I have resolved all of @alexcrichton's comments that are related to Wasmtime, so do you have any feedback on the Cranelift side of things?

cfallin

Thanks @akirilov-arm and sorry for the delay in giving the Cranelift parts a review!

Overall I think this is quite reasonable. I do have one suggestion for a simplification in the way that the locations of indirect-target blocks are determined (and I apologize for being extra-picky with this bit of code but the CFG lowering is a very critical and historically difficult-to-get-right piece of the backend!). With that modified, or at least with the suggestion explored, then LGTM.

cranelift/codegen/src/inst_predicates.rs

cranelift/codegen/src/machinst/vcode.rs

cranelift/jit/src/memory.rs

cfallin

LGTM for the Cranelift bits -- thanks a bunch for the patience!

cc @alexcrichton to double-check the Wasmtime-related changes?

akirilov-arm force-pushed the bti branch from 9a5aaf2 to 8aa19a8 Compare March 7, 2022 16:08

github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Mar 7, 2022

akirilov-arm mentioned this pull request Mar 8, 2022

Compiled Module loading should check isa flags against host cpu #3897

Closed

akirilov-arm force-pushed the bti branch from 8aa19a8 to b1b3ad8 Compare May 4, 2022 13:25

akirilov-arm changed the title ~~[RFC] Initial forward-edge CFI implementation~~ Initial forward-edge CFI implementation May 4, 2022

github-actions bot added the fuzzing Issues related to our fuzzing infrastructure label May 4, 2022

akirilov-arm force-pushed the bti branch 2 times, most recently from f5f2375 to 43c50c2 Compare May 13, 2022 15:38

akirilov-arm force-pushed the bti branch 2 times, most recently from 0ae4b62 to b45cc6d Compare May 26, 2022 18:20

bjorn3 reviewed May 27, 2022

View reviewed changes

cranelift/jit/src/memory.rs Outdated Show resolved Hide resolved

bjorn3 reviewed May 27, 2022

View reviewed changes

cranelift/jit/src/memory.rs Outdated Show resolved Hide resolved

akirilov-arm force-pushed the bti branch 3 times, most recently from cfe59d0 to 0621025 Compare June 22, 2022 13:08

akirilov-arm requested review from alexcrichton and cfallin June 23, 2022 09:39

alexcrichton reviewed Jun 23, 2022

View reviewed changes

crates/runtime/src/mmap.rs Outdated Show resolved Hide resolved

crates/runtime/src/mmap.rs Outdated Show resolved Hide resolved

crates/wasmtime/src/lib.rs Outdated Show resolved Hide resolved

crates/wasmtime/src/module.rs Outdated Show resolved Hide resolved

akirilov-arm force-pushed the bti branch from 0621025 to f3bacaa Compare August 16, 2022 12:20

akirilov-arm requested a review from alexcrichton August 16, 2022 13:49

alexcrichton approved these changes Aug 16, 2022

View reviewed changes

crates/runtime/src/mmap.rs Outdated Show resolved Hide resolved

crates/wasmtime/src/engine.rs Outdated Show resolved Hide resolved

crates/wasmtime/src/module.rs Outdated Show resolved Hide resolved

crates/wasmtime/src/module.rs Outdated Show resolved Hide resolved

akirilov-arm force-pushed the bti branch from f3bacaa to 2d87142 Compare August 16, 2022 14:36

Initial forward-edge CFI implementation

fb71d22

Give the user the option to start all basic blocks that are targets of indirect branches with the BTI instruction introduced by the Branch Target Identification extension to the Arm instruction set architecture. Copyright (c) 2022, Arm Limited.

akirilov-arm force-pushed the bti branch from 2d87142 to fb71d22 Compare August 19, 2022 11:48

Merge branch 'main' into bti

d7bfc26

akirilov-arm commented Aug 19, 2022

View reviewed changes

crates/jit/src/instantiate.rs Outdated Show resolved Hide resolved

akirilov-arm force-pushed the bti branch 2 times, most recently from 2595dec to d7bfc26 Compare August 22, 2022 12:01

Refactor from_artifacts to avoid second make_executable (#1)

38f92d5

This involves "parsing" twice but this is parsing just the header of an ELF file so it's not a very intensive operation and should be ok to do twice.

cfallin reviewed Aug 22, 2022

View reviewed changes

cranelift/codegen/src/inst_predicates.rs Show resolved Hide resolved

cranelift/codegen/src/machinst/vcode.rs Outdated Show resolved Hide resolved

cranelift/jit/src/memory.rs Outdated Show resolved Hide resolved

akirilov-arm added 2 commits September 7, 2022 09:16

cfallin approved these changes Sep 8, 2022

View reviewed changes

alexcrichton approved these changes Sep 8, 2022

View reviewed changes

alexcrichton merged commit d8b2908 into bytecodealliance:main Sep 8, 2022

akirilov-arm deleted the bti branch September 8, 2022 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial forward-edge CFI implementation #3693

Initial forward-edge CFI implementation #3693

akirilov-arm commented Jan 14, 2022 •

edited

Loading

github-actions bot commented Mar 7, 2022

github-actions bot commented May 4, 2022

akirilov-arm commented May 4, 2022

akirilov-arm commented Jun 23, 2022

alexcrichton left a comment

akirilov-arm commented Aug 16, 2022

afonso360 commented Aug 16, 2022 •

edited

Loading

alexcrichton left a comment

alexcrichton commented Aug 16, 2022

akirilov-arm commented Aug 22, 2022

cfallin left a comment

cfallin left a comment

Initial forward-edge CFI implementation #3693

Initial forward-edge CFI implementation #3693

Conversation

akirilov-arm commented Jan 14, 2022 • edited Loading

github-actions bot commented Mar 7, 2022

Subscribe to Label Action

github-actions bot commented May 4, 2022

Subscribe to Label Action

akirilov-arm commented May 4, 2022

akirilov-arm commented Jun 23, 2022

alexcrichton left a comment

Choose a reason for hiding this comment

akirilov-arm commented Aug 16, 2022

afonso360 commented Aug 16, 2022 • edited Loading

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton commented Aug 16, 2022

akirilov-arm commented Aug 22, 2022

cfallin left a comment

Choose a reason for hiding this comment

cfallin left a comment

Choose a reason for hiding this comment

akirilov-arm commented Jan 14, 2022 •

edited

Loading

afonso360 commented Aug 16, 2022 •

edited

Loading