1887332 - (CVE-2024-4775) Null-deref at js::gc::HeaderWord::getAtomic() gc/Cell.h:100:12 with ReadGeckoProfilingStack

Status: RESOLVED FIXED

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

Description

[lukas.bernhard]

Attachment: crash.js

Steps to reproduce:

On git commit f63ca2952da98e0817bdae0ddf1314281a497106 the attached sample crashes in the js-shell when accessing NULL, invoked as obj-x86_64-pc-linux-gnu/dist/bin/js --fast-warmup --fuzzing-safe crash.js
The issue seems to be related to GC and profiling; flagging as s-s as I'm not sure whether profiling is an actual prerequisite for the bug to manifest.

#0 0x5a1cefb4cea5 in js::gc::HeaderWord::getAtomic() const js/src/gc/Cell.h:100:12
#1 0x5a1cefb4cea5 in js::gc::HeaderWord::flags() const js/src/gc/Cell.h:116:36
#2 0x5a1cefb4cea5 in js::gc::HeaderWord::isForwarded() const js/src/gc/Cell.h:117:37
#3 0x5a1cefb4cea5 in js::gc::Cell::isForwarded() const js/src/gc/Cell.h:153:45
#4 0x5a1cefb4cea5 in bool js::gc::IsForwarded<js::Shape>(js::Shape const*) js/src/gc/Marking-inl.h:101:13
#5 0x5a1cefb4cea5 in js::Shape* js::gc::MaybeForwarded<js::Shape*>(js::Shape*) js/src/gc/Marking-inl.h:118:7
#6 0x5a1cefdcfa65 in js::NativeObject::numFixedSlotsMaybeForwarded() const js/src/vm/NativeObject-inl.h:55:10
#7 0x5a1cefdcfa65 in js::NativeObject::slotIsFixed(unsigned int) const js/src/vm/NativeObject.cpp:278:17
#8 0x5a1cef8f73cf in js::NativeObject::getFixedSlot(unsigned int) const js/src/vm/NativeObject.h:1340:5
#9 0x5a1cef8f73cf in JSFunction::flagsAndArgCountRaw() const js/src/vm/JSFunction.h:168:12
#10 0x5a1cef8f73cf in JSFunction::flags() const js/src/vm/JSFunction.h:178:35
#11 0x5a1cef8f73cf in JSFunction::isInterpreted() const js/src/vm/JSFunction.h:188:39
#12 0x5a1cef8f73cf in JSFunction::isIncomplete() const js/src/vm/JSFunction.h:528:12
#13 0x5a1cef8f722d in JSFunction::hasBytecode() const js/src/vm/JSFunction.h:226:5
#14 0x5a1cf0e94cdf in JSFunction::nonLazyScript() const js/src/vm/JSFunction.h:532:5
#15 0x5a1cf0e94cdf in js::jit::ScriptFromCalleeToken(void*) js/src/jit/ScriptFromCalleeToken.h:24:44
#16 0x5a1cf0e94cdf in js::jit::JSJitProfilingFrameIterator::frameScript() const js/src/jit/JSJitFrameIter-inl.h:41:10
#17 0x5a1cf0e945e7 in js::jit::JSJitProfilingFrameIterator::tryInitWithPC(void*) js/src/jit/JSJitFrameIter.cpp:579:22
#18 0x5a1cf0e940ce in js::jit::JSJitProfilingFrameIterator::JSJitProfilingFrameIterator(JSContext*, void*, void*) js/src/jit/JSJitFrameIter.cpp:524:7
#19 0x5a1ceffc1ccc in JS::ProfilingFrameIterator::iteratorConstruct(JS::ProfilingFrameIterator::RegisterState const&) js/src/vm/Stack.cpp:572:19
#20 0x5a1ceffc1b05 in JS::ProfilingFrameIterator::ProfilingFrameIterator(JSContext*, JS::ProfilingFrameIterator::RegisterState const&, mozilla::Maybe<unsigned long> const&) js/src/vm/Stack.cpp:488:3
#21 0x5a1cf016f16c in ReadGeckoProfilingStack(JSContext*, unsigned int, JS::Value*) js/src/builtin/TestingFunctions.cpp:4595:35
#22 0x5a1cef996fe6 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) js/src/vm/Interpreter.cpp:479:13
#23 0x5a1cef996201 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) js/src/vm/Interpreter.cpp:573:12
#24 0x5a1cef9ac4d0 in js::CallFromStack(JSContext*, JS::CallArgs const&, js::CallReason) js/src/vm/Interpreter.cpp:645:10
#25 0x5a1cef9ac4d0 in js::Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:3060:16
#26 0x5a1cef995449 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:451:13
#27 0x5a1cef99612e in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) js/src/vm/Interpreter.cpp:605:13
#28 0x5a1cef99813d in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) js/src/vm/Interpreter.cpp:672:8
#29 0x5a1cf08fdc37 in js::jit::InvokeFunction(JSContext*, JS::Handle<JSObject*>, bool, bool, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/jit/VMFunctions.cpp:545:10
#30 0x5a1cf08fe7b8 in js::jit::InvokeFromInterpreterStub(JSContext*, js::jit::InterpreterStubExitFrameLayout*) js/src/jit/VMFunctions.cpp:569:8
#31 0x26c3f1d3ff1f  ([anon:js-executable-memory]+0x2f1f)

