Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/vmwgfx: uninitialized guest_memory_size read in vmw_cmd_res_switch_backup() --
commit 541f6121a2573592c8475e994e0c33c0a080bc71 Author: Zack Rusin <[email protected]> drm/vmwgfx: validate backup buffer size in command buffer processing This commit adds a bounds check in vmw_cmd_res_switch_backup() to ensure that backup_offset plus res->guest_memory_size does not overflow or exceed the backing MOB size before switching a resource's backup buffer. > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > index a9136a6523cba..0156aee84c9fb 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > @@ -1718,6 +1718,7 @@ static int vmw_cmd_res_switch_backup(struct vmw_private > *dev_priv, > { > struct vmw_bo *vbo; > void *info; > + u64 end; > int ret; > > info = vmw_execbuf_info_from_res(sw_context, res); > @@ -1728,6 +1729,13 @@ static int vmw_cmd_res_switch_backup(struct > vmw_private *dev_priv, > if (ret) > return ret; > > + if (check_add_overflow((u64)backup_offset, > + (u64)res->guest_memory_size, &end) || > + end > vbo->tbo.base.size) { > + VMW_DEBUG_USER("Backup buffer too small for resource.\n"); > + return -EINVAL; > + } [Severity: High] Can this check read uninitialized slab memory from res->guest_memory_size when binding DX shaders or DX streamoutput resources? In drivers/gpu/drm/vmwgfx/vmwgfx_resource.c, vmw_resource_init() initializes struct vmw_resource fields individually without setting res->guest_memory_size: drivers/gpu/drm/vmwgfx/vmwgfx_resource.c:vmw_resource_init() { ... res->id = -1; res->guest_memory_bo = NULL; res->guest_memory_offset = 0; res->guest_memory_dirty = false; res->res_dirty = false; res->coherent = false; res->used_prio = 3; res->dirty = NULL; ... } For DX shaders (submitted via SVGA_3D_CMD_DX_DEFINE_SHADER and bound via SVGA_3D_CMD_DX_BIND_SHADER), vmw_cmd_dx_define_shader() calls vmw_dx_shader_add() in drivers/gpu/drm/vmwgfx/vmwgfx_shader.c, which allocates struct vmw_dx_shader with kmalloc_obj() (without __GFP_ZERO) and calls vmw_resource_init() without ever initializing shader->res.guest_memory_size: drivers/gpu/drm/vmwgfx/vmwgfx_shader.c:vmw_dx_shader_add() { ... shader = kmalloc_obj(*shader); if (!shader) { return -ENOMEM; } res = &shader->res; ... ret = vmw_resource_init(dev_priv, res, true, vmw_dx_shader_res_free, &vmw_dx_shader_func); ... } Similarly, for DX streamoutput resources (submitted via SVGA_3D_CMD_DX_DEFINE_STREAMOUTPUT and bound via SVGA_3D_CMD_DX_BIND_STREAMOUTPUT), vmw_dx_streamoutput_add() in drivers/gpu/drm/vmwgfx/vmwgfx_streamoutput.c allocates struct vmw_dx_streamoutput with kmalloc_obj() without zeroing. When vmw_cmd_dx_bind_streamoutput() in drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c processes the bind command, it calls vmw_dx_streamoutput_set_size(), which stores cmd->body.sizeInBytes in so->size rather than res->guest_memory_size before calling vmw_cmd_res_switch_backup(): drivers/gpu/drm/vmwgfx/vmwgfx_streamoutput.c:vmw_dx_streamoutput_set_size() { struct vmw_dx_streamoutput *so = vmw_res_to_dx_streamoutput(res); so->size = size; } When vmw_cmd_dx_bind_shader() or vmw_cmd_dx_bind_streamoutput() then calls vmw_cmd_res_switch_backup(), res->guest_memory_size holds uninitialized heap memory. Doesn't this cause valid DX shader and streamoutput bindings to fail with -EINVAL whenever the uninitialized value is large, allow userspace to probe the 64-bit uninitialized slab value by varying offsetInBytes or the MOB size, and leave the actual shader and streamoutput byte sizes unvalidated against the MOB size? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
