1736531 - WebAssembly Crash [@ GetClass] with potential memory corruption

Status: VERIFIED FIXED

(Core :: JavaScript: WebAssembly, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20211019-1e37fc4a7f53 (asan-opt build, run with --fuzzing-safe --ion-offthread-compile=off --baseline-eager --spectre-mitigations=off):

function a(b) {
    binary = wasmTextToBinary(b)
    c = new WebAssembly.Module(binary)
    return new WebAssembly.Instance(c)
}
for (e of [0,3]) {
  f = a(`
    (module
    (memory ${ e })
    (func 
      (export "grow1")
      (result i32)
      (memory.grow i32.const 1)
    ))
  `)
  while (f.exports.grow1());
}

Backtrace:

==27834==ERROR: AddressSanitizer: SEGV on unknown address 0x40007fff7fff (pc 0x56442df2f7fe bp 0x7ffe5cbaae60 sp 0x7ffe5cbaae60 T0)
==27834==The signal is caused by a READ memory access.
    #0 0x56442df2f7fe in GetClass dist/include/js/Object.h:47:56
    #1 0x56442df2f7fe in IsProxy dist/include/js/Proxy.h:379:10
    #2 0x56442df2f7fe in IsWrapper dist/include/js/Wrapper.h:399:10
    #3 0x56442df2f7fe in bool JSObject::is<js::WrapperObject>() const js/src/vm/WrapperObject.h:32:10
    #4 0x56442df1f05d in js::EmulatesUndefined(JSObject*) js/src/builtin/Boolean-inl.h:22:22
    #5 0x56442df8366a in js::ToBooleanSlow(JS::Handle<JS::Value>) js/src/builtin/Boolean.cpp:173:11
    #6 0x56442ed31de6 in ToBoolean dist/include/js/Conversions.h:128:10
    #7 0x56442ed31de6 in js::jit::DoToBoolFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICFallbackStub*, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) js/src/jit/BaselineIC.cpp:607:15
    #8 0x2b4dcb1939f3  (<unknown module>)

This crash only reproduces for me on an ASan optimized build (Use python3 -mfuzzfetch --target js -a -n opt64asan to get the TC build). It also only reproduces when with spectre mitigations turned off, my guess is there is some subtle memory corruption at work here that is influenced by memory layout. Marking s-s.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Lars T Hansen [:lth]]

That's an interesting test case... The function grow1() will return zero in exactly one case, namely, after the first grow call to a memory whose size is zero, ie the first memory. After that it will always return non-zero and thus the loop calling grow should never terminate. (Grow returns -1 when it can't grow any more.) So this would create a memory of size zero, then grow it by one page, then terminate the while. Then create a memory of size one, then forever try to grow it. At some point - about 0.1s on my machine, 64K calls - the grow will fail but the loop won't terminate. Presumably at some point this times out or is sent an interrupt?

Comment 4

[Lars T Hansen [:lth]]

Caused by memory64.

Comment 5

[Lars T Hansen [:lth]]

Tentatively this:

changeset:   666401:13ee9674ee35
user:        Lars T Hansen <[e-mail]>
date:        Mon Oct 18 09:58:14 2021 +0000
summary:     Bug 1727084 - Memory64 - Bulk memory operations. r=yury

Comment 6

[Lars T Hansen [:lth]]

Both Ion and Baseline appear to be affected.

Comment 7

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1727084

Comment 8

[Lars T Hansen [:lth]]

The problem here is as follows:

In Instance::memoryGrow_m32() we call WasmMemoryObject::grow() which returns 0xFFFF_FFFF_FFFF_FFFF for failure, because it has a 64-bit API. However, memoryGrow_m32() must return a 32-bit value (uint32_t) with 0xFFFF_FFFF signifying failure. We therefore cast the return value from grow() to uint32_t expecting it to be chopped with the equivalent of movl rax, rax, but no code is emitted even though the canonical representation for a 32-bit value in a 64-bit register on this platform is all-upper-bits-cleared. Thus rax flowing out of the function is 0xFFFF_FFFF_FFFF_FFFF. Not much later, this "integer" value is to be transformed to a JS value. The code that does that assumes that the upper bits are cleared and just ORs rax into a value template; obviously things go south from there.