Comment 1

[Andrew McCreight [:mccr8]]

The GC stuff is more like it is poking at some GC data structures. The test case involves WebAssembly, so I'll move it over there. I do see frame iter stuff in the stack and a null deref, so maybe it is like that other one?

Comment 2

[Ryan Hunt [:rhunt]]

Attachment: poc-profile.js

Here's a reduced test case.

--fast-warmup is required. Wasm JIT exits are required to be enabled (wasm to JS JIT, aka. GenerateImportJitExit), but Wasm JIT entries are not required to be enabled. There seems to be something going on with the enableGeckoProfiling call happening at the bottom of the call stack. If I hoist that out to not be called every iteration that resolves the issue.

The JSJitProfilingFrameIterator is getting a fp_ from cx->lastProfilingFrame and it seems like it might be garbage, which is causing issues when the callee value is inspected. Reading the descriptor off of it, it looks like FrameType::Ion, with a random argc value, which seems incorrect. I see that enableGeckoProfiling is responsible to set lastProfilingFrame, but don't see anything obviously wrong with it.

Comment 3

[Ryan Hunt [:rhunt]]

Setting this S3 tentatively, as it seems likely this requires some specific invocation of the Gecko profiling code, which is not content exposed.

Comment 4

[Yury Delendik (:yury)]

The JSJitProfilingFrameIterator is getting a fp_ from cx->lastProfilingFrame and it seems like it might be garbage,

I'm finding that as well. The cx->lastProfilingFrame is not updated between enableGeckoProfiling(); and readGeckoProfilingStack();. If enableGeckoProfiling(); is done outside the function, the lastProfilingFrame is staying null, which is can indicate that it was not updated.

Comment 5

[Daniel Veditz [:dveditz]]

A null deref doesn't sound security sensitive, but a "garbage" frame does! Reducing to "sec-low" because it's so far more of a latent issue for users than one that could be reached.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1887332 - Improve handling of WasmToJSJit frames in JSJitProfilingFrameIterator. r?rhunt!

The iterator didn't mark itself done() when reaching a WasmToJSJit entry frame and
this confused the code in GetTopProfilingJitFrame.

We now store the Wasm caller FP separately and mark the iterator as done.
This is less error-prone and is also more consistent with other JS JIT and Wasm
frame iterators.

Comment 7

[Jan de Mooij [:jandem]]

To trigger this, the profiler has to be enabled when there are Wasm frames on the stack.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/098eed68d43b
Improve handling of WasmToJSJit frames in JSJitProfilingFrameIterator. r=rhunt

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/098eed68d43b

Comment 10

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

:jandem this grafts cleanly to esr115.
Could you please add an esr115 uplift request? Assuming we want to backport there too?

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Donal Meehan [:dmeehan] from comment #10)

:jandem this grafts cleanly to esr115.
Could you please add an esr115 uplift request? Assuming we want to backport there too?

This doesn't need to be backported to ESR115.

Comment 12

[Jesse Schwartzentruber (:truber)]

Attachment: advisory.txt

Comment 13

[Daniel Veditz [:dveditz]]

Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons