[webkit-reviews] review granted: [Bug 183458] Split DirectArguments into JSValueOOB and JSValueStrict parts : [Attachment 335496] the patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <utatane.tea at gmail.com> has granted Filip Pizlo
<fpizlo at apple.com>'s request for review:
Bug 183458: Split DirectArguments into JSValueOOB and JSValueStrict parts
https://bugs.webkit.org/show_bug.cgi?id=183458

Attachment 335496: the patch

https://bugs.webkit.org/attachment.cgi?id=335496&action=review




--- Comment #24 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 335496
  --> https://bugs.webkit.org/attachment.cgi?id=335496
the patch

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

Is my understanding about the threat model cared in this patch correct? If so,
r=me.

> Source/JavaScriptCore/ChangeLog:12
> +	   window, a write could appear to be made speculatively and rolled out
later. This means that:

My understanding is that, if you have length in the object (like
DirectArguments), speculative execution of `m_length = xxx` (maybe in an
indirected way) in the untaken path can be done and this updated `m_length` can
be used for speculative loading. Is it correct?

if (unmitigated structure check) {
    // any read and write to this JSObject inline fields should not allow
attackers to speculatively read secret memory region into the cache.
    // But DirectArguments is breaking this rule right now.
}

> Source/JavaScriptCore/ChangeLog:14
> +	   - JSValue objects cannot have lengths, masks, or anything else
inline.

So, if this is used for spectre mitigation, attacker can perform spectre attack
with speculative execution of writes to these fields.

> Source/JavaScriptCore/dfg/DFGCallCreateDirectArgumentsSlowPathGenerator.h:39
>  public:

It restores the value of lengthGPR and storageGPR.

>
Source/JavaScriptCore/dfg/DFGCallCreateDirectArgumentsWithKnownLengthSlowPathGe
nerator.h:37
> +// This calls operationCreateDirectArguments but then restores the value of
lengthGPR.

It restores storageGPR.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5184
> +	   fastStorage = m_out.add(fastStorage,
m_out.constIntPtr(DirectArguments::storageHeaderSize()));
> +	   m_out.store32(length.value, fastStorage,
m_heaps.DirectArguments_Storage_length);
> +	   m_out.store32(m_out.constInt32(minCapacity), fastStorage,
m_heaps.DirectArguments_Storage_minCapacity);
> +	   

OK, we may store the pointer to the middle of auxiliary buffer in the
stack/register and perform GC. But it's OK since it is not JSCell.

> Source/JavaScriptCore/runtime/DirectArguments.h:127
> +	   return *preciseIndexMaskPtr(
> +	       offset.offset(),
> +	       std::max(storageHeader(storage).length,
storageHeader(storage).minCapacity),
> +	       ptr);

It would be nice if we have a comment what preciseIndexMaskPtr does in
wtf/MathExtras.h.
It masks the given ptr with 0xffffffffffffffff (ptrwidth) if `index < length`.
Otherwise, it masks ptr with 0. Similar to Linux kernel's array_ptr.