Bug 222230 - [GPU Process] Implement the ClipToDrawingCommands item by using begin and end markers
Summary: [GPU Process] Implement the ClipToDrawingCommands item by using begin and end...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
: 220377 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-02-20 11:37 PST by Said Abou-Hallawa
Modified: 2021-03-01 22:34 PST (History)
6 users (show)

See Also:


Attachments
Patch (30.72 KB, patch)
2021-02-20 11:55 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (31.59 KB, patch)
2021-02-22 11:52 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (31.59 KB, patch)
2021-02-22 11:53 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2021-02-20 11:37:37 PST
Instead of encoding and decoding the clipping commands as a separate DisplayList, the recorder will insert a "begin" and "end" markers before and after the clipping commands. When replaying the "begin" mark, the relpayer will create a mask ImageBuffer and force using its context for the following items . When replaying the "end" mark, it will clip the original context to the mask ImageBuffer.
Comment 1 Said Abou-Hallawa 2021-02-20 11:55:45 PST
Created attachment 421096 [details]
Patch
Comment 2 Tim Horton 2021-02-20 20:17:18 PST
Comment on attachment 421096 [details]
Patch

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

This does not seem outrageous

> Source/WebCore/ChangeLog:12
> +        When replaying the "begin" mark, the relpayer will create a mask ImageBuffer

sp. `relpayer`

> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:160
> +        if (!m_maskImageBuffer)
> +            return { StopReplayReason::InvalidItem, WTF::nullopt };

Is the item really invalid? Doesn't this probably mean we ran out of memory? I guess it doesn't matter. But, what is the upstream behavior of this?

Also, what happens to the rest of the DL when this happens? Will you replay the items that were supposed to go to the mask onto the main context?
Comment 3 Simon Fraser (smfr) 2021-02-22 11:16:18 PST
Comment on attachment 421096 [details]
Patch

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

> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:164
> +    if (item.is<EndClipToDrawingCommands>()) {

What happens if we never see the "end" command? What if begin/end are deeply nested? Can an attacker exploit either of these trigger buffer overflow?
Comment 4 Said Abou-Hallawa 2021-02-22 11:52:37 PST
Created attachment 421216 [details]
Patch
Comment 5 Said Abou-Hallawa 2021-02-22 11:53:51 PST
Created attachment 421218 [details]
Patch
Comment 6 Said Abou-Hallawa 2021-02-22 12:01:20 PST
Comment on attachment 421096 [details]
Patch

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

>> Source/WebCore/ChangeLog:12
>> +        When replaying the "begin" mark, the relpayer will create a mask ImageBuffer
> 
> sp. `relpayer`

Fixed.

>> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:160
>> +            return { StopReplayReason::InvalidItem, WTF::nullopt };
> 
> Is the item really invalid? Doesn't this probably mean we ran out of memory? I guess it doesn't matter. But, what is the upstream behavior of this?
> 
> Also, what happens to the rest of the DL when this happens? Will you replay the items that were supposed to go to the mask onto the main context?

I added StopReplayReason::OutOfMemory to handle this case.

I added also the following check in RemoteRenderingBackend::nextDestinationImageBufferAfterApplyingDisplayLists()

MESSAGE_CHECK_WITH_RETURN_VALUE(result.reasonForStopping != DisplayList::StopReplayReason::OutOfMemory, nullptr, "Cound not allocate memory");

This will terminate the WebProcess:

#define MESSAGE_CHECK_WITH_RETURN_VALUE(assertion, returnValue, message) do { \
    if (UNLIKELY(!(assertion))) { \
        TERMINATE_WEB_PROCESS_WITH_MESSAGE(message); \
        return (returnValue); \
    } \
} while (0)

>> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:164
>> +    if (item.is<EndClipToDrawingCommands>()) {
> 
> What happens if we never see the "end" command? What if begin/end are deeply nested? Can an attacker exploit either of these trigger buffer overflow?

clipToDrawingCommands() can be called internally. Currently it is only called form one place which is CanvasRenderingContext2DBase::drawTextUnchecked(). It does not make sense to have nested calls from clipToDrawingCommands() because the drawing commands create a mask image. Creating the mask should not involve drawing with gradient. And the drawing happens only in the lambda function which we control.
Comment 7 Simon Fraser (smfr) 2021-02-22 15:36:57 PST
(In reply to Said Abou-Hallawa from comment #6)

> >> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:164
> >> +    if (item.is<EndClipToDrawingCommands>()) {
> > 
> > What happens if we never see the "end" command? What if begin/end are deeply nested? Can an attacker exploit either of these trigger buffer overflow?
> 
> clipToDrawingCommands() can be called internally. Currently it is only
> called form one place which is
> CanvasRenderingContext2DBase::drawTextUnchecked(). It does not make sense to
> have nested calls from clipToDrawingCommands() because the drawing commands
> create a mask image. Creating the mask should not involve drawing with
> gradient. And the drawing happens only in the lambda function which we
> control.

Let me ask it this way. What happens when an IPC fuzzer generates display list IPC with lots of "begin clip to drawing command" but no "end"?
Comment 8 Said Abou-Hallawa 2021-02-22 16:31:03 PST
(In reply to Simon Fraser (smfr) from comment #7)
> (In reply to Said Abou-Hallawa from comment #6)
> 
> > >> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:164
> > >> +    if (item.is<EndClipToDrawingCommands>()) {
> > > 
> > > What happens if we never see the "end" command? What if begin/end are deeply nested? Can an attacker exploit either of these trigger buffer overflow?
> > 
> > clipToDrawingCommands() can be called internally. Currently it is only
> > called form one place which is
> > CanvasRenderingContext2DBase::drawTextUnchecked(). It does not make sense to
> > have nested calls from clipToDrawingCommands() because the drawing commands
> > create a mask image. Creating the mask should not involve drawing with
> > gradient. And the drawing happens only in the lambda function which we
> > control.
> 
> Let me ask it this way. What happens when an IPC fuzzer generates display
> list IPC with lots of "begin clip to drawing command" but no "end"?

Replayer::applyItem() will terminate the web process once it detects a "begin clip to drawing command" item while m_maskImageBuffer is not null.

    if (item.is<BeginClipToDrawingCommands>()) {
        if (m_maskImageBuffer)
            return { StopReplayReason::InvalidItem, WTF::nullopt };
        ...
        return { WTF::nullopt, WTF::nullopt };
    }
Comment 9 Simon Fraser (smfr) 2021-02-22 16:41:39 PST
Comment on attachment 421218 [details]
Patch

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

> Source/WebCore/platform/graphics/displaylists/DisplayListItemType.h:55
> +    BeginClipToDrawingCommands,
> +    EndClipToDrawingCommands,

At some point we should make this more generic (for filters etc).
Comment 10 Said Abou-Hallawa 2021-02-22 17:49:45 PST
Comment on attachment 421218 [details]
Patch

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

>> Source/WebCore/platform/graphics/displaylists/DisplayListItemType.h:55
>> +    EndClipToDrawingCommands,
> 
> At some point we should make this more generic (for filters etc).

I wish we could encode/decode the DisplayList instead of these two markers. But the current DisplayList implementation makes it a little bit hard unless we do a significant code re-factoring.
Comment 11 EWS 2021-02-22 18:01:02 PST
Committed r273291: <https://commits.webkit.org/r273291>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421218 [details].
Comment 12 Radar WebKit Bug Importer 2021-02-22 18:02:33 PST
<rdar://problem/74622997>
Comment 13 Said Abou-Hallawa 2021-03-01 22:34:58 PST
*** Bug 220377 has been marked as a duplicate of this bug. ***