1675216 - Assertion failure: !temp.Is(rt), at jit/arm64/vixl/MacroAssembler-vixl.cpp:1205

Status: RESOLVED FIXED

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

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: test.wasm

This is test.wrapper, test.wasm is attached.

new WebAssembly.Instance(new WebAssembly.Module(read(scriptArgs[0], 'binary')));

Assertion failure: !temp.Is(rt), at /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/vixl/MacroAssembler-vixl.cpp:1205

Thread 1 "js-dbg-64-armsi" received signal SIGSEGV, Segmentation fault.
vixl::MacroAssembler::LoadStoreMacro (this=0x7fffffff8b70, rt=..., addr=..., op=vixl::LDR_x) at /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/vixl/MacroAssembler-vixl.cpp:1205
1205	    VIXL_ASSERT(!temp.Is(rt));
(gdb) bt
#0  vixl::MacroAssembler::LoadStoreMacro (this=0x7fffffff8b70, rt=..., addr=..., op=vixl::LDR_x) at /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/vixl/MacroAssembler-vixl.cpp:1205
#1  0x00005555583aea5a in js::jit::MacroAssemblerCompat::loadPtr (this=0x7fffffff8b70, address=..., dest=...) at /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/MacroAssembler-arm64.h:786
#2  js::jit::MacroAssemblerCompat::jump (this=0x7fffffff8b70, addr=...) at /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/MacroAssembler-arm64.h:725
#3  js::wasm::GenerateFunctionPrologue (masm=..., funcTypeId=..., tier1FuncIndex=..., offsets=<optimized out>) at /home/skygentoo/trees/mozilla-central/js/src/wasm/WasmFrameIter.cpp:635
#4  0x000055555835ef8f in js::wasm::BaseCompiler::beginFunction (this=<optimized out>) at /home/skygentoo/trees/mozilla-central/js/src/wasm/WasmBaselineCompile.cpp:5281
#5  0x000055555831cda8 in js::wasm::BaseCompiler::emitFunction (this=0x7fffffff8150) at /home/skygentoo/trees/mozilla-central/js/src/wasm/WasmBaselineCompile.cpp:15451
#6  js::wasm::BaselineCompileFunctions (moduleEnv=..., compilerEnv=..., lifo=..., inputs=..., code=0x7ffff4fe64b0, error=0x7fffffffb138) at /home/skygentoo/trees/mozilla-central/js/src/wasm/WasmBaselineCompile.cpp:15624
#7  0x00005555583b7738 in ExecuteCompileTask (task=0x7ffff4fe6100, error=0x7fffffffb138) at /home/skygentoo/trees/mozilla-central/js/src/wasm/WasmGenerator.cpp:769
#8  0x00005555583b83c7 in js::wasm::ModuleGenerator::locallyCompileCurrentTask (this=0x7fffffff9be0) at /home/skygentoo/trees/mozilla-central/js/src/wasm/WasmGenerator.cpp:819
#9  js::wasm::ModuleGenerator::finishFuncDefs (this=0x7fffffff9be0) at /home/skygentoo/trees/mozilla-central/js/src/wasm/WasmGenerator.cpp:957
#10 0x000055555832c03e in DecodeCodeSection<js::wasm::Decoder> (env=..., d=..., mg=...) at /home/skygentoo/trees/mozilla-central/js/src/wasm/WasmCompile.cpp:569
#11 0x000055555832bca6 in js::wasm::CompileBuffer (args=..., bytecode=..., error=0x7fffffffb138, warnings=<optimized out>, listener=0x0, telemetrySender=...) at /home/skygentoo/trees/mozilla-central/js/src/wasm/WasmCompile.cpp:592
#12 0x00005555583f14ed in js::WasmModuleObject::construct (cx=<optimized out>, argc=<optimized out>, vp=<optimized out>) at /home/skygentoo/trees/mozilla-central/js/src/wasm/WasmJS.cpp:1631
#13 0x00005555572acdc2 in CallJSNative (cx=0x7ffff6926000, native=0x5555583f11f0 <js::WasmModuleObject::construct(JSContext*, unsigned int, JS::Value*)>, reason=<optimized out>, args=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:507
#14 0x00005555572b28c9 in CallJSNativeConstructor (cx=0x7ffff6926000, native=0x5555583f11f0 <js::WasmModuleObject::construct(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:523
#15 0x000055555729d9b8 in InternalConstruct (cx=<optimized out>, args=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:708
#16 0x0000555557d33ecf in js::jit::DoCallFallback (cx=0x7ffff6926000, frame=0x7ffff677fec0, stub=<optimized out>, argc=1, vp=0x7ffff677fe50, res=...) at /home/skygentoo/trees/mozilla-central/js/src/jit/BaselineIC.cpp:2990
#17 0x0000555557ee42de in vixl::Simulator::VisitCallRedirection (this=0x7ffff6944000, instr=0x7ffff6958f68) at /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/vixl/MozSimulator-vixl.cpp:649
#18 0x0000555557ee307d in vixl::Simulator::VisitException (this=0x7ffff6944000, instr=0x7ffff6958f68) at /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/vixl/MozSimulator-vixl.cpp:429
#19 0x0000555557e91bcd in vixl::Decoder::VisitException (this=<optimized out>, instr=0x7ffff6958f68) at /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/vixl/Decoder-vixl.cpp:876
#20 0x0000555557ee2027 in vixl::CachingDecoder::Decode (this=0x7ffff6902be0, instr=0x7ffff6958f68) at /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/vixl/MozSimulator-vixl.cpp:941
#21 0x0000555557ee1699 in vixl::Simulator::ExecuteInstruction (this=0x7ffff6944000) at /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/vixl/MozSimulator-vixl.cpp:224
#22 0x0000555557ee579c in vixl::Simulator::Run (this=0x7ffff6944000) at /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/vixl/Simulator-vixl.cpp:70
#23 0x0000555557ee2d96 in vixl::Simulator::RunFrom (this=0x7ffff6944000, first=<optimized out>) at /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/vixl/Simulator-vixl.cpp:78
#24 vixl::Simulator::call (this=0x7ffff6944000, entry=<optimized out>, argument_count=<optimized out>) at /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/vixl/MozSimulator-vixl.cpp:310
#25 0x00005555581cd9de in EnterJit (cx=0x7ffff6926000, state=..., code=<optimized out>) at /home/skygentoo/trees/mozilla-central/js/src/jit/Jit.cpp:108
#26 js::jit::MaybeEnterJit (cx=0x7ffff6926000, state=...) at /home/skygentoo/trees/mozilla-central/js/src/jit/Jit.cpp:199
#27 0x000055555728783f in js::RunScript (cx=0x7ffff6926000, state=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:462
#28 0x000055555729e86c in js::ExecuteKernel (cx=0x7ffff6926000, script=..., envChainArg=..., newTargetValue=..., evalInFrame=..., result=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:856
#29 0x000055555729ed50 in js::Execute (cx=0x7ffff6926000, script=..., envChain=..., rval=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:888
#30 0x0000555557459f25 in ExecuteScript (cx=0x7ffff6926000, envChain=..., script=..., rval=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/CompilationAndEvaluation.cpp:423
#31 0x000055555745a110 in JS_ExecuteScript (cx=0x7ffff6926000, scriptArg=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/CompilationAndEvaluation.cpp:456
#32 0x00005555571de2ac in RunFile (cx=0x7ffff6926000, filename=0x5b344f861762c <error: Cannot access memory at address 0x5b344f861762c>, file=<optimized out>, compileMethod=<optimized out>, compileOnly=false) at /home/skygentoo/trees/mozilla-central/js/src/shell/js.cpp:974
#33 0x00005555571dd9ad in Process (cx=0x7ffff6926000, filename=<optimized out>, forceTTY=false, kind=FileScript) at /home/skygentoo/trees/mozilla-central/js/src/shell/js.cpp:1565
#34 0x00005555571a51e6 in ProcessArgs (cx=<optimized out>, op=<optimized out>) at /home/skygentoo/trees/mozilla-central/js/src/shell/js.cpp:10371
#35 Shell (cx=0x7ffff6926000, op=<optimized out>, envp=<optimized out>) at /home/skygentoo/trees/mozilla-central/js/src/shell/js.cpp:11079
#36 0x000055555719dbf9 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at /home/skygentoo/trees/mozilla-central/js/src/shell/js.cpp:11855

