1940716 - Crash [@ ??] on AArch64 with --disable-main-thread-denormals

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20250109-61a6063d8ba3 (opt build, run with --fuzzing-safe --ion-offthread-compile=off --disable-main-thread-denormals --baseline-eager test.js):

function a(b) { 
  c = new WebAssembly.Module(b);
  return new WebAssembly.Instance(c);
}
function d(e) {
  return a(wasmTextToBinary(e));
}
f = [ , Number.MIN_VALUE ]
let { refTest } = d(`(func (export "refTest") (param externref))`).exports;
for (h of f)
  refTest(h);

Backtrace:

received signal SIGTRAP, Trace/breakpoint trap.
#0  0x00000db42acf1314 in ?? ()
#1  0x000031de00e68d30 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
x0	0x1	1
x1	0xb4b87420	268902344455200
x2	0xda826b18	281474347723544
x3	0x2acf1150	15067463487824
x4	0xda826688	281474347722376
x5	0xda826690	281474347722384
x6	0xda826798	281474347722648
x7	0x0	-1407374883553280
x8	0x1	1
x9	0x1e420b00	-6731958325090317568
x10	0x0	9218868437227405312
x11	0xe04d20	54829567200544
x12	0xda8267a0	281474347722656
x13	0xffffffff	4294967295
x14	0xffffffff	4294967295
x15	0x0	0
x16	0x0	0
x17	0xb9cb21b0	268902429565360
x18	0x1	1
x19	0x1	1
x20	0xfffff800	4294965248
x21	0xda826bf0	281474347723760
x22	0xb9936a50	268902425913936
x23	0xb4b87420	268902344455200
x24	0x0	0
x25	0xda826cb8	281474347723960
x26	0xda826d88	281474347724168
x27	0xe6b150	54829567619408
x28	0xda826ad0	281474347723472
x29	0xda826af0	281474347723504
x30	0x2acf135c	15067463488348
sp	0xda826ad0	281474347723472
pc	0x2acf1314	15067463488276
cpsr	[ EL=0 BTYPE=0 SSBS C Z ]
fpcsr	void
fpcr	[ RMode=0 FZ ]
=> 0xdb42acf1314:	brk	#0xf000
   0xdb42acf1318:	ldr	x21, [x23]

Please keep closed as security-sensitive even if the issue itself turns out not to be, until we are done testing with --disable-main-thread-denormals.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Bugmon [:jkratzer for issues]]

Unable to reproduce bug 1940716 using build mozilla-central 20250109041657-61a6063d8ba3. Without a baseline, bugmon is unable to analyze this bug.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 4

[Nicolas B. Pierron [:nbp]]

The problem happens in the code generated by GenerateJitEntry when handling MIRType::WasmAnyRef case, after the failure to convert a value to an integer and to convert it back, we trigger the breakpoint and this ends the execution.

#0  js::jit::MacroAssemblerCompat::breakpoint (this=0x7ffcf2cb7410, tag=0 '\000') at /home/nicolas/mozilla/wksp-7/js/src/jit/arm64/MacroAssembler-arm64.cpp:438
#1  0x00005629f78a73ce in GenerateJitEntry (masm=..., funcExportIndex=0, fe=..., funcType=..., funcPtr=..., offsets=0x7ffcf2cb72ac) at /home/nicolas/mozilla/wksp-7/js/src/wasm/WasmStubs.cpp:1194
#2  0x00005629f78ae9b7 in js::wasm::GenerateEntryStubs (masm=..., funcExportIndex=0, fe=..., funcType=..., callee=..., isAsmJS=false, codeRanges=0x7fec86686490) at /home/nicolas/mozilla/wksp-7/js/src/wasm/WasmStubs.cpp:3230
#3  0x00005629f78ae6db in js::wasm::GenerateEntryStubs (codeMeta=..., exports=..., code=0x7fec866863c0) at /home/nicolas/mozilla/wksp-7/js/src/wasm/WasmStubs.cpp:3195
#4  0x00005629f77adf24 in js::wasm::ModuleGenerator::finishTier (this=0x7ffcf2cb8ab0, linkData=0x7ffcf2cb8820) at /home/nicolas/mozilla/wksp-7/js/src/wasm/WasmGenerator.cpp:1164
#5  0x00005629f77ae069 in js::wasm::ModuleGenerator::finishModule (this=0x7ffcf2cb8ab0, bytecode=..., moduleMeta=..., maybeCompleteTier2Listener=0x0) at /home/nicolas/mozilla/wksp-7/js/src/wasm/WasmGenerator.cpp:1183
#6  0x00005629f775f3f6 in js::wasm::CompileBuffer (args=..., bytecode=..., error=0x7ffcf2cba1c0, warnings=0x7ffcf2cba250, listener=0x0) at /home/nicolas/mozilla/wksp-7/js/src/wasm/WasmCompile.cpp:931
#7  0x00005629f77edf7c in js::WasmModuleObject::construct (cx=0x7fec8663a200, argc=1, vp=0x7fec8647faa0) at /home/nicolas/mozilla/wksp-7/js/src/wasm/WasmJS.cpp:1667
#8  0x00005629f62a566c in CallJSNative (cx=0x7fec8663a200, native=0x5629f77edb70 <js::WasmModuleObject::construct(JSContext*, unsigned int, JS::Value*)>, reason=js::CallReason::Call, args=...)
    at /home/nicolas/mozilla/wksp-7/js/src/vm/Interpreter.cpp:532
