Stream: git-wasmtime

Topic: wasmtime / Issue #1930 Spectre mitigation on heap access ...

view this post on Zulip Wasmtime GitHub notifications bot (Jun 26 2020 at 21:53):

github-actions[bot] commented on Issue #1930:

Subscribe to Label Action

cc @bnjbvr

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:meta"

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

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

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Jun 29 2020 at 09:00):

julian-seward1 commented on Issue #1930:

(Speaking from a position of partial ignorance) it would be best to do this at the aarch64 emission point. I mean, add an AArch64::Inst case with whatever stuff we need, including the conditional move. Then we're guaranteed that no CLIR or isel transformation can break it, and also that regalloc won't insert any reloads in between the branch and cmov.

I suspect that's not feasible, because it would require a combined conditional-branch-and-cmov Inst, and branches are handled specially in the newBE. So the next best option is implement this at the isel level, I guess.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 29 2020 at 09:47):

bnjbvr commented on Issue #1930:

I suspect that's not feasible, because it would require a combined conditional-branch-and-cmov Inst, and branches are handled specially in the newBE. So the next best option is implement this at the isel level, I guess.

I think adding a spectre_icmp instruction at the CLIF level would be the most straightforward, and making so that it's marked as effectful so it does blocks hoisting, etc. Then its lowered form can be a single VCode inst that is generated as cmp + cmov.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 29 2020 at 15:12):

cfallin commented on Issue #1930:

These are all good points -- I hadn't considered that an effect-free cmov might be hoisted above the branch, for example, or legalized back into a form that gets speculated on.

I like @bnjbvr's suggestion the best here: a spectre_icmp that is a black box to optimization passes would stay in place on the correct path, and we could guarantee its lowering. Perhaps we could call it something like spectre_guard_cmov...

view this post on Zulip Wasmtime GitHub notifications bot (Jun 29 2020 at 21:13):

cfallin commented on Issue #1930:

Updated to use a select_spectre_guard instruction, which is like select except that it is (i) marked as effectful, and (ii) unknown to any of the optimization passes or legalizations. On AArch64, this lowers to the same conditional move.

Unfortunately, unless I'm mistaken, on x86 (legacy backend) it seems that full support for cmov instructions is missing? E.g., there is no legalization for select -> selectif, only expand_select that @bjorn3 pointed out (that always lowers to a CFG diamond); and in .../x86/opcodes.rs, there is only the opcode for CMOV_OVERFLOW. Perhaps I'm missing some interaction that results in full cmov support. However, for now, I have not added support for the Spectre-specific guard to the legacy x86 backend. Given our policy of putting any x86-specific efforts toward the new x86 backend, I think this should be OK; but if we feel differently, I can dig into this more and implement what is needed.

Because of that, the flag must be false by default to avoid breaking the legacy x86 backend. IMHO, we should flip it once we transition. Until then, we can ensure that we set it for specific embeddings that we control, e.g. in SpiderMonkey. Thoughts?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 30 2020 at 10:01):

bnjbvr commented on Issue #1930:

CMOV_OVERFLOW represents the initial bits for cmov with no cc set (well, it's because the condition code for overflow is 0), and ye olde recipe adds the cc bits correctly, when it encodes selectif. So I think it could be done without too much trouble in the old backend?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 30 2020 at 23:55):

cfallin commented on Issue #1930:

Just updated -- PTAL. Now the flag is on by default, and I managed to work out support for the legacy x86 backend as well -- by making our side-effecting-black-box-op selectif_spectre_guard rather than select_spectre_guard, we can just associate it with the same encoding as for selectif (except that no legalizations or optimizations understand the op so they will not alter it). I think this addresses all concerns above: on by default, works on all platforms, optimizations won't interfere. Thanks!

Last updated: Nov 22 2024 at 16:03 UTC