Due to skipped revisions, the first bad revision could be any of:
changeset:   https://hg.mozilla.org/mozilla-central/rev/09eb16b8a220
user:        Julian Seward
date:        Tue Aug 04 18:53:20 2020 +0000
summary:     Bug 1657131 - Bump cranelift version to 25e31739a63b7a33a4a34c961b88606c76670e46.  r=cfallin.

changeset:   https://hg.mozilla.org/mozilla-central/rev/057cd099f3bc
user:        Julian Seward
date:        Tue Aug 04 18:53:20 2020 +0000
summary:     Bug 1655928 - Add SpiderMonkey-side artefacts to support Wasm Atomics on Cranelift/newBE/AArch64 (CL PR2077).  r=cfallin.

Run with --fuzzing-safe --ion-offthread-compile=off --ion-eager test.wrapper test.wasm, compile with AR=ar sh ./configure --enable-simulator=arm64 --enable-debug --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests, tested on m-c rev ad4d5535cb4f.

Not sure if this is s-s, I'd leave it to Julian/Chris.

Comment 1

[Christian Holler (:decoder)]

This doesn't cause any issues for me on real ARM64, so it could be a simulator-only problem (the assert is also in simulator code).

Comment 2

[Lars T Hansen [:lth]]

If this is the macroassembler then it's more likely stubs/baseline compiler. I can take a look.

