Project Fortify, an experimental security project, has identified the following potential security issue. If you’re a feature owner CC-ed on this bug, please do your best to review these reports. Please see https://chromium.googlesource.com/chromium/src/+/main/docs/security/ai-generated-security-bugs-faq.md for more information.

Overview: A potential race condition exists in the unsandboxed Android GPU process due to the non-thread-safe reference counting of OverlayImage. During SkiaOutputDeviceBufferQueue teardown under DrDc, references to OverlayImage are released concurrently on both the CompositorGpuThread and the GPU main thread without synchronization. This can result in a concurrent delete-this and subsequent double-free of OverlayImage, causing memory corruption.

Affected files:

• gpu/command_buffer/service/shared_image/ahardwarebuffer_image_backing_factory.cc
• components/viz/service/display_embedder/skia_output_device_buffer_queue.h
• components/viz/service/display_embedder/skia_output_device_buffer_queue.cc
• ui/gl/gl_surface_egl_surface_control.cc

Estimated timestamp from git blame: 2022-10-28

Root Cause Analysis

In gpu/command_buffer/service/shared_image/ahardwarebuffer_image_backing_factory.cc (at line 71), the helper class OverlayImage is declared as inheriting from the non-thread-safe base class base::RefCounted:

class OverlayImage final : public base::RefCounted<OverlayImage> {

Because base::RefCounted is not thread-safe, its reference count decrement and zero-check in Release() are non-atomic and not synchronized across threads.

When Dual Raster Dual Compositor (DrDc) is enabled (which is the default on Android), two separate threads hold references to the exact same OverlayImage instance:

1. GPU Main Thread: AHardwareBufferImageBacking holds a scoped_refptr<OverlayImage> in its overlay_image_ member variable (line 423).
2. CompositorGpuThread: The display compositor’s surface control queue holds a std::unique_ptr<base::android::ScopedHardwareBufferFenceSync> (which wraps OverlayImage inside ScopedHardwareBufferFenceSyncImpl::image_ at line 112) in GLSurfaceEGLSurfaceControl’s frame resource lists (e.g., current_frame_resources_ or pending_transaction_acks_).

Potential Tear-down Race Condition

During the destruction of SkiaOutputDeviceBufferQueue on the CompositorGpuThread, its member variables are destroyed in the reverse order of their declaration in skia_output_device_buffer_queue.h:

std::unique_ptr<OutputPresenter> presenter_; // line 93 (declared first)
...
std::unordered_set<OverlayData, ...> overlays_; // line 123 (declared last)

Because overlays_ is declared after presenter_, it is destroyed first during teardown:

1. On the CompositorGpuThread, overlays_ is cleared, destroying OverlayAHBImageRepresentation and dropping the representation reference on the backing via SharedImageManager::OnRepresentationDestroyed().
2. Concurrently, a compromised renderer sends a Mojo request mojom::DeferredRequest{DestroySharedImage}, which is processed on the GPU main thread and drops the final factory reference on the backing. This triggers the destruction of AHardwareBufferImageBacking on the GPU main thread.
3. Concurrent Release:
   • On the GPU Main Thread, ~AHardwareBufferImageBacking is executed. During the class member destruction phase (which runs after the destructor body completes and is therefore not protected by any internal backing lock), overlay_image_ is destroyed, calling Release() on OverlayImage.
   • Simultaneously, on the CompositorGpuThread, the teardown proceeds to destroy presenter_ (which owns GLSurfaceEGLSurfaceControl). The destruction of current_frame_resources_ releases the ScopedHardwareBufferFenceSyncImpl reference, calling a second concurrent Release() on the same OverlayImage instance.

Since the reference counter is non-atomic and lacks shared synchronization, both threads can concurrently decrement and observe the reference count as zero, resulting in a concurrent delete this (double-free) of the OverlayImage object.

Potential Steps to Trigger

Since our tooling does not currently have the ability to run code, the following are potential steps an attacker might follow from a compromised renderer to attempt to trigger this vulnerability:

1. On Android with DrDc enabled, open a same-origin popup window to gain control over a RootCompositorFrameSink lifetime.
2. Create a SharedImage using gpu.mojom.SharedImageInterface with the flags SHARED_IMAGE_USAGE_SCANOUT | DISPLAY_READ | RASTER_WRITE and format RGBA_8888 to create a backing.
3. Submit a CompositorFrame containing a TextureDrawQuad referencing the created SharedImage and promote it to a SurfaceControl overlay. This sets up the concurrent OverlayImage reference states.
4. Close the popup window to force the browser to initiate teardown of the RootCompositorFrameSink and call ~SkiaOutputDeviceBufferQueue on the CompositorGpuThread.
5. Simultaneously send a rapid stream of DestroySharedImage Mojo calls from the renderer to trigger destruction of the factory reference on the GPU main thread, aiming to hit the race window during the compositor thread’s teardown of the presenter.

Impact

If successfully exploited, a double-free of OverlayImage results in heap corruption. It also causes a double close() of the held fence file descriptors (end_read_fence_ and previous_end_read_fence_), which can close unrelated newly-opened file descriptors in the process. Because the Android GPU process is unsandboxed, this memory corruption vulnerability provides a privilege escalation pathway.

Suggested Fix

To resolve this potential race, make OverlayImage thread-safe refcounted by changing its inheritance from base::RefCounted<OverlayImage> to base::RefCountedThreadSafe<OverlayImage>:

class OverlayImage final : public base::RefCountedThreadSafe<OverlayImage> {

Evaluated with Chrome root at commit: 5133b93d189b383c37805b1cf3a9d2dbfe8d7379

Results so far have been promising, but there can be wrong deductions. Feel free to adjust as follows:

• If you are familiar with the severity guidelines, you may adjust the severity.
• If this is a false positive, and there’s no work to be done, please close as WAI.
• If there is work to do here but not a vulnerability, please change the issue type to Task/Bug/FR.

Data from false positives will be used to improve accuracy over time. And please feel free to reach out to me directly if you have concerns or feedback on the project.