Importing a client's GPU buffer via the generic DRM_IOCTL_PRIME_FD_TO_HANDLE ioctl succeeds for surface-backed dma-bufs, but releasing the resulting handle via the equally-generic DRM_IOCTL_GEM_CLOSE fails with -EINVAL.
vmw_prime_fd_to_handle() imports through ttm_prime_fd_to_handle(), which files the handle in the driver's private ttm_object table (tdev->idr). DRM_IOCTL_GEM_CLOSE only ever looks in the standard per-file GEM handle table (file_priv->object_idr) -- there is no per-driver override for it anywhere in struct drm_driver -- so it can never find a handle from the private table and always returns -EINVAL. This breaks any generic userspace that assumes PRIME_FD_TO_HANDLE and GEM_CLOSE operate as a matched pair on the same table -- true for every other driver, but not this one. Wayland compositors that validate a client's dmabuf with a driver-agnostic import-then-close probe (e.g. Hyprland's linux-dmabuf-v1 create_immed validation) treat the close failure as "the buffer is invalid" and kill the client, even though the buffer is fine. ttm_prime_fd_to_handle() cannot simply be replaced with the generic drm_gem_prime_fd_to_handle() path: vmw_prime_dmabuf_ops leaves .attach and .map_dma_buf stubbed with -ENOSYS (surface storage lives host-side), so the generic import path can never succeed for these buffers at all -- ttm_prime_fd_to_handle() is the only mechanism capable of importing a surface-backed dma-buf. Fix this by wrapping the private handle in a minimal, non-TTM-backed GEM object registered in the standard per-file handle table, so ordinary GEM_CLOSE finds it and releases the real reference through it. Existing userspace (Mesa's own SVGA winsys, vmw_drm_surface_from_handle() in vmw_screen_dri.c) also calls this ioctl for real rendering imports, then feeds the returned value back into DRM_VMW_REF_SURFACE and DRM_VMW_UNREF_SURFACE as a raw ttm handle -- so vmw_prime_fd_to_handle() must keep returning something that still works as a raw ttm handle for those two ioctls, and for every execbuf command that references a surface by handle. vmw_prime_resolve_handle() is added to transparently redirect a bridge handle back to the real ttm handle, and called from every function that consumes a raw surface handle from userspace: vmw_surface_handle_reference(), vmw_surface_destroy_ioctl(), and vmw_user_resource_lookup_handle() -- the shared choke point behind every execbuf command-validation call site, plus vmw_present_ioctl(), vmw_user_object_lookup(), and vmw_dumb_create(). This keeps all of that existing usage unaffected. The bridge takes its own reference directly on the underlying ttm_base_object (ttm_base_object_lookup_for_ref()/ttm_base_object_unref()), independent of the tfile-scoped ttm_ref_object entry that REF_SURFACE/UNREF_SURFACE manipulate, so its lifetime does not depend on how many times userspace opens and closes its own references to the same handle. The transient ttm_ref_object entry that ttm_prime_fd_to_handle() itself creates is deliberately left in place on success, matching stock behavior: vmw_surface_handle_reference() forces require_exist=true for render clients, and ttm_ref_object_add() with require_existed=true only reuses a pre-existing entry rather than creating one, so removing it here would make every subsequent DRM_VMW_REF_SURFACE call from a render client (both Mesa and this validation pattern connect via the render node) fail with -EPERM. Before this change, every object filed in file_priv->object_idr was guaranteed to be a real struct vmw_bo, so vmw_user_bo_lookup() casts the GEM object behind a looked-up handle straight to struct vmw_bo via an unchecked container_of() (to_vmw_bo()). This is no longer true once a bridge object can occupy a slot in the same table: passing a bridge handle to any of vmw_user_bo_lookup()'s ten call sites (buffer/context/ shader binding, DRM_IOCTL_PRIME_HANDLE_TO_FD's small-handle branch, etc.) would reinterpret a small vmw_prime_import_bridge allocation as the much larger struct vmw_bo, causing an out-of-bounds read on every subsequent field access. Fixed by rejecting a non-vmw_bo handle in vmw_user_bo_lookup() itself: reject any handle whose gobj->funcs isn't &vmw_gem_object_funcs, the same driver-established GEM object type tag already used at both of struct vmw_bo's own creation sites, so this introduces no new tagging mechanism and cannot reject a legitimate vmw_bo. Tested on a VMware Workstation guest, kernel 7.2.3-arch1-3 and, after a later kernel update, 7.2.5-3-omarchy: a standalone PRIME_FD_TO_HANDLE/GEM_CLOSE reproducer confirms the fix, real Hyprland now compositing GPU-rendered clients that previously died on their first frame, and the kernel log stays clean across driver init and normal use, with no rejections logged from the new vmw_user_bo_lookup() check. All seven changed files were also confirmed byte-for-byte identical to drm-misc-next, and this patch applies to it directly with no fuzz. Signed-off-by: Claire DuSoleil <[email protected]> --- drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 13 ++ drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 3 + drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 5 +- drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c | 2 +- drivers/gpu/drm/vmwgfx/vmwgfx_prime.c | 147 ++++++++++++++++++++++- drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 4 +- drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 13 +- 7 files changed, 178 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c index 9c7a73c..9d87f19 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c @@ -651,6 +651,19 @@ int vmw_user_bo_lookup(struct drm_file *filp, return -ESRCH; } + /* + * A handle in the standard GEM table is not necessarily a real + * vmw_bo -- it may be a prime-import bridge object (see + * vmwgfx_prime.c). to_vmw_bo() below is an unchecked container_of; + * calling it on anything else corrupts an out-of-bounds pointer. + */ + if (gobj->funcs != &vmw_gem_object_funcs) { + drm_gem_object_put(gobj); + DRM_ERROR("Handle 0x%08lx is not a vmwgfx buffer object.\n", + (unsigned long)handle); + return -ESRCH; + } + *out = to_vmw_bo(gobj); return 0; diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h index 38bea8a..bf8a453 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h @@ -788,6 +788,7 @@ extern int vmw_resource_reserve(struct vmw_resource *res, bool interruptible, extern bool vmw_resource_needs_backup(const struct vmw_resource *res); extern int vmw_user_resource_lookup_handle( struct vmw_private *dev_priv, + struct drm_file *file_priv, struct ttm_object_file *tfile, uint32_t handle, const struct vmw_user_resource_conv *converter, @@ -1105,6 +1106,8 @@ extern int vmw_prime_handle_to_fd(struct drm_device *dev, struct drm_file *file_priv, uint32_t handle, uint32_t flags, int *prime_fd); +extern uint32_t vmw_prime_resolve_handle(struct drm_file *file_priv, + uint32_t handle); struct drm_gem_object *vmw_prime_import_sg_table(struct drm_device *dev, struct dma_buf_attachment *attach, struct sg_table *table); diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c index a9136a6..7383319 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c @@ -648,7 +648,8 @@ vmw_cmd_res_check(struct vmw_private *dev_priv, return ret; ret = vmw_user_resource_lookup_handle - (dev_priv, sw_context->fp->tfile, *id_loc, converter, &res); + (dev_priv, sw_context->filp, sw_context->fp->tfile, *id_loc, + converter, &res); if (ret != 0) { VMW_DEBUG_USER("Could not find/use resource 0x%08x.\n", (unsigned int) *id_loc); @@ -4073,7 +4074,7 @@ static int vmw_execbuf_tie_context(struct vmw_private *dev_priv, return ret; ret = vmw_user_resource_lookup_handle - (dev_priv, sw_context->fp->tfile, handle, + (dev_priv, sw_context->filp, sw_context->fp->tfile, handle, user_context_converter, &res); if (ret != 0) { VMW_DEBUG_USER("Could not find or user DX context 0x%08x.\n", diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c index d962ef2..1494a8d 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c @@ -220,7 +220,7 @@ int vmw_present_ioctl(struct drm_device *dev, void *data, } vfb = vmw_framebuffer_to_vfb(fb); - ret = vmw_user_resource_lookup_handle(dev_priv, tfile, arg->sid, + ret = vmw_user_resource_lookup_handle(dev_priv, file_priv, tfile, arg->sid, user_surface_converter, &res); if (ret) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c b/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c index 598b90a..5315251 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c @@ -35,6 +35,7 @@ #include "vmwgfx_bo.h" #include "ttm_object.h" #include <linux/dma-buf.h> +#include <linux/slab.h> /* * DMA-BUF attach- and mapping methods. No need to implement @@ -72,15 +73,157 @@ const struct dma_buf_ops vmw_prime_dmabuf_ops = { .release = NULL, }; +/* + * A surface-backed dma-buf can only be imported through + * ttm_prime_fd_to_handle() -- vmw_prime_dmabuf_ops leaves .attach and + * .map_dma_buf unimplemented above, so the generic PRIME import path + * (drm_gem_prime_fd_to_handle() -> dma_buf_attach()) can never reach it and + * fails with -ENOSYS before it starts. ttm_prime_fd_to_handle() works + * because it bypasses dma-buf attach/map entirely and reads dma_buf->priv + * directly, but the handle it returns lives in the private ttm_object + * table (tdev->idr), not file_priv->object_idr, so the generic + * DRM_IOCTL_GEM_CLOSE (drm_gem_handle_delete(), which only ever looks in + * file_priv->object_idr) can never find it and fails with -EINVAL. + * + * Bridge the two tables: wrap the real ttm_base_object in a minimal, + * non-TTM-backed GEM object and hand back a handle from the standard + * table instead, so ordinary GEM_CLOSE succeeds. + * + * Existing userspace (Mesa's own SVGA winsys, in vmw_drm_surface_from_handle()) + * calls this exact ioctl for real rendering imports too, then feeds the + * returned value straight back into DRM_VMW_REF_SURFACE / DRM_VMW_UNREF_SURFACE + * as a raw ttm handle -- it is not just a probe-only code path. So the value + * returned here must keep working as a raw ttm handle for those two ioctls + * and for every execbuf command that references a surface by handle + * (vmw_user_resource_lookup_handle(), the single choke point all of those + * funnel through). vmw_prime_resolve_handle() below is called from all three + * of those places to transparently redirect a bridge handle back to the real + * ttm handle, so existing userspace keeps working unmodified. + * + * The bridge's own hold on the object is a *separate*, independent + * ttm_base_object reference (taken via ttm_base_object_lookup_for_ref(), + * released via ttm_base_object_unref()), not a claim on the tfile-scoped + * ttm_ref_object entry that REF_SURFACE/UNREF_SURFACE manipulate. That + * entry is created transiently by ttm_prime_fd_to_handle() below and is + * deliberately left alone on success -- see the comment at the end of + * vmw_prime_fd_to_handle() for why it cannot simply be dropped here + * (vmw_surface_handle_reference()'s require_exist path depends on it + * still existing). This bridge's own reference exists alongside it, + * purely so this bridge's lifetime never depends on how many times + * userspace itself opens and closes references to the same ttm handle: + * without it, if userspace fully released the ttm handle through its own + * REF/UNREF_SURFACE calls while this GEM handle was still open, this + * bridge's eventual .free() would unref a handle number that may by then + * have been recycled for a completely unrelated object. + */ +struct vmw_prime_import_bridge { + struct drm_gem_object base; + struct ttm_base_object *base_obj; + uint32_t ttm_handle; +}; + +static void vmw_prime_import_bridge_free(struct drm_gem_object *obj) +{ + struct vmw_prime_import_bridge *bridge = + container_of(obj, struct vmw_prime_import_bridge, base); + + ttm_base_object_unref(&bridge->base_obj); + drm_gem_object_release(obj); + kfree(bridge); +} + +static const struct drm_gem_object_funcs vmw_prime_import_bridge_funcs = { + .free = vmw_prime_import_bridge_free, +}; + +/** + * vmw_prime_resolve_handle - Translate a possible prime-import bridge GEM + * handle back to the real ttm handle it wraps. + * + * @file_priv: The caller's drm file, whose own GEM handle table is checked. + * @handle: A handle as supplied by userspace -- either an ordinary raw ttm + * handle (the common case, unchanged from historical behavior), or a + * bridge handle returned by vmw_prime_fd_to_handle() above. + * + * Returns the real ttm handle to use. If @handle does not name one of this + * file's own prime-import bridge objects, @handle is returned unchanged. + */ +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; + } + 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) - ret = drm_gem_prime_fd_to_handle(dev, file_priv, fd, handle); + 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); + if (!base_obj) { + ttm_ref_object_base_unref(tfile, ttm_handle); + return -EINVAL; + } + + bridge = kzalloc(sizeof(*bridge), GFP_KERNEL); + if (!bridge) { + ttm_base_object_unref(&base_obj); + ttm_ref_object_base_unref(tfile, ttm_handle); + return -ENOMEM; + } + + drm_gem_private_object_init(dev, &bridge->base, PAGE_SIZE); + bridge->base.funcs = &vmw_prime_import_bridge_funcs; + bridge->base_obj = base_obj; + bridge->ttm_handle = ttm_handle; + + ret = drm_gem_handle_create(file_priv, &bridge->base, handle); + drm_gem_object_put(&bridge->base); + + /* + * On success, deliberately leave the transient ttm_ref_object entry + * ttm_prime_fd_to_handle() created in place -- do not touch it. + * vmw_surface_handle_reference()'s DRM_VMW_REF_SURFACE path forces + * require_exist=true for render clients (drm_is_render_client()), + * which is exactly what Mesa and this bridge's own callers are, and + * ttm_ref_object_add() with require_existed=true returns -EPERM + * unless a ref-object entry for this (tfile, ttm_handle) pair + * already exists -- it will not create a new one. This entry is + * that pre-existing one. Stock vmw_prime_fd_to_handle() never + * touched it either, for the same reason. Our own independent + * base_obj reference above is what makes GEM_CLOSE work; it does + * not replace this entry, it exists alongside it. + * + * On failure, nothing else will ever be able to reach this handle to + * release it, so clean it up here to avoid leaking it. + */ + if (ret) + ttm_ref_object_base_unref(tfile, ttm_handle); return ret; } diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c index e3a187a..7ba4dbf 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c @@ -255,6 +255,7 @@ int vmw_resource_init(struct vmw_private *dev_priv, struct vmw_resource *res, * type, -EINVAL will be returned. */ int vmw_user_resource_lookup_handle(struct vmw_private *dev_priv, + struct drm_file *file_priv, struct ttm_object_file *tfile, uint32_t handle, const struct vmw_user_resource_conv @@ -265,6 +266,7 @@ int vmw_user_resource_lookup_handle(struct vmw_private *dev_priv, struct vmw_resource *res; int ret = -EINVAL; + handle = vmw_prime_resolve_handle(file_priv, handle); base = ttm_base_object_lookup(tfile, handle); if (unlikely(!base)) return -EINVAL; @@ -300,7 +302,7 @@ int vmw_user_object_lookup(struct vmw_private *dev_priv, WARN_ON(uo->surface || uo->buffer); - ret = vmw_user_resource_lookup_handle(dev_priv, tfile, handle, + ret = vmw_user_resource_lookup_handle(dev_priv, filp, tfile, handle, user_surface_converter, &res); if (!ret) { diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c index bd05637..5aabb36 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); } /** @@ -999,7 +1000,13 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, &handle, base_p); } else { - handle = u_handle; + /* + * u_handle may be a raw ttm handle (the historical case), or + * a prime-import bridge GEM handle returned by an earlier + * generic PRIME_FD_TO_HANDLE call (see vmwgfx_prime.c) -- + * resolve it back to the real ttm handle either way. + */ + handle = vmw_prime_resolve_handle(file_priv, u_handle); } ret = -EINVAL; @@ -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); if (ret) {
