1874502 - (CVE-2024-1552) Assertion failure: dest.isDouble(), at jit/arm/MacroAssembler-arm.cpp:195

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

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

Attachment: debug shell stack

function f(x) {
  Math.fround(function () {
    x;
  });
}
for (let j = 0; j < 29; j++) {
  f(Math.fround(1));
}

Assertion failure: dest.isDouble(), at /home/gen32gx500/trees/mozilla-central/js/src/jit/arm/MacroAssembler-arm.cpp:195
#01: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x2731ee3]
#02: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x274e9ad]
#03: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x26fbd29]
#04: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x290a2fe]
#05: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x296abb1]
#06: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x29b42c2]
#07: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x29b56d5]
#08: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x29b62de]
#09: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x29b6c47]
#10: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x2763d65]
#11: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x2760d57]
#12: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x27696ab]
#13: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x2769c70]
#14: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x25d2b87]
#15: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x1a5b0d6]
#16: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x1a55073]
#17: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x1a54b9f]
#18: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x1a57e1c]
#19: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x1a5828a]
#20: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x1bdecb8]
#21: JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>)[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x1bdee98]
#22: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x1994677]
#23: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x1993767]
#24: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x1953758]
#25: ???[/home/gen32gx500/shell-cache/js-dbg-32-armsim32-linux-x86_64-734e2e027196/js-dbg-32-armsim32-linux-x86_64-734e2e027196 +0x194d636]
Segmentation fault

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/ee6468bab0dd
user:        Jan de Mooij
date:        Wed Apr 05 11:37:15 2023 +0000
summary:     Bug 1826078 part 4 - Optimize GC barriers for stores to MNewCallObject. r=iain

Run with --fuzzing-safe --no-threads --fast-warmup, compile with PKG_CONFIG_PATH=/usr/lib/x86_64-linux-gnu/pkgconfig 'CC="clang -msse2 -mfpmath=sse"' 'CXX="clang++ -msse2 -mfpmath=sse"' AR=ar sh ../configure --host=x86_64-pc-linux-gnu --target=i686-pc-linux --enable-simulator=arm --enable-debug --enable-debug-symbols --with-ccache --enable-gczeal --enable-rust-simd --disable-tests, tested on m-c rev 734e2e027196.

Jan/Iain, is bug 1826078 a likely regressor? Setting s-s just in case.

Comment 1

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

Set release status flags based on info from the regressing bug 1826078

Comment 2

[Iain Ireland [:iain]]

I can reproduce this locally.

The bug is silly and trivial:

In this code in moveValue:

ScratchFloat32Scope scratch(*this);
  FloatRegister freg = reg.fpu();
  if (type == MIRType::Float32) {
    convertFloat32ToDouble(freg, scratch);
    freg = scratch;
  }
  ma_vxfer(freg, dest.payloadReg(), dest.typeReg());

convertFloat32ToDouble expects the destination register to be a double, but we are instead using the float32 scratch register. We must not enter this block in any of our tests, because it will fail every time. The fix is to use ScratchDoubleScope instead.

In non-debug builds, It looks like we actually end up generating the same code for the convertFloat32ToDouble, but slightly different code for the ma_vxfer below. Specifically, we want:

eeb7fae0      vcvt f64.f32 d15, s1
ec524b1f      vmov r4, r2, d15

but we generate

eeb7fae0      vcvt f64.f32 d15, s1
ec524a1f      vmov r4, r2, s30,s31

Coming into this code, the single-precision FPR s1 contains some number. After this code, r4 and r2 contain a JS::Value. We want them to contain a the double-precision representation of s1. In the buggy case, they instead contain whatever happened to be in s30 and s31.

Most of the time, that will result in r4/r2 safely encoding a DoubleValue. The only case in which it would not is if it encodes a NaN, in which case it could be interpreted as any arbitrary Value, potentially opening us up to type confusion. To do so, all 11 exponent bits must be set. This implies that the corresponding bits in s30 must be set. Since there are only 8 exponent bits in a 32-bit float, A) s30 would have to be a NaN itself, and B) the 3 most significant bits of the payload of s30 must be set.

I can't think of any way to accomplish B. There are very few ways for JS code to manipulate the float32 registers. With some very delicate massaging of the register allocator, it might be possible to get a 32-bit NaN in s30 using (eg) Math.fround(0) / Math.fround(0), but that will generate the default hardware NaN, with all payload bits set to 0.

So I think this is probably not exploitable, but it's possible I'm missing something. It's probably safest to keep it closed for now, at least until someone has had a chance to double-check my work.

This was introduced by bug 1523941, although it might not have been reachable before bug 1826078.

Comment 3

[Iain Ireland [:iain]]

Attachment: Bug 1874502: Use ScratchDoubleScope in moveValue r=jandem

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/974c389fc056
Use ScratchDoubleScope in moveValue r=jandem

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/974c389fc056

Comment 6

[Frederik Braun [:freddy]]

Gary: We're awarding an amount of the lower end of the spectrum for bug bounty, given Ian's assessment that this is likely not exploitable. We would reconsider if you can bring evidence that indicates we've been wrong.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR approval when you get a chance. It grafts cleanly.

Comment 8

[Iain Ireland [:iain]]

Comment on attachment 9372669 [details]
Bug 1874502: Use ScratchDoubleScope in moveValue r=jandem

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: We think this bug is unlikely to be exploitable, but the patch is small and well understood.
• User impact if declined: Incorrect code generation in a weird case. Small chance that we've missed an avenue of exploitation. Only occurs on 32-bit Arm devices.
• Fix Landed on Version: 123
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's a one-line patch fixing an obvious bug in a cold code path.

Beta/Release Uplift Approval Request

• User impact if declined: Incorrect code generation in a weird case. Small chance that we've missed an avenue of exploitation. Only occurs on 32-bit Arm devices.
• Is this code covered by automated tests?: Yes
• 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): It's a one-line patch fixing an obvious bug in a cold code path.
• String changes made/needed: None
• Is Android affected?: Yes

Comment 9

[Iain Ireland [:iain]]

Not sure if we also want this in beta/release, so I added those requests just in case.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9372669 [details]
Bug 1874502: Use ScratchDoubleScope in moveValue r=jandem

This landed during the last cycle, so it's already on track to ship in the 123 release.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9372669 [details]
Bug 1874502: Use ScratchDoubleScope in moveValue r=jandem

Approved for 115.8esr.

Comment 12

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/4aebdf426218

Comment 13

[Tom Ritter [:tjr]]

Attachment: advisory.txt