Stack out-of-bounds read [@ js::ion::IonFrameIterator::prevType]

VERIFIED FIXED in Firefox 24

Status

()

defect
--
critical
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: decoder, Assigned: h4writer)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
mozilla25
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox23 unaffected, firefox24+ fixed, firefox25+ fixed, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [asan])

Attachments

(2 attachments, 1 obsolete attachment)

The following testcase shows Stack out-of-bounds read on mozilla-central revision 76820c6dff7b (run with --ion-eager):


function coerceForeign(stdlib, foreign) {
    "use asm";
    var g = foreign.g;
    var h = foreign.h;
    function f() {
        +g(0);
        +g(1);
        +g(2);
        +h(2);
    }
    return f;
}
var t = coerceForeign(undefined, {
  g: function(a) {
    if (a == (/\u00ea/ ))
      var blaat = new blaat();
  },
  h: function(b) function i() {  }    
})
t();
Here is the full trace from ASan:


==31307== ERROR: AddressSanitizer: stack-buffer-overflow on address 0xff816a08 at pc 0x8aefd23 bp 0xff814938 sp 0xff814930
READ of size 4 at 0xff816a08 thread T0
    #0 0x8aefd22 in js::ion::IonFrameIterator::prevType() const js/src/ion/shared/IonFrames-x86-shared.h:31
    #1 0x85b0a7d in JSContext::currentScript(unsigned char**, JSContext::MaybeAllowCrossCompartment) const js/src/jscntxtinlines.h:579
    #2 0x85d4d89 in CallResolveOp(JSContext*, JS::Handle<JSObject*>, JS::Handle<int>, unsigned int, JS::MutableHandle<JSObject*>, JS::MutableHandle<js::Shape*>, bool*) js/src/jsobj.cpp:3538
    #3 0x85e7cfd in JSObject::getGeneric(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<int>, JS::MutableHandle<JS::Value>) js/src/jsobjinlines.h:134
    #4 0x85e6ee2 in js::DefaultValue(JSContext*, JS::Handle<JSObject*>, JSType, JS::MutableHandle<JS::Value>) js/src/jsobj.cpp:4673
    #5 0x858f3dd in JSObject::defaultValue(JSContext*, JS::Handle<JSObject*>, JSType, JS::MutableHandle<JS::Value>) js/src/jsobjinlines.h:37
    #6 0x880772a in JS::ToNumber(JSContext*, JS::Value const&, double*) js/src/jsapi.h:1515
    #7 0xf727522d in
Address 0xff816a08 is located at offset 328 in frame <InvokeFromAsmJS_ToNumber(JSContext*, js::AsmJSModule::ExitDatum*, int, JS::Value*)> of T0's stack:
  This frame has 6 object(s):
    [32, 40) 'agg.tmp.i21'
    [96, 104) 'agg.tmp.i'
    [160, 168) 'fval'
    [224, 232) 'rval'
    [288, 296) 'ref.tmp'
    [352, 360) 'dbl'
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
Shadow bytes around the buggy address:
  0x3ff02cf0: f2 f2 f2 f2 04 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4
  0x3ff02d00: f3 f3 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
  0x3ff02d10: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00 f4 f4 f4
  0x3ff02d20: f2 f2 f2 f2 00 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4
  0x3ff02d30: f2 f2 f2 f2 00 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4
=>0x3ff02d40: f2[f2]f2 f2 00 f4 f4 f4 f3 f3 f3 f3 00 00 00 00
  0x3ff02d50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x3ff02d60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x3ff02d70: f1 f1 f1 f1 00 f4 f4 f4 f2 f2 f2 f2 00 00 00 00
  0x3ff02d80: 00 00 00 00 00 00 f4 f4 f2 f2 f2 f2 00 f4 f4 f4
  0x3ff02d90: f2 f2 f2 f2 00 00 00 f4 f2 f2 f2 f2 00 00 00 f4
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:     fa
  Heap righ redzone:     fb
  Freed Heap region:     fd
  Stack left redzone:    f1
  Stack mid redzone:     f2
  Stack right redzone:   f3
  Stack partial redzone: f4
  Stack after return:    f5
  Stack use after scope: f8
  Global redzone:        f9
  Global init order:     f6
  Poisoned by user:      f7
  ASan internal:         fe



This cannot be detected with Valgrind. Marking sec-high based on invalid read on the stack. If this is harmless, feel free to downgrade.
Whiteboard: [asan]
This also looks like one of these empty-IonActivation issues.
Assignee: general → hv1989
Posted patch Possible patch (obsolete) — Splinter Review
This fixes the problem for me. But I'm not 100% sure about the fix. Gonna ask jandem tomorrow to explain ionTop better.
Posted patch PatchSplinter Review
I think this is the better patch.
setActive(false), didn't reset the ionTop to prevIonTop. Just like the deconstructor of JitActivation would do.
Attachment #766908 - Attachment is obsolete: true
Attachment #767057 - Flags: review?(jdemooij)
Comment on attachment 767057 [details] [diff] [review]
Patch

Review of attachment 767057 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Stack.cpp
@@ +1303,5 @@
>  }
>  #endif
>  
> +void
> +Activation::setActive(bool active) {

Nit: { on its own line

@@ +1322,5 @@
>      prevIonJSContext_(cx->mainThread().ionJSContext),
>      firstFrameIsConstructing_(firstFrameIsConstructing)
>  {
> +    // Unsupported to change context for inactive activation.
> +    JS_ASSERT_IF(!active, cx->mainThread().ionJSContext == cx);

This assert will fail if there's no other JitActivation on the stack. I think you can just remove it?
Attachment #767057 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/ec1974330bfc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment on attachment 767057 [details] [diff] [review]
Patch

[Approval Request Comment]

Bug caused by (feature/regressing bug #): Bug 860838

User impact if declined: High chance of asm.js code not working properly and crashing.

Testing completed (on m-c, etc.): m-c for a day, m-i for 2 days

Risk to taking this patch (and alternatives if risky): Very low risk.

String or IDL/UUID changes made by this patch: /
Attachment #767057 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 886282
Attachment #767057 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
This is Firefox 23 unaffected based on Bug 860838 checkin date of 6/13.
Group: core-security
You need to log in before you can comment on or make changes to this bug.