Re: [PATCH] drm/vmwgfx: fix surface size overflow, execbuf bounds, and cursor/msg leaks

[sashiko-bot@kernel.org]

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 <benquike@gmail.com>

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/20260919223441.3884374-1-benquike@gmail.com?part=1