Re: [PATCH] drm/vmwgfx: Release PRIME import in the BO destroy path

[sashiko-bot@kernel.org]

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 <michaltoma@sicoop.com>

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, &params, &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/20260910142359.13716-1-michaltoma@sicoop.com?part=1