Bug 129768 (CVE-2014-1362) - llint_slow_path_check_has_instance() should not adjust PC before accessing operands
Summary: llint_slow_path_check_has_instance() should not adjust PC before accessing op...
Status: RESOLVED FIXED
Alias: CVE-2014-1362
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-03-05 18:06 PST by Mark Lam
Modified: 2014-06-18 17:26 PDT (History)
8 users (show)

See Also:

Attachments
the patch. (5.75 KB, patch)
2014-03-05 18:27 PST, Mark Lam 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2014-03-05 18:06:36 PST 
When evaluating "a instanceof b" where b is an object that ImplementsHasInstance and OverridesHasInstance (e.g. a bound function), the LLINT will take the slow path llint_slow_path_check_has_instance(), and execute a code path that does the following:
1. Adjusts the byte code PC to the jump target PC.
2. For the purpose of storing the result, get the result registerIndex from the 1st operand using the PC as if the PC is still pointing to op_check_has_instance bytecode.

The result is that whatever value resides after where the jump target PC is will be used as a result register value.  Depending on what that value is, the result can be:
1. the code coincidently works correctly
2. memory corruption
3. crashes

The fix is to only adjust the byte code PC after we have stored the result.

ref: <rdar://problem/15394423>
Comment 1 Mark Lam 2014-03-05 18:27:49 PST 
Created attachment 225934 [details]
the patch.
Comment 2 Mark Hahnenberg 2014-03-05 18:27:59 PST 
You should audit the other uses of LLINT_RETURN to make sure we're not doing this in any other places.
Comment 3 Mark Lam 2014-03-05 18:30:02 PST 
(In reply to comment #2)
> You should audit the other uses of LLINT_RETURN to make sure we're not doing this in any other places.

Already did.  I audited adjustments made to the pc and made sure that they did not precede a LLINT_RETURN().
Comment 4 WebKit Commit Bot 2014-03-05 18:30:09 PST 
Attachment 225934 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:125:  __r_returnValue is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Mark Hahnenberg 2014-03-05 18:30:28 PST 
(In reply to comment #3)
> (In reply to comment #2)
> > You should audit the other uses of LLINT_RETURN to make sure we're not doing this in any other places.
> 
> Already did.  I audited adjustments made to the pc and made sure that they did not precede a LLINT_RETURN().

Cool :-)
Comment 6 Mark Hahnenberg 2014-03-05 18:32:43 PST 
Comment on attachment 225934 [details]
the patch.

r=me
Comment 7 WebKit Commit Bot 2014-03-05 19:11:45 PST 
Comment on attachment 225934 [details]
the patch.

Clearing flags on attachment: 225934

Committed r165162: <http://trac.webkit.org/changeset/165162>
Comment 8 WebKit Commit Bot 2014-03-05 19:11:48 PST 
All reviewed patches have been landed.  Closing bug.