1732785 - Consider clobbering registers before jump to catch handler in throw stub

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, task, P3)

Description

[Ryan Hunt [:rhunt] (on leave until early May)]

In the wasm throw stub we will perform a longjmp to the nearest enclosing wasm catch handler. As a security hardening measure, we should consider clobbering registers immediately before the jump [1] to mitigate possible regalloc bugs in the function we're jumping to. There could be other security issues this would help out with as well, the regalloc one just seems the most likely.

Two questions if we do this:

1. Which registers? I think we could clobber all GPR's. I don't think we need to do floating point registers.
2. What value to clobber with? I think zero is the safest, but maybe there's a sentinel value that could help us identify crashes with?

I don't think this would be a code bloat issue as this is once per-module. I don't think we would be concerned with performance either, as we're intentionally not concerned with performance of the exceptional path.

[1] https://searchfox.org/mozilla-central/rev/1df999af9999ccb436512cfece57a68d94d36e08/js/src/wasm/WasmStubs.cpp#2857

Comment 1

[Lars T Hansen [:lth]]

In principle all GPRs except nonvolatile registers (tls, heapreg, sp, psp). I think clobbering the FPRs would be a good thing too; if we're concerned about data leakage, we should worry about all data. This is especially true as wasm makes it fairly easy to control / predict register content.

I think using a nonzero sentinel value would be good. A signaling NaN with a distinct payload might be good for the FPRs.

Comment 2

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: Bug 1732785 - wasm: Clobber registers before jump in throw stub. r?lth

This commit changes the throw stub to clobber most registers
before jumping to the catch landing pad. The exact details of
which registers to clobber is commented in the stub.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30a14fc76d6f
wasm: Clobber registers before jump in throw stub. r=lth

Comment 4

[Norisz Fay [:noriszfay]]

Backed out for causing SM bustages on Assembler-arm.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/60101e3d02df7032545c71f18df2cf7942d64bce

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=YqXpKrUzRgmaFkSSVIp_kQ.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=30a14fc76d6fb8f3ad13f58e7477a3e5a2e013e0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=362697406&repo=autoland

Failure line: Assertion failure: dest.isDouble(), at /builds/worker/checkouts/gecko/js/src/jit/arm/Assembler-arm.cpp:1587

Comment 5

[Ryan Hunt [:rhunt] (on leave until early May)]

Arm32 needs special handling due to its unique FPU registers.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/522bac8e0bb9
wasm: Clobber registers before jump in throw stub. r=lth

Comment 7

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/522bac8e0bb9