1664979 - Crash [@ ??] with invalid read involving asm.js

Status: VERIFIED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20200914-09390cf1d667 (debug build, run with --fuzzing-safe --ion-offthread-compile=off):

var all = [undefined, null, ];
function AsmModule(stdlib) {
    "use asm";
    var fround = stdlib.Math.fround;
    function fltConvNot(y38) {
        y38 = fround(y38);
        var i38 = 0;
        i38 = ~((~~y38) | 0);
        return (!!i38) | 0;
    }
    return {
        fltConvNot: fltConvNot,
    };
}
var asmModule = AsmModule({
    Math: Math
});
for (var i38 = 0; i38 < 10; ++i38)
  asmModule.fltConvNot(all[i38])

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x0000289c56a8c00a in ?? ()
#1  0x00007fffffffb438 in ?? ()
#2  0x00007fffffffb8b8 in ?? ()
#3  0x0000289c56a7c11b in ?? ()
#4  0x00007ffff4de13c0 in ?? ()
#5  0x000000007fc00000 in ?? ()
#6  0x000000007fc00000 in ?? ()
#7  0x0000000000000000 in ?? ()
rax	0x289c56a8c000	44651933908992
rbx	0x7fffffffb570	140737488336240
rcx	0x7fffffffb901	140737488337153
rdx	0x1	1
rsi	0x7ffff4de13c0	140737301582784
rdi	0x7fffffffb8b8	140737488337080
rbp	0x7fffffffb2d8	140737488335576
rsp	0x7fffffffb2d8	140737488335576
r8	0xffffd555559ce578	-46912491428488
r9	0xffffd555559ce568	-46912491428504
r10	0xaaaaaaaaaaaaaaaa	-6148914691236517206
r11	0x7ffff4a7c6a8	140737298024104
r12	0x7fffffffb468	140737488335976
r13	0x1	1
r14	0x7fffffffb8b8	140737488337080
r15	0x0	0
rip	0x289c56a8c00a	44651933909002
=> 0x289c56a8c00a:	mov    0x1e0(%r10),%r10
   0x289c56a8c011:	movl   $0x1,0x78(%r10)

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20200915092930-24b917577203.
The bug appears to have been introduced in the following build range:

Start: 83a3ed07220dfe4c8a2d16a2c48cfa8d49635362 (20200914043200)
End: 09390cf1d66716146aeb3112f0ac640e317890d9 (20200914054405)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=83a3ed07220dfe4c8a2d16a2c48cfa8d49635362&tochange=09390cf1d66716146aeb3112f0ac640e317890d9

Comment 3

[Dmitry Bezhetskov]

Thanks Christian, I managed to reproduce the SIGSEGV, I'll provide a fix soon.

Comment 4

[Dmitry Bezhetskov]

Attachment: Bug 1664979 - Fix tls offset in case of widenFloatToDouble is true. r=lth

When we do Push(srcSingle) inside outOfLineTruncateSlow but we don't adjust tlsOffset.
To account for this push Push(WasmTlsReg) was moved deeper. Also, it
allows us to reduce the code size a little bit.

Comment 5

[Jan de Mooij [:jandem]]

Would be great if we could land this ASAP to help fuzzing.

decoder is fighting a few different crashes in JIT code and it's hard to categorize those.

Comment 6

[Dmitry Bezhetskov]

I can't land without the approve and then I don't have permissions to land by myself, I can only put the checkinNeeded flag in the review.

Comment 7

[Lars T Hansen [:lth]]

I'll attempt to review (late) this afternoon, and land it if I r+ it.

Comment 8

[Dmitry Bezhetskov]

Attachment: Bug 1664979 - Fix tls offset in case of widenFloatToDouble is true [fast fix]. r=lth

When we do Push(srcSingle) inside outOfLineTruncateSlow but we don't adjust tlsOffset.

Comment 9

[Dmitry Bezhetskov]

I've create the fast fix version of the previous patch to land them asap.
Actually it is just a one-liner patch: https://phabricator.services.mozilla.com/D90537

Comment 10

[Lars T Hansen [:lth]]

Oh, forgot to clear the ni?. I reviewed the fast fix last night; it needs further work.

Comment 11

[Dmitry Bezhetskov]

Updated the fast fix, I think we can land it and then replace it with the more robust fix via the normal schedule.

Comment 12

[Lars T Hansen [:lth]]

This is good to go and a nightly-only regression, so I'll land it. I'll try to get to the other patch this week but we may possibly not land until after the merge window.

Comment 13

[Lars T Hansen [:lth]]

That fix was backed out for setting the tree on fire (https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2550e7f176e49dc7f44fbe449a7e7c75e0b2c1bd). I think need to see a fresh green try run for the other patch probably by tomorrow, or the main option left to me will be to start backing out the patches from bug 1639153 before the merge window closes.

Comment 14

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

Backout got merged: https://hg.mozilla.org/mozilla-central/rev/2550e7f176e4

Comment 15

[Lars T Hansen [:lth]]

Upping to S1 due to related bug.

Comment 16

[Lars T Hansen [:lth]]

This can be fixed by backing out part 6.6 from bug 1639153 (changeset 09390cf1d667, there's a small conflict but it is easily resolved).

Comment 17

[Lars T Hansen [:lth]]

I'm going to backout part 6.6, patch to do so will appear here and will need uplift. Doing a broad try run now (unavoidable), https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7986401ed5cb95bce8daa27bb9982d86276d9f0.

Comment 18

[Lars T Hansen [:lth]]

Recent patches from Bug 1639153 (eight of them, from part 6 through part 6.6 plus a subsequent fix) were backed out on my request, and the backout will be uplifted to beta. That should take care of this problem and at least bug 1664045. The backout was clean.

I see there's a sec-high rating here, that's probably overstating it. The crash here is because a register value that should contain the TLS pointer on a callout to C++ contains something else due to faulty register save/restore logic. While an attacker can perhaps set things up so as to get a value into that register (and this is not obvious), it is hard to see that value being used effectively from asm.js in the window that's available until the value is used in a way that will cause a crash. (This is my attempt at a post-facto justification for the backout without asking permission.)

Comment 19

[Julien Cristau [:jcristau]]

AIUI we can call this one fixed?
https://bugzilla.mozilla.org/show_bug.cgi?id=1639153#c100
https://bugzilla.mozilla.org/show_bug.cgi?id=1639153#c101

Comment 20

[Lars T Hansen [:lth]]

Almost certainly, though decoder promised to re-test once the nightlies are live.

Comment 21

[Christian Holler (:decoder)]

(In reply to Lars T Hansen [:lth] from comment #20)

Almost certainly, though decoder promised to re-test once the nightlies are live.

Confirmed, this is fixed.

Comment 22

[Jason Kratzer [:jkratzer]]

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

Comment 23

[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.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Should we land the testcase from this bug?

Comment 25

[Lars T Hansen [:lth]]

Yes, I'll take care of it. Will land with the test case on bug 1666051.