Bug 184280 – Don't do index masking or poisoning for DirectArguments

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 337126 [details]
the patch

[Filip Pizlo]

Comment on attachment 337126 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337126&action=review

> Source/JavaScriptCore/ChangeLog:-2936
> -2018-03-09  Filip Pizlo  <[e-mail]>
> -
> -        Split DirectArguments into JSValueOOB and JSValueStrict parts
> -        https://bugs.webkit.org/show_bug.cgi?id=183458
> -
> -        Reviewed by Yusuke Suzuki.
> -        
> -        Our Spectre plan for JSValue objects is to allow inline JSValue stores and loads guarded by
> -        unmitigated structure checks. This works because objects reachable from JSValues (i.e. JSValue
> -        objects, like String, Symbol, and any descendant of JSObject) will only contain fields that it's OK
> -        to read and write within a Spectre mitigation window. Writes are important, because within the
> -        window, a write could appear to be made speculatively and rolled out later. This means that:
> -        
> -        - JSValue objects cannot have lengths, masks, or anything else inline.
> -        
> -        - JSValue objects cannot have an inline type that is used as part of a Spectre mitigation for a type
> -          check, unless that type is in the form of a poison key.
> -        
> -        This means that the dynamic poisoning that I previously landed for DirectArguments is wrong. It also
> -        means that it's wrong for DirectArguments to have an inline length.
> -        
> -        This changes DirectArguments to use poisoning according to the universal formula:
> -        
> -        - The random accessed portions are out-of-line, pointed to by a poisoned pointer.
> -        
> -        - No inline length.
> -        
> -        Surprisingly, this is perf-neutral. It's probably perf-neutral because our compiler optimizations
> -        amortize whatever cost there was.
> -

I'll revert this.

[Filip Pizlo]

Created attachment 337128 [details]
the patch

[EWS Watchlist]

Attachment 337128 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/DirectArguments.h:123:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 337132 [details]
the patch

[EWS Watchlist]

Attachment 337132 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/DirectArguments.h:123:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 337132 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337132&action=review

rs=me

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:15570
> +    LValue dynamicPoison(LValue value, LValue poison)

Do we still need these?

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/230266/webkit

[Radar WebKit Bug Importer]

<rdar://problem/39180055>

[Filip Pizlo]

(In reply to Saam Barati from comment #7)
> Comment on attachment 337132 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337132&action=review
> 
> rs=me
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:15570
> > +    LValue dynamicPoison(LValue value, LValue poison)
> 
> Do we still need these?

Probably not.  We might still use them for scoped arguments poisoning or something.  I'll kill them once I'm done killing other things.