1879939 - (CVE-2024-2607) Assertion failure: volatileLiveRegs.has(ReturnRegVal0), at jit/arm/MacroAssembler-arm.cpp:5994

Status: RESOLVED FIXED

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

Description

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

Attachment: debug stack

var x = 2;
(1).toString(x)[0];
for (var y = 0; y < 1; y++) {}

(gdb) bt
#0  js::jit::MacroAssembler::flexibleDivMod32 (this=0xf63bb318, rhs=..., 
    lhsOutput=..., remOutput=..., isUnsigned=<optimized out>, volatileLiveRegs=...)
    at /home/gen32gx500/trees/mozilla-central/js/src/jit/arm/MacroAssembler-arm.cpp:5994
#1  0x591f9d9e in js::jit::MacroAssembler::loadInt32ToStringWithBase (
    this=0xf63bb318, input=..., base=..., dest=..., scratch1=..., scratch2=..., 
    staticStrings=..., volatileRegs=..., lowerCase=<optimized out>, fail=0xf63d1414)
    at /home/gen32gx500/trees/mozilla-central/js/src/jit/MacroAssembler.cpp:1884
#2  0x58f9d5c7 in js::jit::CodeGenerator::visitInt32ToStringWithBase (
    this=0xf63bb300, lir=0xf63ce6d8)
    at /home/gen32gx500/trees/mozilla-central/js/src/jit/CodeGenerator.cpp:10821
#3  0x58f78f9e in js::jit::CodeGenerator::generateBody (this=0xf63bb300)
    at /home/gen32gx500/trees/mozilla-central/js/src/jit/CodeGenerator.cpp:7388
#4  0x58fdedb1 in js::jit::CodeGenerator::generate (this=0xf63bb300)
    at /home/gen32gx500/trees/mozilla-central/js/src/jit/CodeGenerator.cpp:15230
#5  0x5902ae72 in js::jit::GenerateCode (mir=0xf63ca100, lir=0xf63cda08)
    at /home/gen32gx500/trees/mozilla-central/js/src/jit/Ion.cpp:1585
#6  js::jit::CompileBackEnd (mir=0xf63ca100, snapshot=0xf63ca518)
    at /home/gen32gx500/trees/mozilla-central/js/src/jit/Ion.cpp:1614
#7  0x5902c285 in js::jit::IonCompile (cx=0xf7614100, script=..., 
    osrPc=<optimized out>)
    at /home/gen32gx500/trees/mozilla-central/js/src/jit/Ion.cpp:1736
#8  js::jit::Compile (cx=0xf7614100, script=..., osrFrame=<optimized out>, 
    osrPc=0xf7603b12 "\227\b")
    at /home/gen32gx500/trees/mozilla-central/js/src/jit/Ion.cpp:1890
#9  0x5902ce8e in BaselineCanEnterAtBranch (cx=0xf7614100, osrFrame=0xf67fff08, 
    script=..., pc=<optimized out>)
    at /home/gen32gx500/trees/mozilla-central/js/src/jit/Ion.cpp:2091
#10 IonCompileScriptForBaseline (cx=<optimized out>, frame=0xf67fff08, 
    pc=0xf7603b12 "\227\b")
    at /home/gen32gx500/trees/mozilla-central/js/src/jit/Ion.cpp:2143
#11 0x5902d7f7 in js::jit::IonCompileScriptForBaselineOSR (cx=0xf7614100, 
    frame=0xf67fff08, frameSize=48, pc=0xf7603b12 "\227\b", infoPtr=0xf67ffee8)
    at /home/gen32gx500/trees/mozilla-central/js/src/jit/Ion.cpp:2255
/snip

Run with --fuzzing-safe --no-threads --ion-eager --arm-hwcap=vfp, compile with PKG_CONFIG_PATH=/usr/lib/x86_64-linux-gnu/pkgconfig AR=ar 'CC="clang -msse2 -mfpmath=sse"' 'CXX="clang++ -msse2 -mfpmath=sse"' 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 13676fc9b0cd.

This seems to regress somewhere between m-c rev fac963c3bc00 (Aug 2022) and m-c rev e963fffcb3a0 (Jun 2023) but I'm still digging more into this.

Setting s-s to be safe.

Comment 1

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

I got it down to a range between Sep 2022 and Dec 2022:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8e4ceab106b12c3816163829494ac5e7938a6be6&tochange=62de122b294888133131b4a03c0985736522b0e9

but I'll keep trying.

One thing though, bug 1802497 is in the regression range and seems to involve the MacroAssembler but I tried the patch that :anba posted yesterday in bug 1879688 and it does not seem to fix this issue.

