[PATCH v2] drm/vmwgfx: Fix NULL vs error pointer bug in vmw_prime_import_sg_table()

[PATCH v2] drm/vmwgfx: Fix NULL vs error pointer bug in vmw_prime_import_sg_table()

[Dan Carpenter]

The vmw_prime_import_sg_table() function is expected to return error
pointers on error and not NULL.  Otherwise it leads to a NULL dereference
in the caller drm_gem_prime_import_dev().

Propagate the error code from vmw_bo_create().

Fixes: b32233acceff ("drm/vmwgfx: Fix prime import/export")
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
v2: Propagate the error code from vmw_bo_create() instead of hardcoding
    -ENOMEM.

 drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
index 39f8c46550c2..93a92ae625f8 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
@@ -184,7 +184,7 @@ struct drm_gem_object *vmw_prime_import_sg_table(struct drm_device *dev,
 {
 	int ret;
 	struct vmw_private *dev_priv = vmw_priv(dev);
-	struct drm_gem_object *gem = NULL;
+	struct drm_gem_object *gem;
 	struct vmw_bo *vbo;
 	struct vmw_bo_params params = {
 		.domain = (dev_priv->has_mob) ? VMW_BO_DOMAIN_SYS : VMW_BO_DOMAIN_VRAM,
@@ -201,8 +201,10 @@ struct drm_gem_object *vmw_prime_import_sg_table(struct drm_device *dev,
 	dma_resv_lock(params.resv, NULL);
 
 	ret = vmw_bo_create(dev_priv, &params, &vbo);
-	if (ret != 0)
+	if (ret != 0) {
+		gem = ERR_PTR(ret);
 		goto out_no_bo;
+	}
 
 	vbo->tbo.base.funcs = &vmw_gem_object_funcs;
 
-- 
2.53.0

Re: [PATCH v2] drm/vmwgfx: Fix NULL vs error pointer bug in vmw_prime_import_sg_table()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Imported DMA buffers and their scatterlist mappings are leaked because vmw_gem_object_free() does not call drm_prime_gem_destroy().
--

commit 122b763cb3d6da8120f8ce7568a560873e4c1325
Author: Dan Carpenter <error27@gmail.com>

drm/vmwgfx: Fix NULL vs error pointer bug in vmw_prime_import_sg_table()

The vmw_prime_import_sg_table() function is expected to return error
pointers on error and not NULL.  Otherwise it leads to a NULL dereference
in the caller drm_gem_prime_import_dev().

Propagate the error code from vmw_bo_create().

Fixes: b32233acceff ("drm/vmwgfx: Fix prime import/export")
Signed-off-by: Dan Carpenter <error27@gmail.com>

> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
> index 39f8c46550c27..93a92ae625f8e 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c

[ ... ]

> @@ -201,8 +201,10 @@ struct drm_gem_object *vmw_prime_import_sg_table(struct drm_device *dev,
>  	dma_resv_lock(params.resv, NULL);
>  
>  	ret = vmw_bo_create(dev_priv, &params, &vbo);
> -	if (ret != 0)
> +	if (ret != 0) {
> +		gem = ERR_PTR(ret);
>  		goto out_no_bo;
> +	}
>  
>  	vbo->tbo.base.funcs = &vmw_gem_object_funcs;

[Severity: High]
This is a pre-existing issue, but while reviewing the dmabuf import lifecycle,
I noticed a potential memory leak in the GEM object free hook.

The DRM core contract requires drivers using the generic dmabuf import helper
to call drm_prime_gem_destroy() to unmap and free the dmabuf attachment when
the object is freed.

Looking at vmw_gem_object_free() in the same file:

static void vmw_gem_object_free(struct drm_gem_object *gobj)
{
	struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gobj);
	if (bo)
		ttm_bo_fini(bo);
}

Because this completely omits the drm_prime_gem_destroy() call, does this leak
the dma_buf_attachment and sg_table?

Could an untrusted userspace application trigger a denial-of-service by
repeatedly importing and closing dmabufs, permanently pinning the exported
buffers in memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/akPFZ-e9usvsYgf_@stanley.mountain?part=1