Re: [PATCH] drm/vmwgfx: validate pitch coming from userspace

On 8/9/26 1:45 PM, Zack Rusin wrote:
> Validate the pitch, alongside the box dimensions before trying
> to copy data from the underlying surface. Fixes possible
> out of bounds reads with cursor snooping.
> 
> Fixes: 2ac863719e51 ("vmwgfx: Snoop DMA transfers with non-covering sizes")
> Cc: [email protected]
> Reported-by: Youness HFA <[email protected]>
> Signed-off-by: Zack Rusin <[email protected]>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c b/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
> index d1e7df500190..f4d14b00d7aa 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
> @@ -324,6 +324,7 @@ void vmw_kms_cursor_snoop(struct vmw_surface *srf,
>  	unsigned long kmap_num;
>  	SVGA3dCopyBox *box;
>  	u32 box_count;
> +	u64 src_extent;
>  	void *virtual;
>  	bool is_iomem;
>  	struct vmw_dma_cmd {
> @@ -372,8 +373,19 @@ void vmw_kms_cursor_snoop(struct vmw_surface *srf,
>  		return;
>  	}
>  
> +	if (box->w == 0 || box->h == 0)
> +		return;
> +
> +	src_extent = (u64)(box->h - 1) * cmd->dma.guest.pitch +
> +		     (u64)box->w * desc->pitchBytesPerBlock;
> +	if (src_extent > bo->base.size) {
> +		DRM_ERROR("Cursor snoop source of %llu bytes exceeds the %zu byte buffer\n",
> +			  src_extent, bo->base.size);
> +		return;
> +	}
> +
>  	kmap_offset = cmd->dma.guest.ptr.offset >> PAGE_SHIFT;
> -	kmap_num = (VMW_CURSOR_SNOOP_HEIGHT * image_pitch) >> PAGE_SHIFT;
> +	kmap_num = PFN_UP(src_extent);
>  
>  	ret = ttm_bo_reserve(bo, true, false, NULL);
>  	if (unlikely(ret != 0)) {
> @@ -387,14 +399,16 @@ void vmw_kms_cursor_snoop(struct vmw_surface *srf,
>  
>  	virtual = ttm_kmap_obj_virtual(&map, &is_iomem);
>  
> -	if (box->w == VMW_CURSOR_SNOOP_WIDTH && cmd->dma.guest.pitch == image_pitch) {
> +	if (box->w == VMW_CURSOR_SNOOP_WIDTH &&
> +	    box->h == VMW_CURSOR_SNOOP_HEIGHT &&
> +	    cmd->dma.guest.pitch == image_pitch) {
>  		memcpy(srf->snooper.image, virtual,
>  		       VMW_CURSOR_SNOOP_HEIGHT * image_pitch);
>  	} else {
>  		/* Image is unsigned pointer. */
>  		for (i = 0; i < box->h; i++)
>  			memcpy(srf->snooper.image + i * image_pitch,
> -			       virtual + i * cmd->dma.guest.pitch,
> +			       virtual + (size_t)i * cmd->dma.guest.pitch,
>  			       box->w * desc->pitchBytesPerBlock);
>  	}
>  	srf->snooper.id++;


LGTM!

Reviewed-by: Maaz Mombasawala <[email protected]>

Will you also backport this to pre-6.14 LTR kernels when all this code was in vmwgfx_kms.c?


-- 
Maaz Mombasawala <[email protected]>