[PATCH] drm/vmwgfx: Erase the detached resource entry on resource release

[Jaeyeong Lee]

vmw_bo_add_detached_resource() stores a raw struct vmw_resource pointer in
bo->detached_resources without taking a reference on the resource, and the
only place that erases the entry again is vmw_resource_mob_attach(). A
resource that is never validated, and therefore never attached to its
backing mob, keeps its entry until the buffer object itself is destroyed.

vmw_resource_release() detaches the resource from the mob RB tree and drops
its reference on the backing buffer object, but leaves the
detached_resources entry in place. Userspace can hold an independent GEM
handle on that buffer object, either because DRM_VMW_GB_SURFACE_CREATE
returned one in rep.buffer_handle, or because DRM_VMW_GB_SURFACE_REF minted
one with drm_gem_handle_create(). The buffer object then outlives the
surface while still holding a pointer to freed memory.

Any later surface lookup for that buffer object dereferences it:

  vmw_prime_handle_to_fd()
    vmw_lookup_surface_handle_for_buffer()
      vmw_lookup_user_surface_for_buffer()
        vmw_bo_surface()
          res->func->res_type		<-- use-after-free read

vmw_bo_surface() walks detached_resources before the mob RB tree, so the
stale entry is matched first. The kref_get_unless_zero() in
vmw_lookup_user_surface_for_buffer() does not help here: it runs after
the dereference, and struct vmw_user_surface is freed with kfree_rcu()
on a slab that is not SLAB_TYPESAFE_BY_RCU, so the memory is handed out
to unrelated allocations once the grace period elapses.

A sequence that reaches this from userspace is to create a guest-backed
surface with drm_vmw_surface_flag_create_buffer, never validate it,
release it with DRM_VMW_UNREF_SURFACE, and then call
DRM_IOCTL_PRIME_HANDLE_TO_FD on the buffer handle returned by the create
ioctl. All of the ioctls involved are DRM_RENDER_ALLOW. The code was
analysed on v7.2-rc4; the erase call site has been the same since the
commit below. Found by code inspection, not by a runtime reproducer.

Erase the entry next to the existing vmw_resource_mob_detach() call, so
that a released resource is removed from both containers, mirroring what
vmw_resource_mob_attach() does. The dumb buffer teardown path in
vmw_bo_free() clears res->guest_memory_bo before dropping its surface
reference, so it does not reach this code and is unaffected.

Fixes: d6667f0ddf46 ("drm/vmwgfx: Fix handling of dumb buffers")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Jaeyeong Lee <finpiler@xxxxxxxxxxxxxx>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
@@ -138,6 +138,7 @@ static void vmw_resource_release(struct kref *kref)
 		}
 		res->guest_memory_size = false;
 		vmw_resource_mob_detach(res);
+		vmw_bo_del_detached_resource(res->guest_memory_bo, res);
 		if (res->dirty)
 			res->func->dirty_free(res);
 		if (res->coherent)
--
2.43.0