Comment 3

[Lars T Hansen [:lth]]

Certainly there's a bug here in the jump(Address) implementation, it uses the temp register ip0 directly without claiming it from the scratch register pool. Later, when vixl needs a temp it happens to get the same one (from the pool) and fails because it asserts that it can't be the same register as the target register it's given. The jump() function has been unchanged for two years, and was then touched only by the Big Gratuitous Reformatting, so it's an old bug. The immediate problem here seems to be that we now have tiering on ARM64, hence there is code that uses jump(Address) where perhaps there was none before. That's the code in GenerateFunctionPrologue.

I can't repro locally (yet) but will try some more. But likely we just need to make sure always to claim a scratch register from the pool.

Comment 4

[Lars T Hansen [:lth]]

Reproducing turns out to be much easier when not enabling SIMD (since enabling SIMD disables Cranelift and hence tiering)...

Comment 5

[Lars T Hansen [:lth]]

Indeed the temps used by the macroassembler are ip0 and ip1: https://searchfox.org/mozilla-central/source/js/src/jit/arm64/vixl/MacroAssembler-vixl.cpp#36.

In addition to the problem in jump(), which appears to be fixed by using the scratch register scope, there are a handful of naked uses of ip0 throughout our arm64 masm code. Full list:

https://searchfox.org/mozilla-central/source/js/src/jit/arm64/MacroAssembler-arm64.h#725 -- definitely wrong, but fixed by patch
https://searchfox.org/mozilla-central/source/js/src/jit/arm64/MacroAssembler-arm64.h#1302 -- makes me nervous
https://searchfox.org/mozilla-central/source/js/src/jit/arm64/MacroAssembler-arm64.cpp#982 -- makes me very nervous
https://searchfox.org/mozilla-central/source/js/src/jit/arm64/Trampoline-arm64.cpp#174 -- makes me nervous
https://searchfox.org/mozilla-central/source/js/src/jit/arm64/Assembler-arm64.cpp#130 -- probably ok

