Bug 182086 - DirectArguments should protect itself using dynamic poisoning and precise index masking
Summary: DirectArguments should protect itself using dynamic poisoning and precise ind...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-24 23:48 PST by Filip Pizlo
Modified: 2018-01-25 17:37 PST (History)
11 users (show)

See Also:


Attachments
maybe the patch (18.49 KB, patch)
2018-01-24 23:54 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (20.66 KB, patch)
2018-01-25 14:44 PST, Filip Pizlo
saam: review+
Details | Formatted Diff | Diff
patch for landing (20.19 KB, patch)
2018-01-25 15:46 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2018-01-24 23:48:09 PST
Patch forthcoming.
Comment 1 Filip Pizlo 2018-01-24 23:54:32 PST
Created attachment 332233 [details]
maybe the patch
Comment 2 Filip Pizlo 2018-01-25 14:44:32 PST
Created attachment 332319 [details]
the patch
Comment 3 Saam Barati 2018-01-25 15:25:10 PST
Comment on attachment 332319 [details]
the patch

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

r=me w/ some naming nits and other tidbits

> Source/JavaScriptCore/runtime/DirectArguments.h:122
> +        return *preciseIndexMaskPtr(offset.offset(), std::max(m_length, m_minCapacity), dynamicPoison(type(), DirectArgumentsType, ptr));

could this max() be spectre trained in a way that hurts us?

> Source/WTF/wtf/MathExtras.h:492
> +inline unsigned preciseIndexMaskShiftForSize(unsigned size)
> +{
> +    return size * 8 - 1;
> +}
> +
> +template<typename T>
> +unsigned preciseIndexMaskShift()
> +{
> +    return preciseIndexMaskShiftForSize(sizeof(T));
> +}

constexpr for both of these?

> Source/WTF/wtf/MathExtras.h:497
> +    asm("" : "+r"(pointer));

asm volatile?

> Source/WTF/wtf/MathExtras.h:516
> +        (static_cast<uintptr_t>(opaque(actual) ^ expected) << 40));

Is it worth giving this 40 a name since it's hard coded inside JSC in a few places too?
Comment 4 Filip Pizlo 2018-01-25 15:35:02 PST
(In reply to Saam Barati from comment #3)
> Comment on attachment 332319 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332319&action=review
> 
> r=me w/ some naming nits and other tidbits
> 
> > Source/JavaScriptCore/runtime/DirectArguments.h:122
> > +        return *preciseIndexMaskPtr(offset.offset(), std::max(m_length, m_minCapacity), dynamicPoison(type(), DirectArgumentsType, ptr));
> 
> could this max() be spectre trained in a way that hurts us?

No.  At worst, you'll get a smaller limit.

> 
> > Source/WTF/wtf/MathExtras.h:492
> > +inline unsigned preciseIndexMaskShiftForSize(unsigned size)
> > +{
> > +    return size * 8 - 1;
> > +}
> > +
> > +template<typename T>
> > +unsigned preciseIndexMaskShift()
> > +{
> > +    return preciseIndexMaskShiftForSize(sizeof(T));
> > +}
> 
> constexpr for both of these?

I'll try that.

> 
> > Source/WTF/wtf/MathExtras.h:497
> > +    asm("" : "+r"(pointer));
> 
> asm volatile?

I don't need it to be volatile.

> 
> > Source/WTF/wtf/MathExtras.h:516
> > +        (static_cast<uintptr_t>(opaque(actual) ^ expected) << 40));
> 
> Is it worth giving this 40 a name since it's hard coded inside JSC in a few
> places too?

I'll do that.
Comment 5 Filip Pizlo 2018-01-25 15:46:15 PST
Created attachment 332327 [details]
patch for landing
Comment 6 Radar WebKit Bug Importer 2018-01-25 16:08:23 PST
<rdar://problem/36887421>
Comment 7 Filip Pizlo 2018-01-25 16:08:32 PST
Landed in https://trac.webkit.org/changeset/227643/webkit
Comment 8 Mark Lam 2018-01-25 16:30:44 PST
Comment on attachment 332327 [details]
patch for landing

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

> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1049
> +    UNUSED_PARAM(actualValue);

This is a typo, and as a result, you're breaking the 32-bit builds.
Comment 9 Mark Lam 2018-01-25 16:42:39 PST
Comment on attachment 332327 [details]
patch for landing

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

>> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1049
>> +    UNUSED_PARAM(actualValue);
> 
> This is a typo, and as a result, you're breaking the 32-bit builds.

Fixed 32-bit build in r227644: <https://trac.webkit.org/r227644>.
Comment 10 Matt Lewis 2018-01-25 17:31:56 PST
This also broke the windows build.

Looks like its a syntax error and another error:

MathExtras.h(497): error C2059: syntax error: ':' 

https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/7382
Comment 11 Filip Pizlo 2018-01-25 17:37:48 PST
(In reply to Matt Lewis from comment #10)
> This also broke the windows build.
> 
> Looks like its a syntax error and another error:
> 
> MathExtras.h(497): error C2059: syntax error: ':' 
> 
> https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/
> 7382

Fixed in https://trac.webkit.org/changeset/227648/webkit