Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] Unsynchronized rbtree and xarray traversal in vmw_bo_surface() can lead to crashes or UAF. - [High] Kernel stack memory leak in `vmw_gb_surface_reference_internal` error path. -- commit 6ca6707ae183fa071e8ebb217050fe9e1947a0b2 Author: Michal TOMA <[email protected]> drm/vmwgfx: Don't leak a GEM handle when referencing a surface by fd This commit prevents a GEM handle leak by looking up a surface buffer through its dma-buf directly instead of importing the fd into the DRM file. It rejects buffers not exported by the device and simplifies object reference cleanup. Cc: [email protected] # v6.11+ > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > index bd0563741e89d..27f68fd9c22bd 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c [ ... ] > @@ -931,33 +934,37 @@ u32 vmw_lookup_surface_handle_for_buffer(struct > vmw_private *vmw, > > static int vmw_buffer_prime_to_surface_base(struct vmw_private *dev_priv, > struct drm_file *file_priv, > - u32 fd, u32 *handle, > + u32 fd, > struct ttm_base_object **base_p) > { [ ... ] > - user_srf = vmw_lookup_user_surface_for_buffer(dev_priv, bo, *handle); > - if (WARN_ON(!user_srf)) { > - drm_warn(&dev_priv->drm, > - "User surface fd %d (handle %d) is null.\n", fd, > *handle); > + user_srf = vmw_lookup_user_surface_for_buffer(dev_priv, > + to_vmw_bo(dma_buf->priv), > + fd); [Severity: Critical] This is a pre-existing issue, but does this lockless lookup risk a use-after-free or kernel crash? When vmw_lookup_user_surface_for_buffer() is called here, it calls vmw_bo_surface() which iterates over the detached_resources xarray and res_tree rbtree without holding the dma_resv lock or an RCU read lock: drivers/gpu/drm/vmwgfx/vmwgfx_bo.c:vmw_bo_surface() { ... xa_for_each(&vbo->detached_resources, index, res) { if (res->func->res_type == vmw_res_surface) goto out; } ... for (rb_itr = rb_first(&vbo->res_tree); rb_itr; rb_itr = rb_next(rb_itr)) { ... } If another thread concurrently modifies these structures (for example, by adding or removing resources), this lockless traversal could access freed nodes. Should this lookup take the buffer object's dma_resv lock? > + if (!user_srf) { > + drm_dbg_driver(&dev_priv->drm, > + "No user surface for buffer fd %d.\n", fd); > ret = -EINVAL; > goto out; > } [ ... ] [Severity: High] As an additional observation in this file, this is a pre-existing issue, but does vmw_gb_surface_reference_internal() leak kernel stack memory to userspace in its error path? If srf->res.guest_memory_bo is NULL, the error path skips setting a negative error code and jumps directly to out_bad_resource: drivers/gpu/drm/vmwgfx/vmwgfx_surface.c:vmw_gb_surface_reference_internal() { ... if (!srf->res.guest_memory_bo) { DRM_ERROR("Shared GB surface is missing a backup buffer.\n"); goto out_bad_resource; } ... out_bad_resource: ... return ret; } Because ret is still 0 at this point, the caller vmw_gb_surface_reference_ioctl() assumes success and copies the uninitialized stack variable rep_ext into the user-visible rep pointer: drivers/gpu/drm/vmwgfx/vmwgfx_surface.c:vmw_gb_surface_reference_ioctl() { struct drm_vmw_gb_surface_ref_ext_rep rep_ext; int ret; ret = vmw_gb_surface_reference_internal(dev, req, &rep_ext, file_priv); if (unlikely(ret != 0)) return ret; rep->creq = rep_ext.creq.base; rep->crep = rep_ext.crep; } Could this be fixed by explicitly setting ret = -EINVAL before jumping to out_bad_resource in vmw_gb_surface_reference_internal()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
