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

[sashiko-bot@kernel.org]

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