1766806 - (CVE-2022-31740) Assertion failure: *def->output() == alloc, at jit/RegisterAllocator.cpp:270

Status: RESOLVED FIXED

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

Description

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

Attachment: stack

new WebAssembly.Module(wasmTextToBinary(`
(module
  (global $global$1
    (mut i32)
    (i32.const 0)
  )
  (func $0
    (param $0 f64)
    (param $1 f32)
    (result f32)
    (f32.copysign
      (local.get $1)
      (f32.const 0)
    )
  )
)
`));

(gdb) bt
#0  js::jit::AllocationIntegrityState::checkIntegrity (this=this@entry=0x7fffffff7800, block=block@entry=0x7ffff5f1cfa0, ins=0x7ffff5f1d158, ins@entry=0x7ffff5f1e020, vreg=vreg@entry=4, alloc=...) at /home/skygentoo/trees/mozilla-central/js/src/jit/RegisterAllocator.cpp:270
#1  0x0000555557be4538 in js::jit::AllocationIntegrityState::check (this=<optimized out>) at /home/skygentoo/trees/mozilla-central/js/src/jit/RegisterAllocator.cpp:207
#2  0x0000555557a9d11a in js::jit::GenerateLIR (mir=mir@entry=0x7fffffff7b50) at /home/skygentoo/trees/mozilla-central/js/src/jit/Ion.cpp:1534
#3  0x0000555557d41453 in js::wasm::IonCompileFunctions (moduleEnv=..., compilerEnv=..., lifo=..., inputs=..., code=<optimized out>, error=error@entry=0x7fffffffb828) at /home/skygentoo/trees/mozilla-central/js/src/wasm/WasmIonCompile.cpp:6944
#4  0x0000555557d0cbc3 in ExecuteCompileTask (task=0x7ffff6ace800, error=0x7fffffffb828) at /home/skygentoo/trees/mozilla-central/js/src/wasm/WasmGenerator.cpp:712
#5  0x0000555557d0d89a in js::wasm::ModuleGenerator::locallyCompileCurrentTask (this=0x7fffffffa2e0) at /home/skygentoo/trees/mozilla-central/js/src/wasm/WasmGenerator.cpp:773
#6  js::wasm::ModuleGenerator::finishFuncDefs (this=0x7fffffffa2e0) at /home/skygentoo/trees/mozilla-central/js/src/wasm/WasmGenerator.cpp:913
#7  0x0000555557cea100 in DecodeCodeSection<js::wasm::Decoder> (env=..., d=..., mg=...) at /home/skygentoo/trees/mozilla-central/js/src/wasm/WasmCompile.cpp:709
#8  0x0000555557ce9de7 in js::wasm::CompileBuffer (args=..., bytecode=..., error=error@entry=0x7fffffffb828, warnings=warnings@entry=0x7fffffffb858, listener=listener@entry=0x0) at /home/skygentoo/trees/mozilla-central/js/src/wasm/WasmCompile.cpp:731
#9  0x0000555557d5a8c9 in js::WasmModuleObject::construct (cx=cx@entry=0x7ffff6a2c200, argc=<optimized out>, vp=<optimized out>) at /home/skygentoo/trees/mozilla-central/js/src/wasm/WasmJS.cpp:1815
#10 0x0000555556c398d0 in CallJSNative (cx=cx@entry=0x7ffff6a2c200, native=native@entry=0x555557d5a730 <js::WasmModuleObject::construct(JSContext*, unsigned int, JS::Value*)>, reason=<optimized out>, reason@entry=js::CallReason::Call, args=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:420
#11 0x0000555556c3dd25 in CallJSNativeConstructor (cx=cx@entry=0x7ffff6a2c200, native=0x555557d5a730 <js::WasmModuleObject::construct(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:436
#12 0x0000555556c2d4db in InternalConstruct (cx=<optimized out>, cx@entry=0x7ffff6a2c200, args=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:633
#13 0x0000555556c22503 in js::ConstructFromStack (cx=0x7ffff6a2c200, args=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:679
#14 Interpret (cx=cx@entry=0x7ffff6a2c200, state=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:3304
#15 0x0000555556c18dcf in js::RunScript (cx=cx@entry=0x7ffff6a2c200, state=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:389
#16 0x0000555556c2e4a4 in js::ExecuteKernel (cx=cx@entry=0x7ffff6a2c200, script=script@entry=..., envChainArg=envChainArg@entry=..., evalInFrame=evalInFrame@entry=..., result=result@entry=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:781
#17 0x0000555556c2e87c in js::Execute (cx=cx@entry=0x7ffff6a2c200, script=..., envChain=..., rval=..., rval@entry=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:813
#18 0x0000555556d93daf in ExecuteScript (cx=cx@entry=0x7ffff6a2c200, envChain=..., script=..., rval=rval@entry=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/CompilationAndEvaluation.cpp:509
#19 0x0000555556d93fd8 in JS_ExecuteScript (cx=cx@entry=0x7ffff6a2c200, scriptArg=scriptArg@entry=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/CompilationAndEvaluation.cpp:533
#20 0x0000555556b5e7a1 in RunFile (cx=cx@entry=0x7ffff6a2c200, filename=0x7fffffffde82 "testcase.js", filename@entry=0x7ffff7864020 "\230$\255\373\344\344\344", <incomplete sequence \344>, file=<optimized out>, file@entry=0x7ffff7864020, compileMethod=compileMethod@entry=CompileUtf8::DontInflate, compileOnly=false) at /home/skygentoo/trees/mozilla-central/js/src/shell/js.cpp:1065
#21 0x0000555556b5ded2 in Process (cx=cx@entry=0x7ffff6a2c200, filename=<optimized out>, forceTTY=false, kind=kind@entry=FileScript) at /home/skygentoo/trees/mozilla-central/js/src/shell/js.cpp:1654
#22 0x0000555556b2443c in ProcessArgs (cx=0x7ffff6a2c200, op=<optimized out>) at /home/skygentoo/trees/mozilla-central/js/src/shell/js.cpp:10921
#23 Shell (cx=0x7ffff6a2c200, op=op@entry=0x7fffffffd6e8) at /home/skygentoo/trees/mozilla-central/js/src/shell/js.cpp:11660
#24 0x0000555556b1d81a in main (argc=<optimized out>, argv=<optimized out>) at /home/skygentoo/trees/mozilla-central/js/src/shell/js.cpp:12702
(gdb)