Setting needinfo? from :mgaudet who may have added MacroAssembler::flexibleDivMod32 in bug 1438727 (this is on the top of the debug stack here, and is pointed out by :anba in bug 1879688) as a start.

I'll keep bisecting to see if I can nail a cause.

Comment 2

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

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/1c95cd727056
user:        André Bargull
date:        Tue Nov 29 13:52:54 2022 +0000
summary:     Bug 1802497 - Part 5: Inline Number.prototype.toString when an explicit "base" argument is present. r=jandem

Bug 1802497 again, but likewise, I wonder if it's just exposing the issue from bug 1438727.

Comment 3

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

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

Comment 4

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

Oh, erm, BugBot, I should have set to bug 1802497 for now, until the devs look at it.

Comment 5

[André Bargull [:anba]]

(In reply to Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) from comment #1)

One thing though, bug 1802497 is in the regression range and seems to involve the MacroAssembler but I tried the patch that :anba posted yesterday in bug 1879688 and it does not seem to fix this issue.

This is similar to bug 1879688: MacroAssembler::flexibleDivMod32 was added in bug 1438727, but the code path which can trigger the assertion is only possible through bug 1802497. This bug is about arm32, whereas bug 1879688 is for x86, though.

Comment 6

[André Bargull [:anba]]

Attachment: Bug 1879939: Save output registers. r=jandem!

Comment 7

[André Bargull [:anba]]

Attachment: Bug 1879939: Add test case. r=jandem!

Depends on D201684

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b111d27fa95
Save output registers. r=jandem

Comment 9

[Daniel Veditz [:dveditz]]

André: does your comment 5 mean this bug is not a security issue just as bug 1879688 wasn't?

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0b111d27fa95

Comment 11

[André Bargull [:anba]]

(In reply to Daniel Veditz [:dveditz] from comment #9)

André: does your comment 5 mean this bug is not a security issue just as bug 1879688 wasn't?

No, this is a different case. This bug can overwrite return registers on arm32 platforms when the sdiv instruction isn't supported ("idiva" processor flag). AFAICT, Armv7-A is still supported as tier 1 and in Armv7-A, sdiv is only optionally supported.

Comment 12

[Dianna Smith [:diannaS]]

:dveditz This was declared sec-high post landing in central. Does D201684 need sec-approval to land on esr115?
:andré also, can you nominate this for uplift?

Comment 13

[André Bargull [:anba]]

Attachment: Bug 1879939: Save output registers. r=jandem!

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

Comment 14

[André Bargull [:anba]]

(In reply to Dianna Smith [:diannaS] from comment #12)

:andré also, can you nominate this for uplift?

Sure, created the uplift request for esr115.

Comment 15

[Phabricator Automation]

Uplift Approval Request

• String changes made/needed: None
• Risk associated with taking this patch: Low risk
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Explanation of risk level: Low risk because it only changes the registers which are saved during a call from JIT to C++.
• User impact if declined: Overwrites return registers on Armv7-A devices which don't support the "sdiv" instruction.
• Steps to reproduce for manual QE testing: None
• Code covered by automated testing: yes
• Is Android affected?: yes

Comment 16

[Ryan VanderMeulen [:RyanVM]]

sec-approval doesn't need to block uplifting to ESR at this point, but we probably should still add it retroactively. Andre, can you please do so?

Comment 17

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/f080bbff0923

Comment 18

[André Bargull [:anba]]

Comment on attachment 9388726 [details]
Bug 1879939: Save output registers. r=jandem!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The patch makes it obvious that return registers aren't saved across a call, because the unfixed code only asserted the registers are saved, whereas the fixed code explicitly ensures the register will get saved. There are only three callers to this code (CacheIRCompiler::emitInt32{Div,Mod}Result and MacroAssembler::loadInt32ToStringWithBase), which may make it easier to create an exploit. After finding out that only MacroAssembler::loadInt32ToStringWithBase calls the faulty code, an attacker needs to write code which holds other data in the return registers, which will then get overwritten after the call to __aeabi_idivmod in MacroAssembler::flexibleDivMod32.

The code is only run on Armv7-A targets which don't support sdiv instruction, which will likely reduce the affected targets somewhat, though.

• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• 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 supported branches
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions, because it only ensures that return registers are saved across a call.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 19

[Tom Ritter [:tjr] (OOTO until 9/2)]

Comment on attachment 9388726 [details]
Bug 1879939: Save output registers. r=jandem!

sec-approved

Comment 20

[Tyson Smith [:tsmith]]

Attachment: advisory.txt

Comment 21

[Pulsebot]

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

Comment 22

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/6c6355062108