What I don't know yet is whether I've violated some C++ semantics (eg, the cast is illegal if the value is out of range?) or if it's a compiler bug. I tried to AND the return value of grow() with 0xFFFF_FFFF before the cast, but to now avail. I tried some other casts too but they all boil away. A functional workaround is to store the value in memory and then load just the 32-bit low end of it; pretty ugly, but serviceable.

This is with Clang++ 12.0.1; I probably haven't run bootstrap since last week though.

It is possible that this is an artifact of the asan build too, and I will look into that.

Comment 9

[Lars T Hansen [:lth]]

While it is true that the x64 and arm64 architectures will clear the upper bits of the 64-bit register on a 32-bit operation (and ignore these bits in inputs, by and large), the ABI is another matter, and it's the ABI that matters here, with the value being returned.

When a 32-bit value is passed or returned in a 64-bit register, the Windows ABI states explicitly that the upper bits "can be ignored" in the parameter and "are undefined" on return (https://docs.microsoft.com/en-us/cpp/build/x64-calling-convention?view=msvc-160).

The System V AMD64 ABI is silent on the matter (except for _Bool, for which only the low eight bits are valid and whose value shall be 0b00000000 or 0b00000001; and except for the x32 model, in which the upper 32 bits are required to be zero).

I think in the final analysis the problem here lies in the instance-call mechanism on 64-bit systems: we must clear the upper bits of the returned 64-bit value to maintain our internal invariants.

Comment 10

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Verified bug as reproducible on mozilla-central 20211019095357-4185629111d3.
The bug appears to have been introduced in the following build range:

Start: e2b5e618de42b6de694ca95e75e9bd442c2ff9ff (20211018093031)
End: ee8efced380b871deac4fba285955953a4a89ef5 (20211018133124)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=e2b5e618de42b6de694ca95e75e9bd442c2ff9ff&tochange=ee8efced380b871deac4fba285955953a4a89ef5

Comment 11

[Lars T Hansen [:lth]]

Analysis (from a discussion with decoder):

This is strictly about 32-bit returns from C++ to wasm on 64-bit systems where the C++ code has been processing large 64-bit values and ends with a (no-op) truncation to 32-bit thus leaving garbage bits in the high bits of the return register, and then the value has to flow out to JS without being touched by the wasm code at all. (If it were touched by wasm the high bits would almost certainly be cleared since virtually every operation will clear them.)

A couple of other aspect further reduce the problem:

• Until a very short time ago (bug 1730161) we had no comprehensive support for 64-bit returns from C++ to wasm at all so the ground was prepared for this problem only very recently.
• It turns out that we had run into this problem before in a different guise, namely as a spectre issue, see bug 1646787. In that case, the garbage high bits could be sniffed as part of a spectre attack, so we added code to clear the high bits of 32-bit values after return. This is the reason why the present bug is only detectable if spectre mitigations are disabled.
• Currently, only memory.grow and memory.size are candidates for the offending C++ function, and among those only memory.grow has a return path that could trigger the bug. That code landed earlier this week (bug 1727084).

All that said, it turns out that while the spectre issue in bug 1646787 applies to all 64-bit systems, the mitigation was implemented only on x64. So that ought to be fixed too.

In sum, what's going to happen here is that the existing spectre mitigation will be repurposed to be applied unconditionally on all 64-bit systems, thus killing(?) both birds with one stone. When spectre mitigations are eventually removed, the fix will remain because it is no longer spectre-specific.

I think that we'll be OK to just land this on Nightly, there's no significant attack surface as things stand

Comment 12

[Lars T Hansen [:lth]]

Attachment: Bug 1736531 - Fix. r?jseward

See bug for analysis.

Comment 13

[Daniel Veditz [:dveditz]]

Could this be called "disabled" in Fx95 because we do have the Spectre mitigations in place? I do know people would like to turn those off, though.

Comment 14

[Lars T Hansen [:lth]]

(In reply to Daniel Veditz [:dveditz] from comment #13)

Could this be called "disabled" in Fx95 because we do have the Spectre mitigations in place?

I think that would be fair, modulo that spectre mitigation not being properly implemented on arm64 and mips64.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Fix. r=jseward
https://hg.mozilla.org/integration/autoland/rev/3ecda60613c5cb7a57cc7004919c7d12c3a29268
https://hg.mozilla.org/mozilla-central/rev/3ecda60613c5

Comment 16

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Verified bug as fixed on rev mozilla-central 20211022100052-c5a7a3551be5.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 18

[BugBot [:suhaib / :marco]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.