Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Double free of the exporter's sg_table triggered by combining the newly added drm_prime_gem_destroy() with a pre-existing incorrect error path in vmw_ttm_map_dma(). Pre-existing issues: - [High] NULL pointer dereference in drm_gem_prime_import_dev() because vmw_prime_import_sg_table() returns NULL instead of ERR_PTR on failure. -- commit 9bf7a32b370ca37cb8db240835af8ec49f8b2120 Author: Michal TOMA <[email protected]> drm/vmwgfx: Release PRIME import in the BO destroy path This commit addresses a reference leak for imported dma-buf objects by adding a call to drm_prime_gem_destroy() in vmw_bo_free(). This ensures the sg mapping, attachment, and dma-buf reference are properly released when the GEM object is freed. > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c > index 9c7a73c0b0dc3..56bc94edc30b7 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c [ ... ] > @@ -69,6 +70,8 @@ static void vmw_bo_free(struct ttm_buffer_object *bo) > vmw_surface_unreference(&vbo->dumb_surface); > } > WARN_ON(!RB_EMPTY_ROOT(&vbo->res_tree)); > + if (drm_gem_is_imported(&vbo->tbo.base)) > + drm_prime_gem_destroy(&vbo->tbo.base, vbo->tbo.sg); [Severity: High] Does this code cause a double free of the exporter's sg_table if mapping fails? When an imported PRIME buffer (TTM_TT_FLAG_EXTERNAL) is mapped or bound, vmw_ttm_map_dma() sets up the sg_table using the exporter's sg_table: drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c:vmw_ttm_map_dma() { ... if (vmw_tt->dma_ttm.page_flags & TTM_TT_FLAG_EXTERNAL) { vsgt->sgt = vmw_tt->dma_ttm.sg; } else { ... } ret = vmw_ttm_map_for_dma(vmw_tt); if (unlikely(ret != 0)) goto out_map_fail; ... out_map_fail: drm_warn(&dev_priv->drm, "VSG table map failed!"); sg_free_table(vsgt->sgt); vsgt->sgt = NULL; ... } In vmw_ttm_map_dma(), vmw_ttm_map_for_dma() attempts to call dma_map_sgtable() on an uninitialized inline &vmw_tt->sgt, which predictably fails and jumps to out_map_fail. The error path (as well as vmw_ttm_unmap_dma) incorrectly calls sg_free_table() on the exporter's sg_table. When the buffer object is subsequently destroyed, this newly added call to drm_prime_gem_destroy() asks the exporter to unmap its attachment, causing the exporter to call sg_free_table() again on the exact same scatterlist. [Severity: High] This isn't a bug introduced by this patch, but a pre-existing issue was exposed by analyzing the error path: can a failure in vmw_bo_create() cause a NULL pointer dereference? If a local user imports a dma-buf via DRM_IOCTL_PRIME_FD_TO_HANDLE and vmw_bo_create() fails (e.g., due to memory pressure), vmw_prime_import_sg_table() returns NULL instead of propagating an ERR_PTR: drivers/gpu/drm/vmwgfx/vmwgfx_gem.c:vmw_prime_import_sg_table() { ... ret = vmw_bo_create(dev_priv, ¶ms, &vbo); if (ret != 0) goto out_no_bo; ... out_no_bo: dma_resv_unlock(params.resv); return gem; } The caller expects an ERR_PTR on failure. Since IS_ERR(NULL) evaluates to false, it proceeds to unconditionally assign to obj->import_attach, dereferencing the NULL pointer: drivers/gpu/drm/drm_prime.c:drm_gem_prime_import_dev() { ... obj = dev->driver->gem_prime_import_sg_table(dev, attach, sgt); if (IS_ERR(obj)) { ret = PTR_ERR(obj); goto fail_unmap; } obj->import_attach = attach; ... } > drm_gem_object_release(&vbo->tbo.base); > WARN_ON(vbo->dirty); > kfree(vbo); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