#9  0x00005629f62a5814 in CallJSNativeConstructor (cx=0x7fec8663a200, native=0x5629f77edb70 <js::WasmModuleObject::construct(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/nicolas/mozilla/wksp-7/js/src/vm/Interpreter.cpp:550
#10 0x00005629f627ad67 in InternalConstruct (cx=0x7fec8663a200, args=..., reason=js::CallReason::Call) at /home/nicolas/mozilla/wksp-7/js/src/vm/Interpreter.cpp:756
#11 0x00005629f627b0a6 in js::ConstructFromStack (cx=0x7fec8663a200, args=..., reason=js::CallReason::Call) at /home/nicolas/mozilla/wksp-7/js/src/vm/Interpreter.cpp:803
#12 0x00005629f6ffba54 in js::jit::DoCallFallback (cx=0x7fec8663a200, frame=0x7fec8647fb08, stub=0x7fec866db3c0, argc=1, vp=0x7fec8647faa0, res=...) at /home/nicolas/mozilla/wksp-7/js/src/jit/BaselineIC.cpp:1682

The sequence of code executed which yield to the execution of the breakpoint is:

0x14df41a962d0: fcvtzs  w0, d16   ; d16 = 4.9406564584124654e-324 (0x1)
0x14df41a962d4: scvtf   d31, w0   ; w0 = 0
0x14df41a962d8: fcmp    d31, d16  ; d31 = 0
0x14df41a962dc: b.ne    #+0x98 (addr 0x14df41a96374)
0x14df41a96374: brk     #0xf000

———
This analysis can be reproduced by using the ARM64 simulator, and instrumenting the breakpoint function as follows:

void MacroAssemblerCompat::breakpoint() {
  static uint16_t extra_tag = 0;
  // Note, other payloads are possible, but GDB is known to misinterpret them
  // sometimes and iloop on the breakpoint instead of stopping properly.
  extra_tag = ++extra_tag % 0x1000;
  Brk(0xf000 | extra_tag);
}

When the execution ends, the breakpoint extra_tag can be displayed using the vixl::GdbDisassembleInstruction function on the pc_ argument given to the Decode function. Using a conditional breakpoint with the extra_tag value can help find the corresponding stack. The sequence to identify convertDoubleToInt32 can be found by calling the disassembling function every time the Decode function is called.

Comment 5

[Nicolas B. Pierron [:nbp]]

I have not yet figured out the gist of it. So far I managed to isolate a where the denormal / non-denormal interpretation starts to diverge on an x64 build. Which is in the convertInt32ToDouble function, where a denormal of 0x1 sets the zero-flag within the WasmAnyRef path.

(x64) In the denormals-enabled case the CoerceInPlace_JitEntry gets called while in the denormals-disabled it is not.
I am going to investigate the difference between x64 denormals-disabled and arm64 denormals-disabled builds.

I have yet figured out who this belongs to, while the code is within WebAssembly, it runs the MAcroAssembler is used as much in JS.

Comment 6

[Nicolas B. Pierron [:nbp]]

Ok I apparently miss quoted the assembly, and the problem comes from the way negative-zero checks is performed within convertDoubleToInt32.

On x64 the negative-zero test will succeed at saying that this is not a negative zero value, whereas on Arm64 our implementation would match a negative-zero value after comparing the raw bits with 0 and fail by jumping to the breakpoint.

Comment 7