For at least some of these, it should be possible to provoke assert crashes (and incorrect execution) with carefully engineered inputs. For the first and third, which are control flow operations, I would expect exploitation to be possible, if hard.

Comment 6

[Lars T Hansen [:lth]]

These problems could also crash the browser with legitimate content, as Gary has discovered, at which point there's not much of a workaround, so P1/S1. I'll try to attend to it this week. For this test case at least it's a nightly-only problem (no tiering off nightly) but we could imagine problems more generally.

Comment 7

[Lars T Hansen [:lth]]

Found no naked uses of ip1 in the code.

Comment 8

[Nicolas B. Pierron [:nbp]]

https://searchfox.org/mozilla-central/rev/e75e8e5b980ef18f4596a783fbc8a36621de7d1e/js/src/jit/arm64/MacroAssembler-arm64.h#724-726

Looking for calls of an analog function on ARM, it seems to me that all ARM uses cases are unlikely to make use of ip0 register as the call to the jump(Address&) function is not called within another MacroAssembler function, which would already have taken this temporary register. Thus the case where the jump function override the ip0 register while already being used is non-existent.

Thus what remains, is the case where the jump > loadPtr > Ldr > LoadStoreMacro attempts to use the ip0 register in the computation of the target address, which is being reported here.

This case sounds like it could be a false positive, as least in the case of a load. In the case of a load, the fact that the destination register is clobbered before the load to store the computation of the offset should not matter.

 ip0 <-- offset // Assert here!
 ip0 <-- *(base + ip0)

Note, this assertion is still valid in the case of a store instruction, thus we should not remove this assertion.

Conclusion:
So, there is indeed an inconsistency in the usage of temporary register in jump(const Address&), which is not a security issue, as ip0 register is not used prior the use of the jump function, and within it is used as a temporary register for the address computation prior the actual use of it as a temporary register for the branching case.

Comment 9

[Nicolas B. Pierron [:nbp]]

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

https://searchfox.org/mozilla-central/source/js/src/jit/arm64/MacroAssembler-arm64.h#1302 -- makes me nervous
https://searchfox.org/mozilla-central/source/js/src/jit/arm64/MacroAssembler-arm64.cpp#982 -- makes me very nervous

ip0 is a volatile register, thus they are not preserved across function calls. Seeing it used in a call / return instruction as a temporary destination sounds consistent with the fact that this is a volatile register.

https://searchfox.org/mozilla-central/source/js/src/jit/arm64/Trampoline-arm64.cpp#174 -- makes me nervous

This is a hard coded stub, it is likely fine, otherwise we might have bigger issues, including correctness issues.

Comment 10

[Lars T Hansen [:lth]]

Thanks for the analysis. It is good to know that it is not exploitable.

There however seems to be no good reason for our masm to use ip0 without claiming it from the temp pool, and we should fix these occurences. If the specific register ip0 must be used by some instance then we should still use the temp pool and grab a register from it and assert that it is in fact ip0.

Comment 11

[Lars T Hansen [:lth]]

A further complication is that ip0 is sometimes known as 'ScratchReg' and 'ScratchReg_64' and ip1 is sometimes known as 'ScratchReg2' and 'ScratchReg2_64', and the latter is sometimes used without reserving it first. (In fact ip1 is used by that name in the same function in Trampoline-arm64.cpp that uses ip0.)

Comment 12

[Lars T Hansen [:lth]]

Attachment: Bug 1675216 - Insert assertions that scratch registers are available when they are used. r?nbp

Comment 13

[Lars T Hansen [:lth]]

That patch should not change any behavior, it just asserts that the temps have not been claimed when they are used by masm code. However, running the wasm tests locally, wasm/regress/ion-callerfp-tag.js now fails (in the updated 'call' code), so there is a conflict in the use of scratches. Will investigate that next, and will also run many more tests.

