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 <[email protected]> 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/[email protected]?part=1
