Stream: git-wasmtime

Topic: wasmtime / PR #7468 PCC: fully support dynamic and static...

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 05:27):

cfallin requested fitzgen for a review on PR #7468.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 05:27):

cfallin requested wasmtime-core-reviewers for a review on PR #7468.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 05:27):

cfallin requested wasmtime-compiler-reviewers for a review on PR #7468.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 05:27):

cfallin opened PR #7468 from cfallin:pcc-dynamic-wasm to bytecodealliance:main:

This PR completes the work to add proof-carrying-code validation to Wasm heap accesses in Wasmtime, for all bounds-check cases (dynamic and static, covering the 4 and 3 cases of the former and latter respectively), on x86-64 and aarch64.

The PR includes an integration test at the top level (tests/pcc_memory.rs) that compiles a Wasm module with PCC enabled under a range of memory configurations. Ideally we'd use the existing Wasm filetest infrastructure for this; but it has its own Wasm environment definitions and PCC hasn't been plumbed through those (this would also mean we'd be testing a slightly different path of at least the memory-type setup than production Wasmtime).

The test expectations change slightly because this PR had to change iadd_imm(x, -k) to isub(x, iconst(k)) in the generated bounds-checking code. Some of the backends (riscv64, s390x) seem to match iadd-of-negative better than isub-of-positive; but that's an orthogonal isel issue and can be fixed up.

I hope to add fuzzing to exercise this further, but we at least have (theoretical) functional completeness with this PR. Hence, I think this fixes #6090.

Followup work on PCC could include use of PCC annotations to verify table accesses as well, and a strong-enforcing mode that disallows all non-checked memory accesses (so we have to audit and allowlist constant pools and ABI code and the like, and ensure all lowered Wasm ops are covered). However I don't think this additional assurance level is necessary to turn on and benefit from Wasm-memory-bounds-checking PCC.

The first half of this PR was

Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 05:30):

cfallin updated PR #7468.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 18:03):

fitzgen submitted PR review:

r=me with comments addressed

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 18:03):

fitzgen submitted PR review:

r=me with comments addressed

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 18:03):

fitzgen created PR review comment:

Do we need this fact? Why isn't looking at the DFG itself good enough?

Ah is this for once we've lowered to machinsts and we need to know if a particular machinst is defining a Value? If so, can you clarify that in a comment here?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 18:03):

fitzgen created PR review comment:

Let's use i64::from here instead of as.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 18:03):

fitzgen created PR review comment:

Oh it is parsed as def(...) not value(...) here? Is the printer just wrong then?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 18:03):

fitzgen created PR review comment:

Can we make the text format and Rust type names align? This will make it easier when trying to figure out the implementation for stuff people see in the clif text format or vice versa.

So either def({value}) in the text format or rename Fact::Def to Fact::Value.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 18:03):

fitzgen created PR review comment:

Should this be a checked sub?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 18:03):

fitzgen created PR review comment:

Can this also add

In particular, we aren't exercising all branches in bounds_checks.rs right now because there are special cases for when offset + access_size == 1 for example.

For anything that isn't handled yet, could we still have the test do something like

for enable_spectre in [true, /* TODO: support pcc without spectre */] {
    ...
}

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 18:03):

fitzgen created PR review comment:

Can avoid writing out our own is_some method by removing AddrPcc::None and then using Option<AddrPcc> in place of where AddrPcc is used now. Rust will even pack the type into the same size. In general, Option<T> is preferred over adding a None variant to T since you get all the helpers and combinators like map and and_then etc.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2023 at 18:03):

fitzgen created PR review comment:

It is surprising to me that the Fact::value constructor doesn't create a Fact::Def variant aka a value(...) fact in the text format. Maybe more reason to use def(...) instead?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 00:07):

cfallin updated PR #7468.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 00:07):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 00:07):

cfallin created PR review comment:

Added the extra loops as signposts for future, good idea! Also expanded the test bodies to include all of i8, i16, i32, i64. The test bodies do have loads with offsets of 0 and 0x10000, so I think that was already covered.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 00:07):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 00:07):

cfallin created PR review comment:

Ah, yes, fixed.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 00:07):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 00:07):

cfallin created PR review comment:

Done!

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 00:07):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 00:07):

cfallin created PR review comment:

Done; i64::try_from actually (it's a u64 source value) but now make_compare takes an Option and only unwraps it if PCC is enabled, and PCC only supports memory32.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 00:07):

cfallin created PR review comment:

Fixed in the printer!

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 00:07):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 00:07):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 00:07):

cfallin created PR review comment:

Hopefully clearer after the doc-comment on def; I'll add a note here too.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 00:07):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 00:07):

cfallin created PR review comment:

Ah, yes, switched to def; that's what the parser takes, and what should have been here. Thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 00:07):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 00:07):

cfallin created PR review comment:

Added more to the doc-comment clarifying this!

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 00:10):

cfallin has enabled auto merge for PR #7468.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 01:09):

cfallin updated PR #7468.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 04:40):

cfallin updated PR #7468.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 04:40):

cfallin has enabled auto merge for PR #7468.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 04 2023 at 05:52):

cfallin merged PR #7468.

Last updated: Nov 22 2024 at 16:03 UTC