Wasmtime GitHub notifications bot (Apr 30 2022 at 00:10): Wasmtime GitHub notifications bot (Apr 30 2022 at 00:10):cfallin opened PR #4092 from x64-spectre-table-bounds to main:
Currently, we have partial Spectre mitigation: we protect heap accesses
with dynamic bounds checks. Specifically, we guard against errant
accesses on the misspeculated path beyond the bounds-check conditional
branch by adding a conditional move that is also dependent on the
bounds-check condition. This data dependency on the condition is not
speculated and thus will always pick the "safe" value (in the heap case,
a NULL address) on the misspeculated path, until the pipeline flushes
and recovers onto the correct path.This PR uses the same technique both for table accesses -- used to
implement Wasm tables -- and for jumptables, used to implement Wasm
br_tableinstructions.In the case of Wasm tables, the cmove picks the table base address on
the misspeculated path. This is equivalent to reading the first table
entry. This prevents loads of arbitrary data addresses on the
misspeculated path.In the case of
br_table, the cmove picks index 0 on the misspeculated
path. This is safer than allowing a branch to an address loaded from an
index under misspeculation (i.e., it preserves control-flow integrity
even under misspeculation).The table mitigation is controlled by a Cranelift setting, on by
default. The br_table mitigation is always on, because it is part of the
single lowering pseudoinstruction. In both cases, the impact should be
minimal: a single extra cmove in a (relatively) rarely-used operation.The table mitigation is architecture-independent (happens during
legalization); the br_table mitigation has been implemented for both x64
and aarch64. (I don't know enough about s390x to implement this
confidently there, but would happily review a PR to do the same on that
platform.)<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Apr 30 2022 at 00:10):cfallin requested abrown for a review on PR #4092.
Wasmtime GitHub notifications bot (Apr 30 2022 at 00:10):cfallin requested akirilov-arm for a review on PR #4092.
Wasmtime GitHub notifications bot (Apr 30 2022 at 00:10):cfallin requested fitzgen for a review on PR #4092.
Wasmtime GitHub notifications bot (Apr 30 2022 at 01:28):cfallin updated PR #4092 from x64-spectre-table-bounds to main.
Wasmtime GitHub notifications bot (Apr 30 2022 at 06:34):bjorn3 created PR review comment:
This removes new_inst and not inst, right?
Wasmtime GitHub notifications bot (Apr 30 2022 at 06:34):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (Apr 30 2022 at 21:50):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Apr 30 2022 at 21:50):cfallin created PR review comment:
No, it removes
inst; you can verify by looking at the added tests in this PR that the conditional move (selectif_spectre_guard) is present. The reason is thatreplace_with_aliasesreplaces uses ofinst's results but the cursor still remains atinst.
Wasmtime GitHub notifications bot (May 02 2022 at 17:35):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (May 02 2022 at 18:19):cfallin merged PR #4092.
Last updated: Oct 23 2024 at 20:03 UTC