1666140 - Crash [@ ??] with WebAssembly

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

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

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jason Kratzer [:jkratzer]]

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.

Comment 3

[Jan de Mooij [:jandem]]

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);
  }
`);

Comment 4

[Lars T Hansen [:lth]]

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.

Comment 5

[Lars T Hansen [:lth]]

This is not fixed by backing out part 6.6 from bug 1639153.

Comment 6

[Lars T Hansen [:lth]]

Will test this further when the backout for all the patches from bug 1639153 has propagated to central, as discussed on IM.

Comment 7

[Lars T Hansen [:lth]]

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.

Comment 8

[Lars T Hansen [:lth]]

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.

Comment 9

[Lars T Hansen [:lth]]

Attachment: Bug 1666140 - Fix OOM bailout path. r?rhunt

Comment 10

[Lars T Hansen [:lth]]

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.

Comment 11

[Lars T Hansen [:lth]]

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);
`);

Comment 12

[Lars T Hansen [:lth]]

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.

Comment 13

[Lars T Hansen [:lth]]

Verified that the crash reproduces on ESR78 and is fixed by the patch.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 9178464 [details]
Bug 1666140 - Fix OOM bailout path. r?rhunt

sec-approval=dveditz

Comment 15

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

https://hg.mozilla.org/integration/autoland/rev/fe1b1a8fd6190c3eca1258f7e48ce6e99d726b04
https://hg.mozilla.org/mozilla-central/rev/fe1b1a8fd619

Comment 16

[Lars T Hansen [:lth]]

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:

Comment 17

[Lars T Hansen [:lth]]

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

[Julien Cristau [:jcristau]]

Comment on attachment 9178464 [details]
Bug 1666140 - Fix OOM bailout path. r?rhunt

approved for 82.0b7

Comment 19

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0f4ad29c7026

Comment 20

[Pascal Chevrel:pascalc]

Comment on attachment 9178464 [details]
Bug 1666140 - Fix OOM bailout path. r?rhunt

Approved for ESR

Comment 21

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/1a793d052f25

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

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.