[PATCH] drm/vmwgfx: Validate vmw_surface_metadata::array_size

[PATCH] drm/vmwgfx: Validate vmw_surface_metadata::array_size

[Ian Forbes]

This field comes from userspace and should be validated against specific
limits depending on which Shader Model (SM) is available.

Fixes: 504901dbb0b5 ("drm/vmwgfx: Refactor surface_define to use vmw_surface_metadata")
Signed-off-by: Ian Forbes <ian.forbes@broadcom.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index b2d3927b5567..bd0563741e89 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -78,7 +78,7 @@ static int vmw_gb_surface_unbind(struct vmw_resource *res,
 static int vmw_gb_surface_destroy(struct vmw_resource *res);
 static int
 vmw_gb_surface_define_internal(struct drm_device *dev,
-			       struct drm_vmw_gb_surface_create_ext_req *req,
+			       const  struct drm_vmw_gb_surface_create_ext_req *req,
 			       struct drm_vmw_gb_surface_create_rep *rep,
 			       struct drm_file *file_priv);
 static int
@@ -1503,7 +1503,7 @@ int vmw_gb_surface_reference_ext_ioctl(struct drm_device *dev, void *data,
  */
 static int
 vmw_gb_surface_define_internal(struct drm_device *dev,
-			       struct drm_vmw_gb_surface_create_ext_req *req,
+			       const  struct drm_vmw_gb_surface_create_ext_req *req,
 			       struct drm_vmw_gb_surface_create_rep *rep,
 			       struct drm_file *file_priv)
 {
@@ -1521,9 +1521,21 @@ vmw_gb_surface_define_internal(struct drm_device *dev,
 				req->base.svga3d_flags);
 
 	/* array_size must be null for non-GL3 host. */
-	if (req->base.array_size > 0 && !has_sm4_context(dev_priv)) {
-		VMW_DEBUG_USER("SM4 surface not supported.\n");
-		return -EINVAL;
+	if (req->base.array_size > 0) {
+		if (has_sm5_context(dev_priv)) {
+			if (req->base.array_size > SVGA3D_SM5_MAX_SURFACE_ARRAYSIZE) {
+				VMW_DEBUG_USER("Invalid Surface Array Size.\n");
+				return -EINVAL;
+			}
+		} else if (has_sm4_context(dev_priv)) {
+			if (req->base.array_size > SVGA3D_SM4_MAX_SURFACE_ARRAYSIZE) {
+				VMW_DEBUG_USER("Invalid Surface Array Size.\n");
+				return -EINVAL;
+			}
+		} else {
+			VMW_DEBUG_USER("SM4+ surface not supported.\n");
+			return -EINVAL;
+		}
 	}
 
 	if (!has_sm4_1_context(dev_priv)) {
-- 
2.54.0

Re: [PATCH] drm/vmwgfx: Validate vmw_surface_metadata::array_size

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Integer overflows and missing bounds checks in surface size computations lead to severe buffer undersizing and out-of-bounds memory access.
- [Critical] Integer truncation from `unsigned long` to `uint32_t` causes critical buffer undersize when allocating GEM objects for large surfaces.
--

commit 50825a4076b6af2aa84715573bcdaf84763db731
Author: Ian Forbes <ian.forbes@broadcom.com>

drm/vmwgfx: Validate vmw_surface_metadata::array_size

This field comes from userspace and should be validated against specific
limits depending on which Shader Model (SM) is available.

> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> index b2d3927b55673..bd0563741e89d 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> @@ -1521,9 +1521,21 @@ vmw_gb_surface_define_internal(struct drm_device *dev,
>  				req->base.svga3d_flags);
>  
>  	/* array_size must be null for non-GL3 host. */
> -	if (req->base.array_size > 0 && !has_sm4_context(dev_priv)) {
> -		VMW_DEBUG_USER("SM4 surface not supported.\n");
> -		return -EINVAL;
> +	if (req->base.array_size > 0) {
> +		if (has_sm5_context(dev_priv)) {
> +			if (req->base.array_size > SVGA3D_SM5_MAX_SURFACE_ARRAYSIZE) {
> +				VMW_DEBUG_USER("Invalid Surface Array Size.\n");
> +				return -EINVAL;
> +			}
> +		} else if (has_sm4_context(dev_priv)) {
> +			if (req->base.array_size > SVGA3D_SM4_MAX_SURFACE_ARRAYSIZE) {
> +				VMW_DEBUG_USER("Invalid Surface Array Size.\n");
> +				return -EINVAL;
> +			}
> +		} else {
> +			VMW_DEBUG_USER("SM4+ surface not supported.\n");
> +			return -EINVAL;
> +		}
>  	}

[Severity: Critical]
This is a pre-existing issue, but does the surface size computation lack
sufficient bounds checking for non-scanout formats?

If a user-space process issues a surface define ioctl for a non-scanout
format with extremely large base_size dimensions, vmw_gb_surface_define()
skips checking the maximum boundaries:

drivers/gpu/drm/vmwgfx/vmwgfx_surface.c:vmw_gb_surface_define() {
    ...
    } else {
        const SVGA3dSurfaceDesc *desc =
            vmw_surface_get_desc(req->format);

        if (desc->blockDesc == SVGA3DBLOCKDESC_NONE) {
            VMW_DEBUG_USER("Invalid surface format.\n");
            return -EINVAL;
        }
    }
    ...
}

This allows massive dimensions to be processed without validation by
vmw_surface_calculate_pitch() and vmw_surface_get_serialized_size(), where
the 32-bit multiplications can wrap around:

drivers/gpu/drm/vmwgfx/vmw_surface_cache.h:vmw_surface_calculate_pitch() {
    ...
    pitch = blocks.width * desc->pitchBytesPerBlock;
    ...
}

Because this wraps around to a small u32 value, it can evade the U32_MAX
clamp in vmw_surface_get_serialized_size_extended(), resulting in a
tiny guest_memory_size allocation while the original massive dimensions
remain in the surface metadata. Can this lead to severe guest physical
memory corruption from out-of-bounds access?

[ ... ]

>  	} else if (req->base.drm_surface_flags &
>  		   (drm_vmw_surface_flag_create_buffer |
>  		    drm_vmw_surface_flag_coherent)) {
>  		ret = vmw_gem_object_create_with_handle(dev_priv, file_priv,
>  							res->guest_memory_size,
>  							&backup_handle,
>  							&res->guest_memory_bo);
>  	}

[Severity: Critical]
This isn't a bug introduced by this patch, but does the guest memory size
get silently truncated when creating the GEM object for large surfaces?

If a user-space process requests a large surface that causes
vmw_surface_get_serialized_size_extended() to return exactly U32_MAX
(the clamped maximum), and sets the SVGA3D_SURFACE_BIND_STREAM_OUTPUT flag,
vmw_gb_surface_define() unconditionally adds 16 bytes to the size:

drivers/gpu/drm/vmwgfx/vmwgfx_surface.c:vmw_gb_surface_define() {
    ...
    if (metadata->flags & SVGA3D_SURFACE_BIND_STREAM_OUTPUT)
        srf->res.guest_memory_size += sizeof(SVGA3dDXSOState);
    ...
}

This pushes the 64-bit value to 0x10000000F (U32_MAX + 16). When this is
passed to vmw_gem_object_create_with_handle(), the size appears to be
truncated because the function signature takes a 32-bit size:

drivers/gpu/drm/vmwgfx/vmwgfx_gem.c:vmw_gem_object_create_with_handle() {
    int vmw_gem_object_create_with_handle(struct vmw_private *dev_priv,
                                          struct drm_file *filp,
                                          uint32_t size,
                                          uint32_t *handle,
                                          struct vmw_bo **p_vbo)
    ...
}

This truncation wraps the 0x10000000F size down to just 15 bytes. Is it
possible for this to allocate a tiny 15-byte GEM object for a surface
that the metadata considers to be 4GB, leading to massive out-of-bounds
memory overrides during surface operations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623193314.506257-1-ian.forbes@broadcom.com?part=1

Re: [PATCH] drm/vmwgfx: Validate vmw_surface_metadata::array_size

[Maaz Mombasawala]

On 6/23/26 12:33 PM, Ian Forbes wrote:
> This field comes from userspace and should be validated against specific
> limits depending on which Shader Model (SM) is available.
> 
> Fixes: 504901dbb0b5 ("drm/vmwgfx: Refactor surface_define to use vmw_surface_metadata")
> Signed-off-by: Ian Forbes <ian.forbes@broadcom.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> index b2d3927b5567..bd0563741e89 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> @@ -78,7 +78,7 @@ static int vmw_gb_surface_unbind(struct vmw_resource *res,
>  static int vmw_gb_surface_destroy(struct vmw_resource *res);
>  static int
>  vmw_gb_surface_define_internal(struct drm_device *dev,
> -			       struct drm_vmw_gb_surface_create_ext_req *req,
> +			       const  struct drm_vmw_gb_surface_create_ext_req *req,
>  			       struct drm_vmw_gb_surface_create_rep *rep,
>  			       struct drm_file *file_priv);
>  static int
> @@ -1503,7 +1503,7 @@ int vmw_gb_surface_reference_ext_ioctl(struct drm_device *dev, void *data,
>   */
>  static int
>  vmw_gb_surface_define_internal(struct drm_device *dev,
> -			       struct drm_vmw_gb_surface_create_ext_req *req,
> +			       const  struct drm_vmw_gb_surface_create_ext_req *req,
>  			       struct drm_vmw_gb_surface_create_rep *rep,
>  			       struct drm_file *file_priv)
>  {
> @@ -1521,9 +1521,21 @@ vmw_gb_surface_define_internal(struct drm_device *dev,
>  				req->base.svga3d_flags);
>  
>  	/* array_size must be null for non-GL3 host. */
> -	if (req->base.array_size > 0 && !has_sm4_context(dev_priv)) {
> -		VMW_DEBUG_USER("SM4 surface not supported.\n");
> -		return -EINVAL;
> +	if (req->base.array_size > 0) {
> +		if (has_sm5_context(dev_priv)) {
> +			if (req->base.array_size > SVGA3D_SM5_MAX_SURFACE_ARRAYSIZE) {
> +				VMW_DEBUG_USER("Invalid Surface Array Size.\n");
> +				return -EINVAL;
> +			}
> +		} else if (has_sm4_context(dev_priv)) {
> +			if (req->base.array_size > SVGA3D_SM4_MAX_SURFACE_ARRAYSIZE) {
> +				VMW_DEBUG_USER("Invalid Surface Array Size.\n");
> +				return -EINVAL;
> +			}
> +		} else {
> +			VMW_DEBUG_USER("SM4+ surface not supported.\n");
> +			return -EINVAL;
> +		}
>  	}
>  
>  	if (!has_sm4_1_context(dev_priv)) {

LGTM!

Reviewed-by: Maaz Mombasawala <maaz.mombasawala@broadcom.com>

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