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

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

[Zack Rusin]

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: stable@vger.kernel.org
Reported-by: Youness HFA <hfa.youness@gmail.com>
Signed-off-by: Zack Rusin <zack.rusin@broadcom.com>
---
 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++;
-- 
2.53.0

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

[Maaz Mombasawala]

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: stable@vger.kernel.org
> Reported-by: Youness HFA <hfa.youness@gmail.com>
> Signed-off-by: Zack Rusin <zack.rusin@broadcom.com>
> ---
>  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 <maaz.mombasawala@broadcom.com>

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


-- 
Maaz Mombasawala <maaz.mombasawala@broadcom.com>