Closed Bug 1518331 Opened 2 years ago Closed 2 years ago

Assertion failure: false (offset.isValid()), at js/src/jit/shared/Assembler-shared.h:286

Categories

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

x86
All
defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- fixed

People

(Reporter: gkw, Assigned: lth)

Details

(5 keywords, Whiteboard: [jsbugmon:update][adv-main66+])

Attachments

(7 files)

The following testcase crashes on mozilla-central revision aac6849c6ff0 (build with --32 --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion --spectre-mitigations=off w49-out.wrapper w49-out.wasm):

See attachment.

Backtrace:

#0 js::jit::HighWord (address=...) at js/src/jit/shared/Assembler-shared.h:286
#1 0x5709c035 in js::jit::HighWord (address=...) at js/src/jit/x86/Assembler-x86.h:230
#2 js::jit::HighWord (op=...) at js/src/jit/x86/Assembler-x86.h:224
#3 0x570a8326 in js::jit::MacroAssembler::wasmLoadI64 (this=0xffe2cc00, access=..., srcAddr=..., out=...) at js/src/jit/x86/MacroAssembler-x86.cpp:696
#4 0x570be984 in js::jit::CodeGeneratorX86::emitWasmLoad<js::jit::LWasmLoadI64> (this=0xffe2d688, ins=0xf1416fa0) at js/src/jit/x86/CodeGenerator-x86.cpp:258
#5 0x570a86a9 in js::jit::CodeGenerator::visitWasmLoadI64 (this=0xffe2d688, ins=0xf1416fa0) at js/src/jit/x86/CodeGenerator-x86.cpp:266
/snip

For detailed crash information, see attachment.

Seems to be 32-bit only, setting s-s to be safe as we've got macroassembler stuff on the stack and also seems to involve spectre mitigations.

Attachment #9034861 - Attachment description: Testcase → w49-out.wasm

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/25900f3b9936
user: Dan Gohman
date: Thu Feb 15 09:56:00 2018 +0200
summary: Bug 1435369: Implement non-trapping float-to-int conversions for WebAssembly r=luke

Dan, is bug 1435369 a likely regressor?

Blocks: 1435369
Flags: needinfo?(sunfish)

From the stack trace it doesn't immediately look like it, since this appears to be a crash while translating a load opcode, however I'll investigate further.

The attached wasm file does have saturating float-to-int conversions in it, so before bug 1435369, this wasm file would have just been rejected as invalid wasm.

Confirmed that this doesn't look related to bug 1435369. There's an i64.load with an address of i32.const 0x7fffffff, and codegen is attempting to compute an address for the high half of the 64-bit load, and finding that this pushes the offset out of representable range.

Flags: needinfo?(sunfish)

Not sure who should look at this then. Luke/Lars/Benjamin?

No longer blocks: 1435369
Flags: needinfo?(luke)
Flags: needinfo?(lhansen)
Flags: needinfo?(bbouvier)
Attached file w49-out.wasm

Here's a version of w49-out.wasm which has all saturating float-to-int conversions replaced by trapping float-to-int conversions. It still reproduces the same assertion failure.

Gary, could you try autobisectjs with this version?

It goes back to prior to m-c rev a98f615965d7 then, which was when the --spectre-mitigations=off flag was added. The testcase does not reproduce without the flag.

Not sure if Jan knows what changed to trigger the bug either. (sorry for all the needinfos, I'm not sure who is best to take a look either)

Flags: needinfo?(jdemooij)

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #11)

which was when the --spectre-mitigations=off flag was added. The testcase does not reproduce without the flag.

Hm I'd expect it to repro on that revision (a98f615965d7) and the parent revision without the flag, because at that point the flag wasn't used anywhere...

Flags: needinfo?(jdemooij)

Spectre mitigation is likely a red herring here because it likely {inadvertently,deliberately,occasionally} disables the following optimization and hence masks the problem:

There's an optimization on x86 that allows for a constant base address to be pushed through to code generation without being placed in a register provided the address is zero or the load/store offset is zero, since in this case the add that asserts right now can't assert (one of the operands will be zero). For int64 this is not true since we have an additional offset, which we fail to add here.

See https://searchfox.org/mozilla-central/source/js/src/jit/x86/Lowering-x86.cpp#280.

We can disable the optimization for int64 or we can have an additional check for int64 on the constant value so that we know constant value + 4 does not overflow; ideally we'd abstract this in some way. Does not seem difficult.

I guess in the worst case this bug is actually a bounds check bypass, even if we normally run with spectre mitigations.

Like Jan I'd expect this to repro before spectre mitigations were added, but IIRC the HighWord/LowWord abstraction that catches the problem here was also added sometime during the previous year, and it's possible we didn't have this guard a year ago.

Flags: needinfo?(lhansen)
Flags: needinfo?(bbouvier)
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
OS: Linux → All
Priority: -- → P1
Attached file oob-x86.js

Rather smaller test case. Compile with --enable-debug, run with --no-wasm-baseline --spectre-mitigations=off. Not a problem if spectre mitigations are enabled, presumably that forces the pointer into a register.

Flags: needinfo?(luke)

I think this should be adequate. No comparable code on ARM to worry about that I can see.

No TC included, and anodyne commit msg, until we know security rating.

Attachment #9035070 - Flags: review?(luke)
Attachment #9035070 - Flags: review?(luke) → review+

Re security classification: What's going on here is that there is an overflow in an address calculation in the assembler when a certain large constant address is used to perform an int64 access. The calculation uses CheckedInt so it asserts in debug builds; in release builds the result of the overflowing calculation is zero. In the case where the result is zero no OOB access actually occurs, though the incorrect cell in the wasm memory may be read or written provided a program can be crafted that makes the optimizer elide the normal explicit bounds check; frankly I doubt this can be done.

And all of this is only with --spectre-mitigations=off, not our default.

As a consequence I would say that this is barely a security issue, certainly not sec-high, so I will go ahead and land. I'll request an uplift to beta because the patch applies as-is but I don't see it as critical.

Actually, probably not even beta-worthy.

From what you're saying, I agree that this can just ride the trains.

Also, this was pushed to inbound by lth:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0985217ed619

Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Component: JavaScript Engine → Javascript: Web Assembly
Flags: qe-verify+
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main66+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.