Crash [@ ReplaceLane<js::Float32x4>] with SIMD

VERIFIED FIXED in Firefox 52

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
2 years ago
10 months ago

People

(Reporter: decoder, Assigned: bbouvier)

Tracking

(Blocks: 1 bug, 6 keywords)

Trunk
mozilla52
x86
Linux
crash, csectype-uaf, jsbugmon, regression, sec-critical, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 disabled, firefox50 disabled, firefox51 disabled, firefox52 verified)

Details

(Whiteboard: [jsbugmon:update], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
The following testcase crashes on mozilla-central revision eaf5eb6f8fa0 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu, run with --fuzzing-safe --thread-count=2 --disable-oom-functions --disable-oom-functions --ion-check-range-analysis --baseline-eager):

gczeal(14,2);
try {
Array.prototype.some.Date(x, function() {});
} catch(exc1) {}
function assertEqX4(v, arr) {
    function indexes(n) {}
};
var Float32x4 = SIMD.Float32x4;
function replaceLaneN(laneIndex, arr, value) {
    var copy = arr.slice();
    copy[laneIndex] = value;
    return copy;
}
var replaceLane0 = replaceLaneN.bind(null, 0);
function testReplaceLane(vec, scalar, simdFunc, func) {
    var varr = simdToArray(vec);
    var observed = simdToArray(simdFunc(vec, scalar));
    var expected = func(varr, scalar);
    for (var i = 0; i < observed.length; i++)
        assertEq(observed[i], expected[i]);
}
function test() {
  function testType(type, inputs) {
      for (var [vec, s] of inputs) {
          testReplaceLane(vec, s, (x,y) => SIMD[type].replaceLane(x, 0, y), replaceLane0);
      }
  }
  var good = {valueOf: () => 42};
  var Float32x4inputs = [
      [Float32x4(1, 2, 3, 4), 5],
      [Float32x4(1.87, 2.08, 3.84, 4.17), Math.fround(13.37)],
      [Float32x4(NaN, -0, Infinity, -Infinity), 0]
  ];
  var v = Float32x4inputs[1][0];
  assertEqX4(Float32x4.replaceLane(v, 0, good), replaceLane0(simdToArray(v), good | 0));
}
test();



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x088a5417 in ReplaceLane<js::Float32x4> (vp=0xffffbb98, argc=3, cx=0xf7953000) at js/src/builtin/SIMD.cpp:975
#0  0x088a5417 in ReplaceLane<js::Float32x4> (vp=0xffffbb98, argc=3, cx=0xf7953000) at js/src/builtin/SIMD.cpp:975
#1  js::simd_float32x4_replaceLane (cx=0xf7953000, argc=3, vp=0xffffbb98) at js/src/builtin/SIMD.cpp:1406
#2  0x08716bcb in js::CallJSNative (cx=0xf7953000, native=0x88a52c0 <js::simd_float32x4_replaceLane(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
#3  0x0870dee6 in js::InternalCallOrConstruct (cx=0xf7953000, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:458
#4  0x0870e21d in InternalCall (cx=cx@entry=0xf7953000, args=...) at js/src/vm/Interpreter.cpp:503
#5  0x0870e36f in js::CallFromStack (cx=0xf7953000, args=...) at js/src/vm/Interpreter.cpp:509
#6  0x089f5f08 in js::jit::DoCallFallback (cx=0xf7953000, frame=0xffffbc28, stub_=0xf44ff558, argc=3, vp=0xffffbb98, res=...) at js/src/jit/BaselineIC.cpp:5998
#7  0xf7be367c in ?? ()
[...]
#31 main (argc=8, argv=0xffffcd34, envp=0xffffcd58) at js/src/shell/js.cpp:7663
eax	0x0	0
ebx	0x0	0
ecx	0x0	0
edx	0xffffb778	-18568
esi	0xf7953000	-141217792
edi	0xf4579068	-195587992
ebp	0xffffb858	4294948952
esp	0xffffb800	4294948864
eip	0x88a5417 <js::simd_float32x4_replaceLane(JSContext*, unsigned int, JS::Value*)+343>
=> 0x88a5417 <js::simd_float32x4_replaceLane(JSContext*, unsigned int, JS::Value*)+343>:	flds   0x4(%edi)
   0x88a541a <js::simd_float32x4_replaceLane(JSContext*, unsigned int, JS::Value*)+346>:	fstps  -0x28(%ebp)


Marking s-s due to crash with weird memory address. SIMD is nightly only but enabled by default there.
Dan, could you look at this? Thanks. I'm not sure how to rate it.
status-firefox51: affected → disabled
status-firefox52: --- → affected
status-firefox-esr45: --- → disabled
Flags: needinfo?(sunfish)
status-firefox50: --- → disabled
The ReplaceLane code has a raw pointer to TypedObjectMemory. It appears that GC moves the memory. The pointer is now dangling. The code then reads from the dangling pointer. To be on the safe side, I'd call this sec-critical. Confirmed that it's in SIMD code which is nightly-only.
Flags: needinfo?(sunfish)
Keywords: csectype-uaf, sec-critical
ReplaceLane is grabbing a raw pointer to the SIMD object data buffer, |vec|, and holding it live over a call to a Cast function, which does a GC. At an initial glance, it might be enough to just move the initialization of |vec| down below the Cast call.

Updated

2 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]

Comment 4

2 years ago
JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160913050810" and the hash "99ae6aab3d0b1ebc2be01e4dd95bbcc04c7422d5".
The "bad" changeset has the timestamp "20160913051608" and the hash "d1bf9267ba7da182771c43aec042f0f5f579de93".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=99ae6aab3d0b1ebc2be01e4dd95bbcc04c7422d5&tochange=d1bf9267ba7da182771c43aec042f0f5f579de93
This sounds like the same issue as bug 1296640.
See Also: → bug 1296640
I agree, it looks like another instance of the kind of bug in 1296640.
Setting needinfo? from Benjamin, who is working on bug 1296640. Is this a dupe, shall we add this testcase to the patch?
Flags: needinfo?(bbouvier)
(Assignee)

Comment 8

2 years ago
Oh boy, this is another different instance of the same family of issues.
Flags: needinfo?(bbouvier)
(Assignee)

Comment 9

2 years ago
Created attachment 8796532 [details] [diff] [review]
fix.patch

Same issue, different instance :/
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Attachment #8796532 - Flags: review?(jcoppeard)
(Assignee)

Comment 10

2 years ago
Comment on attachment 8796532 [details] [diff] [review]
fix.patch

[Security approval request comment]
Same as bug 1296640 comment 14.
Attachment #8796532 - Flags: sec-approval?

Updated

2 years ago
Attachment #8796532 - Flags: review?(jcoppeard) → review+
(Assignee)

Comment 11

2 years ago
I've done a full manual audit of all the uses of TypedObjectMemory in builtin/SIMD.cpp and I haven't found anything else. But humans are fallible, so I've added the magic gczeal() invokation at the head of each tests under jit-tests/tests/SIMD/, and the tests all pass without more segfaults, so I think we're good.
(Assignee)

Comment 12

2 years ago
Comment on attachment 8796532 [details] [diff] [review]
fix.patch

Cancelling sec-approval since it's SIMD.js (nightly only), per https://wiki.mozilla.org/Security/Bug_Approval_Process
Attachment #8796532 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/f713114b8c8d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Updated

2 years ago
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified

Comment 15

2 years ago
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
Bug 1307296 automates checking for most problems like this (and would have caught this one; I checked.)
Blocks: 1307296

Updated

10 months ago
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.