Re: [PATCH v6 09/16] drm/msm/gem: Remove useless locking in GEM import

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Pre-existing error path vulnerability in `msm_gem_import` causes a double-free of the exporter's `sgt`, NULL pointer dereference, and wild pointer dereferences during cleanup.
--

--- Patch [9]: [PATCH v6 09/16] drm/msm/gem: Remove useless locking in GEM import ---
commit 19246e79feb40c4e0066b56b3385fea83fcc3d75
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/gem: Remove useless locking in GEM import
    
This patch removes unnecessary locking during the GEM object import process
in msm_gem_import().

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 2cb3ab04f1250..a358fb5ca6d73 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -1302,23 +1302,18 @@ struct drm_gem_object *msm_gem_import(struct drm_device *dev,
>  	npages = size / PAGE_SIZE;
>  
>  	msm_obj = to_msm_bo(obj);
> -	msm_gem_lock(obj);
>  	msm_obj->sgt = sgt;
>  	msm_obj->pages = kvmalloc_objs(struct page *, npages);
>  	if (!msm_obj->pages) {
> -		msm_gem_unlock(obj);
>  		ret = -ENOMEM;
>  		goto fail;
>  	}
>  
>  	ret = drm_prime_sg_to_page_array(sgt, msm_obj->pages, npages);
>  	if (ret) {
> -		msm_gem_unlock(obj);
>  		goto fail;

[Severity: High]
This is a pre-existing issue, but does this error path lead to a NULL
pointer dereference and a double-free?

When msm_gem_import() jumps to fail, it calls drm_gem_object_put(obj).
At this point, obj->import_attach has not yet been set (it is assigned
later by the caller drm_gem_prime_import_dev() on success).

During cleanup, msm_gem_free_object() checks drm_gem_is_imported(obj).
Since import_attach is NULL, this evaluates to false, causing the cleanup
to fall through to the native path:

msm_gem_free_object() ->
    put_pages(obj)

In put_pages(), the code incorrectly frees the exporter-owned sgt (leading
to a double-free) and then calls drm_gem_put_pages().

Finally, drm_gem_put_pages() unconditionally accesses
file_inode(obj->filp)->i_mapping. Because obj->filp is NULL for imported
objects, this causes a NULL pointer dereference.

>  	}
>  
> -	msm_gem_unlock(obj);
> -
>  	drm_gem_lru_move_tail(&priv->lru.pinned, obj);
>  
>  	mutex_lock(&priv->obj_lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717204841.26120-1-robin.clark@oss.qualcomm.com?part=9