Closed Bug 1736009 Opened 3 months ago Closed 3 months ago

The `AutoForbidPoolsAndNops` mechanism fails (asserts) on legitimate use cases on arm64


(Core :: JavaScript Engine: JIT, defect)




95 Branch
Tracking Status
firefox95 --- fixed


(Reporter: jseward, Assigned: lth)


(Blocks 3 open bugs)



(2 files)

Attached patch test case patchSplinter Review

AutoForbidPoolsAndNops is an RAII class available in at least the arm and
arm64 macro assemblers. It facilitates emitting small blocks of instructions
with the guarantee that the block won't be broken up for any reason.

Legitimate uses, as exemplified by the attached test patch, have been observed
to lead to the assertion failure:

Assertion failure: hasSpaceForInsts(maxInst, 0),
  at js/src/jit/shared/IonAssemblerBufferWithConstantPools.h:1086

#0 AssemblerBufferWithConstantPools<..>::enterNoPool (this=0xffffffff67f0,
   at js/src/jit/shared/IonAssemblerBufferWithConstantPools.h:1086
#1 MozBaseAssembler::enterNoPool (this=0xffffffff64c0, maxInst=5)
   at js/src/jit/arm64/vixl/MozBaseAssembler-vixl.h:304
#2 AutoForbidPoolsAndNops::AutoForbidPoolsAndNops(this=0xffffffff5420,
                                                  asm_=0xffffffff64c0, maxInst=5)
   at js/src/jit/arm64/Assembler-arm64.h:775

#3 0x0000aaaaabcb9c9c in js::jit::PreCallAssertions (masm=0xffffffff64c0)
   at js/src/jit/arm64/MacroAssembler-arm64.cpp:


  • pull m-c and apply the test patch
  • configure the shell thusly: --enable-debug --enable-optimize="-g -Og"
  • run jit_tests: at least 3 tests should fail:
    ../src/jit-test/ --no-xdr --args="" -f ./dist/bin/js

one of which is tests/bug1681258.js

Marking sec-low for now, because:

  • the assertion has to do with the relative placement of code and in-line data

  • so it's plausible to argue that it might be exploitable

  • the assertion that fails is not a release assert, hence the failure could be
    silently missed in release builds

Why haven't we seen this before? AutoForbidPoolsAndNops is used only
relatively infrequently. The test patch causes it to be used before many but
not all call instructions that SM generates. Because those are common, this
would represent a large increase in how often it is used, compared to
unpatched SM. And even then we get only 3 failures in circa 8700 tests done
by jit_tests.

So I am inclined to think: it's really a bug, but sufficiently obscure that
jit_tests doesn't uncover it. And release builds won't trigger the assertion
but presumably could fail some other, obscure way, as a result.

This looks like a missed OOM condition, code does not check OOM before checking that invariants hold. The OOM will for sure be detected later so there shouldn't be any risk of bad code being executed. And by assumption all our other emitters check for buffer-full, so there should be no chance of a buffer overrun. So I don't think this is s-s.

Since we depend on the state of the buffer after finishPool returns,
make sure finishPool did not OOM. Returning early seemed the cleanest
to me, as it leaves the pool state intact. In contrast, a
MOZ_ASSERT_IF(!this->oom(), ...) would allow the pool state to change.

Assignee: jseward → lhansen

Not s-s, see comment 1. Can we reopen? Thanks.

Flags: needinfo?(dveditz)
Keywords: sec-low
Group: core-security
Flags: needinfo?(dveditz)
Pushed by
Check OOM after finishPool. r=nbp
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 95 Branch
You need to log in before you can comment on or make changes to this bug.