Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] drm/vmwgfx: handle namespace collision leads to object hijacking and UAF - [Critical] drm/vmwgfx: TOCTOU race in prime import allows cross-process surface hijacking - [High] drm/vmwgfx: PRIME_HANDLE_TO_FD fails on bridge GEM handles - [High] drm/vmwgfx: memory leaks of surface and bridge objects during PRIME cleanup - [Medium] drm/vmwgfx: violation of PRIME_FD_TO_HANDLE caching semantics --
commit 1a74f33f037015680dfd44a4127992d0bec7eedf Author: Claire DuSoleil <[email protected]> drm/vmwgfx: Fix GEM_CLOSE returning -EINVAL for PRIME-imported surfaces This commit addresses a bug where DRM_IOCTL_GEM_CLOSE fails for PRIME-imported surfaces in vmwgfx. It fixes this by wrapping the private handle in a non-TTM-backed GEM object registered in the standard per-file handle table. > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c > index 598b90ac7590b..531525122fda3 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c [ ... ] > +uint32_t vmw_prime_resolve_handle(struct drm_file *file_priv, uint32_t > handle) > +{ > + struct drm_gem_object *gobj = drm_gem_object_lookup(file_priv, handle); > + uint32_t real_handle = handle; > + > + if (gobj) { > + if (gobj->funcs == &vmw_prime_import_bridge_funcs) { > + struct vmw_prime_import_bridge *bridge = > + container_of(gobj, struct > vmw_prime_import_bridge, base); > + real_handle = bridge->ttm_handle; > + } [Severity: Critical] Does this code silently intercept legitimate TTM handles of the same numerical value? GEM handles and TTM handles share the same 32-bit integer space but are allocated from independent IDRs, so collisions are highly likely. By checking the GEM IDR first, if a new TTM surface is allocated with an ID that matches an existing bridge GEM handle, vmw_user_resource_lookup_handle() will resolve it to the bridge's surface instead of the new surface. [Severity: High] Can DRM_IOCTL_PRIME_HANDLE_TO_FD fail on these bridge GEM handles? When exporting, vmw_prime_handle_to_fd() routes handles <= VMWGFX_NUM_MOB to vmw_user_bo_lookup(), which explicitly rejects bridge handles (-ESRCH). Handles > VMWGFX_NUM_MOB are routed to ttm_prime_handle_to_fd(), which looks up the GEM handle in the TTM namespace, causing a failure or exporting an unrelated surface. This breaks the generic PRIME round-trip property for imported surfaces. > + drm_gem_object_put(gobj); > + } > + > + return real_handle; > +} > + > int vmw_prime_fd_to_handle(struct drm_device *dev, > struct drm_file *file_priv, > int fd, u32 *handle) > { > + struct vmw_private *dev_priv = vmw_priv(dev); > struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile; > - int ret = ttm_prime_fd_to_handle(tfile, fd, handle); > + struct vmw_prime_import_bridge *bridge; > + struct ttm_base_object *base_obj; > + uint32_t ttm_handle; > + int ret = ttm_prime_fd_to_handle(tfile, fd, &ttm_handle); > + > + if (ret) > + return drm_gem_prime_fd_to_handle(dev, file_priv, fd, handle); > + > + /* > + * Take our own independent reference before dropping the transient > + * one ttm_prime_fd_to_handle() just created, so the object can never > + * be dropped to zero in between. > + */ > + base_obj = ttm_base_object_lookup_for_ref(dev_priv->tdev, ttm_handle); [Severity: Critical] Is there a TOCTOU race here that allows cross-process surface hijacking? Between the return of ttm_prime_fd_to_handle() and this IDR lookup, the object is only kept alive by the userspace fd and the tfile ref. If a malicious user closes the fd and calls DRM_VMW_UNREF_SURFACE(ttm_handle) concurrently, they can destroy the surface and free the ID. Another process can create a new surface, re-acquiring the same ID. When this thread resumes and calls ttm_base_object_lookup_for_ref(), it finds the new surface and wraps it in a bridge GEM object, granting unauthorized cross-process access to the innocent user's GPU memory. > + if (!base_obj) { > + ttm_ref_object_base_unref(tfile, ttm_handle); > + return -EINVAL; > + } > + > + bridge = kzalloc(sizeof(*bridge), GFP_KERNEL); > + if (!bridge) { [Severity: Medium] Does unconditionally allocating a new bridge and GEM handle violate PRIME_FD_TO_HANDLE caching semantics? The standard DRM PRIME API contract guarantees that importing the same DMA-BUF fd multiple times within the same drm_file returns the identical GEM handle, allowing userspace to deduplicate buffers. This bypasses the generic caching and creates multiple unique GEM handles wrapping the same buffer, breaking buffer tracking logic in compositors and leaking resources. > + ttm_base_object_unref(&base_obj); > + ttm_ref_object_base_unref(tfile, ttm_handle); > + return -ENOMEM; > + } [ ... ] > + /* > + * On success, deliberately leave the transient ttm_ref_object entry > + * ttm_prime_fd_to_handle() created in place -- do not touch it. [Severity: High] Does intentionally skipping cleanup of the TTM reference cause a memory leak when GEM_CLOSE is called? Generic userspace calling DRM_IOCTL_GEM_CLOSE invokes vmw_prime_import_bridge_free(), which frees the bridge but fails to release the ttm_ref_object left alive here, leaking the surface indefinitely. > + * vmw_surface_handle_reference()'s DRM_VMW_REF_SURFACE path forces [ ... ] > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > index bd0563741e89d..5aabb369e83ca 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > @@ -689,8 +689,9 @@ int vmw_surface_destroy_ioctl(struct drm_device *dev, > void *data, > { > struct drm_vmw_surface_arg *arg = (struct drm_vmw_surface_arg *)data; > struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile; > + uint32_t sid = vmw_prime_resolve_handle(file_priv, arg->sid); > > - return ttm_ref_object_base_unref(tfile, arg->sid); > + return ttm_ref_object_base_unref(tfile, sid); [Severity: High] Does dropping the TTM ref without deleting the GEM handle permanently leak the bridge object? Legacy userspace calling DRM_VMW_UNREF_SURFACE invokes this ioctl, which drops the ttm_ref_object but does not close the GEM handle. > } > > /** [ ... ] > @@ -2321,7 +2328,7 @@ int vmw_dumb_create(struct drm_file *file_priv, > args->size = arg.rep.buffer_size; > args->pitch = vmw_surface_calculate_pitch(desc, &drm_size); > > - ret = vmw_user_resource_lookup_handle(dev_priv, tfile, arg.rep.handle, > + ret = vmw_user_resource_lookup_handle(dev_priv, file_priv, tfile, > arg.rep.handle, > user_surface_converter, > &res); [Severity: Critical] If vmw_prime_resolve_handle() intercepts the ID in vmw_user_resource_lookup_handle() and resolves it to an unrelated bridge surface instead of the new surface, does this lead to dropping a reference on the wrong BO later in vmw_dumb_create(), causing a Use-After-Free and cross-process data corruption? vmw_dumb_create() mutates the resolved surface and then executes drm_gem_object_put(&vbo->tbo.base) on it. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