[Nicolas B. Pierron [:nbp]]

Attachment: Bug 1940716 - convertDoubleToInt32: Clobber destination register when zero.

Comment 8

[Nicolas B. Pierron [:nbp]]

The patch fixes the problem by making sure that we zero the register in case the floating point value converts to zero, but the floating point register is non-zero. This happens with denormals (either positive or negative), that the converted value is converted to zero, but ARM and ARM64 are doing bad things, as opposed to x86 / x86_64.

ARM64 will fail any of the convert methods as the content of the register is non-zero upon the raw conversion. Thus interpreting denormals as -0.0 values.

ARM will take the first 32 bits, and compare it to 0x8000000 and fail only if they are equal. Thus interpreting denormals as results of the convertion from double to int32.

The ARM miss-interpretation of denormals has the potential to cause out-of-bounds accesses when denormals are disabled. The problem being that a known range of a floating point value, once converted to integer can yield a value which is outside the expected range. This is possibly a problem for IonMonkey on ARM when MNumberToInt32 is generated.

The problem also affect non TIER-1 backends (mips32, mips64, loong64, riscv64) which are suffering from similar miss-interpretations as ARM.

Comment 9

[Nicolas B. Pierron [:nbp]]

Attachment: Bug 1940716 - Add test case.

Comment 10

[Nicolas B. Pierron [:nbp]]

Comment on attachment 9460571 [details]
Bug 1940716 - convertDoubleToInt32: Clobber destination register when zero.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Quite easily.

The patch changes one function which is used by IonMonkey / WarpBuilder and which data is used to remove checks when running Range Analysis.
This implies that on ARM (not ARM64), and mips32, mips64, loong64, riscv64 backends that an OOB with indexes in the range [0..2^21] can easily be produced.

Note: This patch only handle the ARM backend and fixes a correctness issue on ARM64 backend (probably not sec-high)
Note: x86 / x64 are non affected.
Note: The backends for mips32, mips64, loong64, riscv64 are vulnerable. However I do not see any implementation of WebAudio denormals removal for these architecture. (which does not mean that they do not exists in a fork)

• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: The same patch should apply without issues, otherwise it should be trivial to replicate.
• How likely is this patch to cause regressions; how much testing does it need?: This patch is unlikely to cause a regression.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 11

[Tom Ritter [:tjr]]

Comment on attachment 9460571 [details]
Bug 1940716 - convertDoubleToInt32: Clobber destination register when zero.

Approved to land and request uplift

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9b5c247e242
convertDoubleToInt32: Clobber destination register when zero. r=jandem

Comment 13

[Nicolas B. Pierron [:nbp]]

Attachment: Bug 1940716 - convertDoubleToInt32: Clobber destination register when zero.

Original Revision: https://phabricator.services.mozilla.com/D234893

Comment 14

[Nicolas B. Pierron [:nbp]]

Attachment: Bug 1940716 - convertDoubleToInt32: Clobber destination register when zero.

Original Revision: https://phabricator.services.mozilla.com/D234893

Comment 15

[Nicolas B. Pierron [:nbp]]

Attachment: Bug 1940716 - convertDoubleToInt32: Clobber destination register when zero.

Original Revision: https://phabricator.services.mozilla.com/D234893

Comment 16

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: Exploitable out-of-bound access
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: low
• Explanation of risk level: Behavior should be identical except in Audio Worklets.
• String changes made/needed: N/A
• Is Android affected?: yes

Comment 17

[Phabricator Automation]

esr115 Uplift Approval Request

• User impact if declined: Exploitable out-of-bound access
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: low
• Explanation of risk level: Behavior should be identical except in Audio Worklets.
• String changes made/needed: N/A
• Is Android affected?: yes

Comment 18

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Exploitable out-of-bound access
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: low
• Explanation of risk level: Behavior should be identical except in Audio Worklets.
• String changes made/needed: N/A
• Is Android affected?: yes

Comment 19

[Donal Meehan [:dmeehan - PTO - Back Oct 13]]

https://hg.mozilla.org/mozilla-central/rev/c9b5c247e242

Comment 20

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/17ae09345d93

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/50808f99740b

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/426fa9a488a4

Comment 23

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

2 months ago, tjr placed a reminder on the bug using the whiteboard tag [reminder-test 2025-04-15] .

nbp, please refer to the original comment to better understand the reason for the reminder.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5bba79924f44
Add test case. r=jandem

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/5bba79924f44