Closed Bug 865471 Opened 7 years ago Closed 7 years ago

Crash at null with EnterBaseline on the stack involving enableSPSProfilingAssertions

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
firefox-esr17 --- wontfix
b2g18 --- wontfix

People

(Reporter: gkw, Assigned: djvj)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [fuzzblocker])

Attachments

(2 files)

Attached file stack
var x = [];
for (var y = 0; y < 2; ++y) {
    x.push();
}

crash js debug shell on m-c changeset 8b1a7228674a with --no-jm --no-ti --ion-eager "--execute=enableSPSProfilingAssertions(true)" at probably null.

Tested on a threadsafe deterministic 64-bit debug build on Mac 10.8.

This blocks fuzzing with enableSPSProfilingAssertions(true).
jandem, is this a Baseline issue?
Flags: needinfo?(jdemooij)
Blocks: 865458
Kannan, since you did the SPS integration can you take a look? Thanks!
Flags: needinfo?(jdemooij) → needinfo?(kvijayan)
Assignee: general → kvijayan
Will take a look at it later today or tomorrow.
Flags: needinfo?(kvijayan)
djvj mentions over IRC that this should be s-s.
Group: core-security
Yeah, the problem is that baseline can't treat various fields in spsProfiler as constant, which |MacroAssembler::spsProfileEntryAddress| does.

Ion can do it because those values will stay constant for every spsProfiler session, and Ion will invalidate its jitcode at the session boundaries.

However, baseline scripts don't invalidate at session boundaries, and so the field values can change at runtime after the baseline jitcode has been generated.

Asked to mark as s-s because it suggests that were the addresses to change in the right way (e.g. baseline gets compiled with SPS turned on, then SPS is turned off and on again, changing buffer pointers), then it allows attacker to put controlled bytes into memory (because the bytecode pc is written into the buffer, and the bytecode pc can be controlled by constructing JS source in the right way).
Sorry to stack more r?s on you jandem, but I think you're best equipped to review this one.

The fix for this particular bug is to make "safe" variants of the sps MacroAssembler methods that don't hardcode field values of SPSProfiler.

Additionally, while testing the patch, I ran jit-tests with the SPS enabling prologue, and found a bunch other issues in integration of SPS with the debugger.

Fixing those mainly required fixing up exception handling and debugger epilogue handling to deal with SPS frames appropriately.  Those changes are also rolled into this patch.
Attachment #744778 - Flags: review?(jdemooij)
Comment on attachment 744778 [details] [diff] [review]
Fix bug, and other SPS issues

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

Nice finds!

::: js/src/ion/IonFrames.cpp
@@ +496,5 @@
>              // Unwind profiler pseudo-stack
>              RawScript script = iter.script();
> +            Probes::exitScript(cx, script, script->function(), iter.baselineFrame());
> +            // After this point, any pushed SPS frame would have been popped if it needed
> +            // to be.  Unset the flag here so that any if we call DebugEpilogue below,

Nit: s/any//

::: js/src/ion/IonMacroAssembler.h
@@ +742,5 @@
>          lshiftPtr(Imm32(2 + (sizeof(void*) == 4 ? 2 : 3)), temp);
>          addPtr(ImmWord(p->stack()), temp);
>      }
>  
> +    void spsProfileEntryAddressSafe(SPSProfiler *p, int offset, Register temp,

Nit: would be good to have a short comment here explaining why we need the Safe version.

@@ +834,5 @@
>          movePtr(ImmWord(p->sizePointer()), temp);
>          add32(Imm32(-1), Address(temp, 0));
>      }
>  
> +    void spsPopFrameSafe(SPSProfiler *p, Register temp) {

Ditto.

::: js/src/jsprobes.h
@@ +230,5 @@
>  }
>  
> +inline bool
> +Probes::exitScript(JSContext *cx, RawScript script, RawFunction maybeFun,
> +                   StackFrame *fp)

You can remove this method, AbstractFramePtr has an (implicit) constructor that takes a StackFrame *.
Attachment #744778 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #7)
> You can remove this method, AbstractFramePtr has an (implicit) constructor
> that takes a StackFrame *.

Didn't address this comment in the patch.  The exitScript method is called with NULL, which makes auto-construction of AbstractFramePtr ambiguous (since it can be built from 'StackFrame *' or 'BaselineFrame *').
Flags: in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/6e0956c0dfe4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 870051
Group: core-security
You need to log in before you can comment on or make changes to this bug.