1453416 - MacroAssembler::wasmCallIndirect can clobber WasmTlsReg with a null tls pointer

Status: RESOLVED INVALID

(Core :: JavaScript Engine: JIT, enhancement, P1)

Description

[Lars T Hansen [:lth]]

wasmCallIndirect() loads the target tls pointer into WasmTlsReg before checking that it is null.  If it is null it traps.  The trap handler does not restore the register, so we end up returning up the stack with a null tls.  Stub code that needs the tls after the call to wasm returns will then go wrong.

At worst we'll be looking at an NPE here if anyone can exploit this, which I doubt (this was found because I had a use for the WasmTlsReg in a new way), so not s-s, but we might consider an uplift in any case.

There's already a bit of register pressure in this code so careful surgery is called for.

Comment 1

[Lars T Hansen [:lth]]

So it struck me that this may not technically be a bug.  It is true that the WasmTlsReg shall be preserved across wasm calls, but that is from the point of view of a wasm caller.  The caller in bug 1445277 is not wasm code, it is stub code...  And at present no wasm code can actually observe the buggy behavior.

I think we should probably fix this anyway, but let's get a second opinion.  Luke?

Comment 2

[Luke Wagner [:luke]]

So one subtle aspect of WasmTlsReg is that it isn't assumed to be valid by default; only at certain program points such as at entry and exit from functions.  In contrast, fp is valid by default (with certain notable exceptions that are handled by the profiling frame iter).  Thus, when code doesn't know if WasmTlsReg is valid, it can simply reload it from fp->tls.  Stub code should, in general, not assume WasmTlsReg is valid and reload from fp->tls as necessary.

Comment 3

[Lars T Hansen [:lth]]

The stubs do not, so far as I can see, create a wasm-compatible frame for themselves, indeed around the call for the C++ -> Wasm path the FP is null.  So I think saving and restoring the FP as I was already doing is probably the best bet, though I'll look a little more closely.