Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/vmwgfx: Unconditional handle deletion destroys user GEM handles - [High] drm/vmwgfx: UAF when dereferencing task_pgrp() without RCU read lock - [Low] drm/vmwgfx: Redundant bounds checks duplicate existing logic --
commit 6aae9fde1f923607d6ff8a6e9acd96d7a0082c2d Author: Hui Peng <[email protected]> drm/vmwgfx: fix surface size overflow, execbuf bounds, and cursor/msg leaks This commit fixes multiple integer overflows, out-of-bounds accesses, and resource leaks across drivers/gpu/drm/vmwgfx/. > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > @@ -1511,6 +1511,10 @@ static int vmw_cmd_dma(struct vmw_private *dev_priv, > > cmd = container_of(header, typeof(*cmd), header); > > + if (unlikely(header->size < sizeof(cmd->body) + sizeof(*suffix))) { > + VMW_DEBUG_USER("Invalid DMA command size.\n"); > + return -EINVAL; > + } [Severity: Low] Is this duplicate check in vmw_cmd_dma() intentional? The exact same condition for the header size is verified immediately below this block, making this new check redundant. > if (unlikely(header->size < sizeof(cmd->body) + sizeof(*suffix))) { > VMW_DEBUG_USER("Illegal SVGA_3D_CMD_SURFACE_DMA size.\n"); > return -EINVAL; > } [ ... ] > @@ -1572,6 +1576,10 @@ static int vmw_cmd_draw(struct vmw_private *dev_priv, > VMW_DECLARE_CMD_VAR(*cmd, SVGA3dCmdDrawPrimitives); > SVGA3dVertexDecl *decl = (SVGA3dVertexDecl *)( > (unsigned long)header + sizeof(*cmd)); > + if (unlikely(header->size < sizeof(cmd->body))) { > + VMW_DEBUG_USER("Draw command size too small.\n"); > + return -EINVAL; > + } [Severity: Low] Could we remove this duplicate check? We do the exact same verification a few lines lower in vmw_cmd_draw() right after retrieving the command body with container_of. > SVGA3dPrimitiveRange *range; > uint32_t i; > uint32_t maxnum; > int ret; > > cmd = container_of(header, typeof(*cmd), header); > > if (unlikely(header->size < sizeof(cmd->body))) { [ ... ] > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c > @@ -1045,7 +1046,7 @@ int vmw_mksstat_add_ioctl(struct drm_device *dev, void > *data, > hypervisor_ppn_add((PPN64)page_to_pfn(page)); > > dev_priv->mksstat_user_pages[slot] = page; > - atomic_set(&dev_priv->mksstat_user_pids[slot], task_pgrp_vnr(current)); > + atomic_set(&dev_priv->mksstat_user_pids[slot], > pid_nr(task_pgrp(current))); [Severity: High] Can this sequence cause a use-after-free? The function task_pgrp() returns an RCU protected pointer, but it looks like we are calling pid_nr() here without holding the rcu_read_lock. If a concurrent sys_setpgid() changes the process group and drops the reference to the old pid structure, and this thread is preempted right before pid_nr() executes, could the RCU grace period expire and free the pointer before we dereference it? > arg->id = slot; [ ... ] > @@ -1104,7 +1105,7 @@ int vmw_mksstat_remove_ioctl(struct drm_device *dev, > void *data, > > DRM_DEV_INFO(dev->dev, "pid=%d arg.id=%zu\n", current->pid, slot); > > - pgid = task_pgrp_vnr(current); > + pgid = pid_nr(task_pgrp(current)); [Severity: High] Does this have the same RCU protection issue as in vmw_mksstat_add_ioctl()? We are dereferencing the task_pgrp() return value outside of an RCU read lock. > pid = atomic_cmpxchg(&dev_priv->mksstat_user_pids[slot], pgid, > MKSSTAT_PID_RESERVED); [ ... ] > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > @@ -967,12 +979,14 @@ static int vmw_buffer_prime_to_surface_base(struct > vmw_private *dev_priv, > if (ret) { > drm_warn(&dev_priv->drm, > "Couldn't add an object ref for the buffer (%d).\n", > *handle); > + ttm_base_object_unref(&base); > goto out; > } > > *base_p = base; > out: > vmw_user_bo_unref(&bo); > + drm_gem_handle_delete(file_priv, *handle); > > return ret; > } [Severity: High] Could this unconditional handle deletion destroy a GEM handle that is actively owned by userspace? The function drm_gem_prime_fd_to_handle() will return an existing handle if the userspace process previously imported the fd. By unconditionally calling drm_gem_handle_delete() on both the success and error paths in vmw_buffer_prime_to_surface_base(), the handle is permanently removed from the object IDR table. If the userspace application continues using the handle after this call, might this revoke access or cause a use-after-free if the ID is recycled? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