Run with --fuzzing-safe --no-threads --no-baseline --no-ion testcase.js, compile with AR=ar sh ./configure --enable-simulator=arm64 --enable-debug --with-ccache --enable-gczeal --enable-debug-symbols --disable-bootstrap --disable-tests, tested on m-c rev 79a65e971e59.

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

Comment 1

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

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/62b1090431b0
user:        Yury Delendik
date:        Wed Apr 06 18:42:42 2022 +0000
summary:     Bug 1762899 - Fix {f32,f64}.copysign for Aarch64. r=lth

Yury, is bug 1762899 a likely regressor? Also, apparently this patch was backported to all the releases and even ESR 91...

Comment 2

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

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

Comment 3

[Lars T Hansen [:lth]]

Looking.

Comment 4

[Lars T Hansen [:lth]]

Easy to repro locally in an arm64 simulator debug build, no other special flags needed when compiling or running. The global variable in the TC can be removed.

This is an old register allocator problem: there are hard-to-maintain constraints (see links below) on how defineReuseInput can be combined with multi-argument operations and useRegisterAtStart. I remember looking for this problem when reviewing the regressing patch and not finding what I was looking for, but that's probably what's going on here, because this patch fixes it:

diff --git a/js/src/jit/arm64/Lowering-arm64.cpp b/js/src/jit/arm64/Lowering-arm64.cpp
--- a/js/src/jit/arm64/Lowering-arm64.cpp
+++ b/js/src/jit/arm64/Lowering-arm64.cpp
@@ -983,7 +983,10 @@ void LIRGenerator::visitCopySign(MCopySi
   }
 
   lir->setOperand(0, useRegisterAtStart(lhs));
