Crash at null with EnterBaseline on the stack involving enableSPSProfilingAssertions

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: gkw, Assigned: djvj)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
x86_64
Mac OS X
assertion, regression, sec-moderate, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox-esr17 wontfix, b2g18 wontfix)

Details

(Whiteboard: [fuzzblocker])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Created attachment 741535 [details]
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).
(Reporter)

Comment 1

5 years ago
jandem, is this a Baseline issue?
Flags: needinfo?(jdemooij)
(Reporter)

Updated

5 years ago
Blocks: 865458
Kannan, since you did the SPS integration can you take a look? Thanks!
Flags: needinfo?(jdemooij) → needinfo?(kvijayan)
(Assignee)

Updated

5 years ago
Assignee: general → kvijayan
(Assignee)

Comment 3

5 years ago
Will take a look at it later today or tomorrow.
Flags: needinfo?(kvijayan)
(Reporter)

Comment 4

5 years ago
djvj mentions over IRC that this should be s-s.
Group: core-security
(Assignee)

Comment 5

5 years ago
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).
Keywords: sec-moderate
(Assignee)

Comment 6

5 years ago
Created attachment 744778 [details] [diff] [review]
Fix bug, and other SPS issues

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+
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e0956c0dfe4
(Assignee)

Comment 9

5 years ago
(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 *').
(Assignee)

Updated

5 years ago
Flags: in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/6e0956c0dfe4
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
status-firefox-esr17: --- → wontfix
Depends on: 870051
status-b2g18: --- → wontfix
Group: core-security
You need to log in before you can comment on or make changes to this bug.