Bug 219097 – The web process should be killed after failing to decode display list items in the GPU process

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Wenson Hsieh]

<rdar://problem/71546526>

[Wenson Hsieh]

Created attachment 418085 [details]
Depends on #220710

[Wenson Hsieh]

Created attachment 418180 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 418180 [details]
Patch

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

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:272
> +    callOnMainRunLoop([this, protectedThis = makeRef(*this), messageName = *m_currentMessageName] () mutable {

Is it OK that this is asynchronous? If the GPU Process is compromised, does this leave a window of vulnerability before this gets back to the originating web content process?

[Wenson Hsieh]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 418180 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418180&action=review
> 
> > Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:272
> > +    callOnMainRunLoop([this, protectedThis = makeRef(*this), messageName = *m_currentMessageName] () mutable {
> 
> Is it OK that this is asynchronous? If the GPU Process is compromised, does
> this leave a window of vulnerability before this gets back to the
> originating web content process?

My understanding was that there would be a "window of vulnerability" in any case, since we need to call out to the parent process connection (i.e. UI process) from the UI process to do the killing.

But also, in the case where the GPU Process is compromised, would the attacker just be able to avoid this logic altogether? (My understanding was that the GPU Process, by design, isn't supposed to be compromised; otherwise, we end up with the same security pitfalls as we have now with just the web content process).

[Chris Dumez]

Comment on attachment 418180 [details]
Patch

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

Overall, I am not a fan of this patch. I think it adds quite a bit of complexity and does not look nice.

My proposal would be whenever we get an unexpected value from the WebProcess, call a macro that does:
    RELEASE_LOG_FAULT(xxx);
    m_gpuConnectionToWebProcess->gpuProcess().parentProcessConnection()->send(Messages::GPUProcessProxy::TerminateWebProcess(m_webProcessIdentifier), 0);

xxx being a parameter passed to the macro that is a clear message of what was wrong.

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:407
> +    SetForScope setCurrentMessageName(m_currentMessageName, IPC::MessageName::RemoteRenderingBackend_DidCreateSharedDisplayListHandle);

This looks really ugly and it is very unfortunate people will have to remember to do this every time they add a new IPC.

[Wenson Hsieh]

(In reply to Chris Dumez from comment #5)
> Comment on attachment 418180 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418180&action=review
> 
> Overall, I am not a fan of this patch. I think it adds quite a bit of
> complexity and does not look nice.
> 
> My proposal would be whenever we get an unexpected value from the
> WebProcess, call a macro that does:
>     RELEASE_LOG_FAULT(xxx);
>    
> m_gpuConnectionToWebProcess->gpuProcess().parentProcessConnection()-
> >send(Messages::GPUProcessProxy::TerminateWebProcess(m_webProcessIdentifier),
> 0);
> 
> xxx being a parameter passed to the macro that is a clear message of what
> was wrong.
> 
> > Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:407
> > +    SetForScope setCurrentMessageName(m_currentMessageName, IPC::MessageName::RemoteRenderingBackend_DidCreateSharedDisplayListHandle);
> 
> This looks really ugly and it is very unfortunate people will have to
> remember to do this every time they add a new IPC.

That's a really good point — splitting the logging out from the WP termination message would make this patch a lot less messy. I'll give this a try.

[Wenson Hsieh]

Created attachment 418197 [details]
Patch

[Chris Dumez]

Comment on attachment 418197 [details]
Patch

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

Much nicer IMO

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:49
> +    m_gpuConnectionToWebProcess->gpuProcess().parentProcessConnection()->send(Messages::GPUProcessProxy::TerminateWebProcess(m_gpuConnectionToWebProcess->webProcessIdentifier()), 0);

We may want to add a terminateWebProcess() function to GPUConnectionToWebProcess to not have to duplicate this logic in 2 places.

[Wenson Hsieh]

Comment on attachment 418197 [details]
Patch

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

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:49
>> +    m_gpuConnectionToWebProcess->gpuProcess().parentProcessConnection()->send(Messages::GPUProcessProxy::TerminateWebProcess(m_gpuConnectionToWebProcess->webProcessIdentifier()), 0);
> 
> We may want to add a terminateWebProcess() function to GPUConnectionToWebProcess to not have to duplicate this logic in 2 places.

Sounds good! Pulled out the logic into a helper method:

void GPUConnectionToWebProcess::terminateWebProcess()
{
    gpuProcess().parentProcessConnection()->send(Messages::GPUProcessProxy::TerminateWebProcess(m_webProcessIdentifier), 0);
}

[Wenson Hsieh]

Created attachment 418200 [details]
For landing

[EWS]

Committed r271772: <https://trac.webkit.org/changeset/271772>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418200 [details].