Crash [@ ??] with WebAssembly
Categories
(Core :: JavaScript: WebAssembly, defect, P1)
Tracking
()
People
(Reporter: decoder, Assigned: lth)
References
(Regression)
Details
(4 keywords, Whiteboard: [bugmon:update,bisected,confirmed][post-critsmash-triage][sec-survey][adv-main82+r][adv-esr78.4+r])
Crash Data
Attachments
(2 files)
546 bytes,
text/plain
|
Details | |
47 bytes,
text/x-phabricator-request
|
jcristau
:
approval-mozilla-beta+
pascalc
:
approval-mozilla-esr78+
dveditz
:
sec-approval+
|
Details | Review |
The following testcase crashes on mozilla-central revision 20200918-ab7d302fd318 (debug build, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off --more-compartments --warp):
evalInWorker(`
newGlobal();
newGlobal();
newGlobal();
newGlobal();
newGlobal();
i59 = wasmEvalText(
\`(module
(func (export "foo") (param i64) (result i64)
local.get 0
)
)\`,
).exports;
newGlobal();
function wasmEvalText(str, imports) {
let binary = wasmTextToBinary(str);
m = new WebAssembly.Module(binary);
return new WebAssembly.Instance(m, imports);
}
newGlobal();
function caller(n34) {
return i59.foo(42n);
}
for (let i59 = 0x10F000; i59-- > 0; )
caller(i59);
`);
Backtrace:
received signal SIGSEGV, Segmentation fault.
#0 0x00001093a012c38f in ?? ()
#1 0xaaaaaaaaaaaaaaaa in ?? ()
#2 0xaaaaaaaaaaaaaaaa in ?? ()
#3 0x00001093a015e850 in ?? ()
[...]
#21 0x0000000000000000 in ?? ()
rax 0x0 0
rbx 0x342315a00430 57325291308080
rcx 0x2 2
rdx 0x7ffff5996e58 140737313861208
rsi 0x0 0
rdi 0x7ffff5996e58 140737313861208
rbp 0x7ffff6965ad0 140737330436816
rsp 0x7ffff69659a0 140737330436512
r8 0x0 0
r9 0x7ffff69658e0 140737330436320
r10 0x1093a015e850 18226232027216
r11 0x1fff9 131065
r12 0x0 0
r13 0x0 0
r14 0x998a705040b94858 -7382965149134927784
r15 0x0 0
rip 0x1093a012c38f 18226231821199
=> 0x1093a012c38f: mov 0x20(%r14),%r10
0x1093a012c393: mov 0x1e0(%r10),%r10
Reporter | ||
Comment 1•4 years ago
|
||
Comment 2•4 years ago
|
||
Bugmon Analysis:
Failed to bisect testcase (End build crashes!):
Start: ab7d302fd3186b10ada9264528c80f6840e44571 (20200918211411)
End: ab4f6932ea8abfd45cb679568b95ec9394af6656 (20200919212721)
BuildFlags: BuildFlags(asan=False, tsan=False, debug=False, fuzzing=False, coverage=False, valgrind=False)
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.
Updated•4 years ago
|
Comment 3•4 years ago
|
||
Not a Warp bug. I can reproduce with the testcase below with --no-baseline --more-compartments --wasm-compiler=baseline
.
Wasm code is calling AllocateBigIntTenuredNoGC
and that fails (returns nullptr). Then we return to Wasm code where we do the following (crashing instruction marked):
0x1674d4e6d879: add $0x8,%rsp
0x1674d4e6d87d: mov 0x20(%r14),%r10
0x1674d4e6d881: mov 0x1e0(%r10),%r10
0x1674d4e6d888: movq $0x0,0x70(%r10)
0x1674d4e6d890: movl $0x0,0x78(%r10)
0x1674d4e6d898: pop %rbp
0x1674d4e6d899: pop %r14
0x1674d4e6d89b: retq
0x1674d4e4d24e: mov %rax,%r10
0x1674d4e4d251: test %r10d,%r10d
0x1674d4e4d254: je 0x1674d4e4d385
0x1674d4e4d385: mov 0x10(%rsp),%r10
0x1674d4e4d38a: and $0x3,%r10
0x1674d4e4d38e: test %r10,%r10
0x1674d4e4d391: je 0x1674d4e4d3a2
0x1674d4e4d397: cmp $0x1,%r10
0x1674d4e4d39b: je 0x1674d4e4d3a2
0x1674d4e4d3a1: int3
0x1674d4e4d3a2: mov 0x10(%rsp),%r10
0x1674d4e4d3a7: and $0xfffffffffffffffc,%r10
0x1674d4e4d3ab: mov 0x48(%r10),%r14 <======= CRASH HERE, r10 is 0x0
0x1674d4e4d3af: mov 0x20(%r14),%r10
0x1674d4e4d3b3: mov 0x1e0(%r10),%r10
0x1674d4e4d3ba: mov %rsp,0x70(%r10)
0x1674d4e4d3be: pushq $0x7
0x1674d4e4d3c0: mov 0x10(%r14),%r10
evalInWorker(`
function wasmEvalText(str, imports) {
let binary = wasmTextToBinary(str);
m = new WebAssembly.Module(binary);
return new WebAssembly.Instance(m, imports);
}
var exports = wasmEvalText(
\`(module
(func (export "foo") (param i64) (result i64)
local.get 0
)
)\`,
).exports;
for (var i = 0; i < 7; i++) {
newGlobal();
}
for (var i = 0; i < 1_000_000; i++) {
exports.foo(42n);
}
`);
Assignee | ||
Comment 4•4 years ago
|
||
Yeah, I think this may be Dmitry's thing, there's a cluster of bugs. I'm hoping to get that resolved tomorrow, and then we'll look again. Uplift will be required.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 6•4 years ago
|
||
Will test this further when the backout for all the patches from bug 1639153 has propagated to central, as discussed on IM.
Assignee | ||
Comment 7•4 years ago
|
||
With that bug patched out, I now hit a breakpoint in the debug build. Stack doesn't look useful. Plausible instructions around the crash location:
0xb247760b365: int3
0xb247760b366: movabs $0xb247762b7e0,%rax
0xb247760b370: callq *%rax
0xb247760b372: test $0xf,%spl
0xb247760b376: je 0xb247760b37d
0xb247760b37c: int3
0xb247760b37d: test %eax,%eax
0xb247760b37f: jne 0xb247760b140
0xb247760b385: mov 0x10(%rsp),%r10
0xb247760b38a: and $0x3,%r10
0xb247760b38e: test %r10,%r10
0xb247760b391: je 0xb247760b3a2
0xb247760b397: cmp $0x1,%r10
0xb247760b39b: je 0xb247760b3a2
0xb247760b3a1: int3
=> 0xb247760b3a2: mov 0x10(%rsp),%r10
0xb247760b3a7: and $0xfffffffffffffffc,%r10
0xb247760b3ab: mov 0x48(%r10),%r14
0xb247760b3af: mov 0x20(%r14),%r10
0xb247760b3b3: mov 0x1e0(%r10),%r10
0xb247760b3ba: mov %rsp,0x70(%r10)
0xb247760b3be: pushq $0x7
0xb247760b3c0: mov 0x10(%r14),%r10
0xb247760b3c4: mov 0x18(%r10),%r10
0xb247760b3c8: jmpq *%r10
The code @...8a looks like some kind of inefficient tag assertion (testing <= 1 would be better). It is very plausibly wasm code, given the central use of r14 (tls) following the int3. Indeed the tag check looks like the debug-only code from loadFunctionFromCalleeToken, and the subsequent code matches that too. Altogether the code looks like that from GenerateJitEntryLoadTls. The only caller of that that also generates a jump through r10 is GenerateJitEntryThrow.
Assignee | ||
Comment 8•4 years ago
•
|
||
Perturbing the test case, I crash with a zero callee token @..ab. It's the same problem: the callee token is busted.
It's possible we've not run into this before because there is an allocation failure here, and that we've not tested that. We are on the return path from the call into wasm and we're trying to box up a bigint, but can't.
Looking at the code, it does not seem like it has a coherent view of what the stack looks like. The jump from within the bigint boxing out to the throw code looks particularly wrong.
Assignee | ||
Comment 9•4 years ago
|
||
Assignee | ||
Comment 10•4 years ago
|
||
We're going to want this on beta, I think.
The fix is pretty simple. Except for the one case in JitEntry, the BigInt allocation code always branches to the throw stub, which is very liberal in the VM state it accepts. But in JitEntry the state has to be just so: the stack frame has to be a precise known size for the callee token to be found and the throw to work. So we can move the OOM check to the point after the temporary frame for the C++ call has been torn down, and then we can re-erect a dummy frame in JitEntry itself on our way to the exception handler in the OOM case. It's not precisely elegant but it makes sense, I think.
Assignee | ||
Comment 11•4 years ago
•
|
||
A variation on the test case (--no-ion --no-baseline --more-compartments --wasm-compiler=baseline), which we can land later:
evalInWorker(`
newGlobal();
newGlobal();
newGlobal();
newGlobal();
newGlobal();
newGlobal();
newGlobal();
let i59 = new WebAssembly.Instance(new WebAssembly.Module(wasmTextToBinary(
\`(module
(func (export "foo") (param i64) (result i64)
local.get 0
)
)\`,
))).exports;
for (let x = 0 ; x < 1000000; x++ )
i59.foo(42n);
`);
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 12•4 years ago
|
||
Comment on attachment 9178464 [details]
Bug 1666140 - Fix OOM bailout path. r?rhunt
Security Approval Request
- How easily could an exploit be constructed based on the patch?: Likely difficult. It's an OOM path where exception handling code picks up a bogus value from the stack; engineering this value to be something specific will be hard.
- Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
- Which older supported branches are affected by this flaw?: FF78 and forward
- If not all supported branches, which bug introduced the flaw?: None
- Do you have backports for the affected branches?: Yes
- If not, how different, hard to create, and risky will they be?: The patch applies cleanly to ESR78 and should apply to beta (not tested). Still need to check that the bug reproduces w/o and is fixed w/ the patch but confident this is fine.
- How likely is this patch to cause regressions; how much testing does it need?: I think it is not likely to cause regressions, and we have jit-tests that cover the paths that were not affected by the bug.
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 13•4 years ago
|
||
Verified that the crash reproduces on ESR78 and is fixed by the patch.
Updated•4 years ago
|
Comment 14•4 years ago
|
||
Comment on attachment 9178464 [details]
Bug 1666140 - Fix OOM bailout path. r?rhunt
sec-approval=dveditz
![]() |
||
Comment 15•4 years ago
|
||
https://hg.mozilla.org/integration/autoland/rev/fe1b1a8fd6190c3eca1258f7e48ce6e99d726b04
https://hg.mozilla.org/mozilla-central/rev/fe1b1a8fd619
Assignee | ||
Comment 16•4 years ago
|
||
Comment on attachment 9178464 [details]
Bug 1666140 - Fix OOM bailout path. r?rhunt
Beta/Release Uplift Approval Request
- User impact if declined: Mysterious crash or misbehavior in case of an OOM error along a particular wasm <-> JS path.
- Is this code covered by automated tests?: No
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Localized fix, fairly easy to confirm by inspection that it is not breaking anything.
- String changes made/needed:
Assignee | ||
Comment 17•4 years ago
|
||
Comment on attachment 9178464 [details]
Bug 1666140 - Fix OOM bailout path. r?rhunt
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration:
- User impact if declined: Mysterious crashes or misexecution along an OOM path at the wasm <-> JS boundary.
- Fix Landed on Version: 83
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Local fix; pretty easy to argue by inspection that it does not break anything.
- String or UUID changes made by this patch:
Comment 18•4 years ago
|
||
Comment on attachment 9178464 [details]
Bug 1666140 - Fix OOM bailout path. r?rhunt
approved for 82.0b7
Updated•4 years ago
|
Comment 19•4 years ago
|
||
Comment 20•4 years ago
|
||
Comment on attachment 9178464 [details]
Bug 1666140 - Fix OOM bailout path. r?rhunt
Approved for ESR
Comment 21•4 years ago
|
||
Comment 22•4 years ago
|
||
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.
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Description
•