Comment 14

[Lars T Hansen [:lth]]

Attachment: Bug 1675216 - fix a register scoping bug

Depends on D95867

Comment 15

[Lars T Hansen [:lth]]

I still need to do a full arm64 jstests run with DEBUG; this is a little bit of a headache. It does not look like this is a configuration we're running on CI (at least not on autoland), which is a shame, as it will catch more bugs. The results are architecture-specific and it is not sufficient to run it on one architecture, but it is sufficient to run it on simulator.

Comment 16

[Christian Holler (:decoder)]

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

I still need to do a full arm64 jstests run with DEBUG; this is a little bit of a headache. It does not look like this is a configuration we're running on CI (at least not on autoland), which is a shame, as it will catch more bugs. The results are architecture-specific and it is not sufficient to run it on one architecture, but it is sufficient to run it on simulator.

I can help with this, if it is about running tests on real hardware. We have plenty of it in AWS.

Comment 17

[Lars T Hansen [:lth]]

Anything you can do to further that goal would be most helpful. I'm running tests locally now and that'll be sufficient for the purposes of this bug, but a noopt-debug or (faster) opt-debug run of all of jstests and jittests and in general as much code as possible is sufficient to catch these scratch register scope issues.

Also cc'ing Steve to see if there's anything we can do on CI with the simulator. Arm64 is increasingly important (arguably will be more important than both Arm32 and x86-32 within the year) and we need to beef up our testing significantly.

Comment 18

[Christian Holler (:decoder)]

If you need real ARM64 Linux in CI, then I can bring this up again. We should have this ability by now.

Comment 19

[Lars T Hansen [:lth]]

Crucially we must run the jstests (and ideally anything JS and wasm) with a debug configuration. In the past, the jstest have been somewhat of a blocker because they are slow, and the debug configuration does not help that.

Comment 20

[Lars T Hansen [:lth]]

Local arm64 reldebug test on all of jstests turned up no further scoping issues.

I'm going to designate this as sec-other since comment 8 suggests there is no real danger. And it's a recent nightly-only regression so I'll just land it.

Comment 21

[Christian Holler (:decoder)]

$ ./mach jstests --shell /srv/builds/debugarm64/dist/bin/js -j16 --tbpl
[257754|     0|     0|  5724] 100% ==================================>|7406.8s
PASS

Looks like we are good. Should I open up this bug since apparently, there are no potentially dangerous findings?

Comment 22

[Lars T Hansen [:lth]]

Thanks. I'm going to trust nbp without checking his work, so yes, should be OK to open.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/890642dbd606
Insert assertions that scratch registers are available when they are used. r=nbp

Comment 24

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/890642dbd606

Comment 25

[Steve Fink [:sfink] [:s:]]

(In reply to Christian Holler (:decoder) from comment #18)

If you need real ARM64 Linux in CI, then I can bring this up again. We should have this ability by now.

Yes, please. I read through bug 1636245 and it looks like this isn't a generally available worker type yet, but as you said in bug 1636245 comment 29, it should be. It would be awesome if you could push it through. I'm willing to work on any CI configuration stuff necessary to create new jobs for this purpose, once a worker type is available. Thanks!

Comment 26

[Christian Holler (:decoder)]

(In reply to Steve Fink [:sfink] [:s:] from comment #25)

(In reply to Christian Holler (:decoder) from comment #18)

If you need real ARM64 Linux in CI, then I can bring this up again. We should have this ability by now.

Yes, please. I read through bug 1636245 and it looks like this isn't a generally available worker type yet, but as you said in bug 1636245 comment 29, it should be. It would be awesome if you could push it through. I'm willing to work on any CI configuration stuff necessary to create new jobs for this purpose, once a worker type is available. Thanks!

I filed bug 1675561 for this purpose.