-  lir->setOperand(1, useRegisterAtStart(rhs));
+  lir->setOperand(1, willHaveDifferentLIRNodes(lhs, rhs)
+                  ? useRegister(rhs)
+                  : useRegisterAtStart(rhs));
+
   // The copySignDouble and copySignFloat32 are optimized for lhs == output.
   // It also prevents rhs == output when lhs != output, avoids clobbering.
   defineReuseInput(lir, ins, 0);

It's sort of a miracle that this doesn't blow up more often.

https://searchfox.org/mozilla-central/source/js/src/jit/shared/Lowering-shared.h#109
https://searchfox.org/mozilla-central/source/js/src/jit/shared/Lowering-shared.h#176

Comment 6

[Yury Delendik (:yury)]

Checking the proposed in comment 4 patch with releases.

Julian, I need help with figuring out effect of just ignoring these MOZ_ASSERT in the check_integrity. Basically, I don't see it is crashing in release. I wonder if worst case is just a wrong result of the f32.copysign related code, and not bad memory access.

Comment 7

[Yury Delendik (:yury)]

Attachment: Bug 1766806 - Avoid checkIntegrity failure in f32.copysign wasm instruction. r?jseward

Comment 8

[Yury Delendik (:yury)]

Comment on attachment 9274321 [details]
Bug 1766806 - Avoid checkIntegrity failure in f32.copysign wasm instruction. r?jseward

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It is not clear if the bug has security side-effect: the triggered assert is ensuring the specific invariant of the internal data structure. (I'm not sure if it even breaks the correctness of the affected operation)
• 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 older supported branches are affected by this flaw?: esr91,release,beta
• If not all supported branches, which bug introduced the flaw?: Bug 1762899
• 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?: Since bug 1762899 was uplift, and patch modifies the uplifted code, it is low risk and affects only one specific operation
• Is Android affected?: Yes

Comment 9

[Tom Ritter [:tjr]]

Comment on attachment 9274321 [details]
Bug 1766806 - Avoid checkIntegrity failure in f32.copysign wasm instruction. r?jseward

Approved to land and if needed, uplift

Comment 10

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

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

Comment 11

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

Avoid checkIntegrity failure in f32.copysign wasm instruction. r=lth
https://hg.mozilla.org/integration/autoland/rev/b8656f201b57d17de2bbcb587cdcdcd23c3facaf
https://hg.mozilla.org/mozilla-central/rev/b8656f201b57

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta and ESR approval when you get a chance.

Comment 13

[Yury Delendik (:yury)]

Comment on attachment 9274321 [details]
Bug 1766806 - Avoid checkIntegrity failure in f32.copysign wasm instruction. r?jseward

Beta/Release Uplift Approval Request

• User impact if declined: (Debug builds) can crash due to assert when executing some wasm code with certain SIMD instructions.
• 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): The changes only affect the code that produces the error, limited to the affected platform, and tests are provided.
• String changes made/needed:
• Is Android affected?: Unknown

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Addressing a defect added by uplifted patch.
• User impact if declined: (Debug builds) can crash due to assert when executing some wasm code with certain SIMD instructions.
• Fix Landed on Version: 102
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The changes only affect the code that produces the error, limited to the affected platform, and tests are provided.

Comment 14

[Daniel Veditz [:dveditz]]

Split bounty with duplicate discovery bug 1766809

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9274321 [details]
Bug 1766806 - Avoid checkIntegrity failure in f32.copysign wasm instruction. r?jseward

Approved for 101.0b6 and 91.10esr.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/801993dc5b98

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/5f01542de9f3

Comment 18

[Tom Ritter [:tjr]]

Attachment: advisory.txt