Re: [PATCH v2 2/5] drm/msm/gem: dma_map/unmap_sgtable()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/msm: NULL deref in get_pages() error path via drm_prime_gem_destroy()
- [High] drm/msm: double dma_map/unmap_sgtable() in get_pages() and put_pages()
--

commit 5b4a3d83276519e9cb8831bdfa37a513b1d334a3
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/gem: dma_map/unmap_sgtable()

This commit adds dma_map_sgtable() and dma_unmap_sgtable() calls in
get_pages() and put_pages() to populate the sgtable's dma_address and
dma_len fields for setting up pagetable mappings.

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 181b9ad326729..72e3cd8bb1e7c 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -219,6 +219,15 @@ static struct page **get_pages(struct drm_gem_object *obj)
>  			goto error;
>  		}
>  
> +		ret = dma_map_sgtable(drm_dev_dma_dev(obj->dev),
> +				      msm_obj->sgt,
> +				      DMA_BIDIRECTIONAL,
> +				      DMA_ATTR_SKIP_CPU_SYNC);
> +		if (ret) {
> +			DRM_DEV_ERROR(dev->dev, "could not map sgtable: %d\n", ret);
> +			goto error;
> +		}
> +
>  		update_device_mem(dev->dev_private, obj->size);
>  
>  		/* For non-cached buffers, ensure the new pages are clean
> @@ -233,6 +242,8 @@ static struct page **get_pages(struct drm_gem_object *obj)
>  	return msm_obj->pages;
>  
>  error:
> +	if (!IS_ERR(msm_obj->sgt))
> +		drm_prime_gem_destroy(obj, msm_obj->sgt);

[Severity: High]
Will calling drm_prime_gem_destroy() here dereference a NULL pointer and
leak msm_obj->sgt when dma_map_sgtable() fails?

At the start of get_pages(), imported DMA-BUF objects are rejected:

drivers/gpu/drm/msm/msm_gem.c:get_pages()
	if (drm_WARN_ON_ONCE(obj->dev, drm_gem_is_imported(obj)))
		return ERR_PTR(-EINVAL);

Because get_pages() only runs on native GEM objects, obj->import_attach is
always NULL and msm_obj->sgt is allocated via drm_prime_pages_to_sg().

If dma_map_sgtable() fails and jumps to error, drm_prime_gem_destroy() is
called with a valid msm_obj->sgt, which then dereferences attach (NULL) at
attach->dmabuf while obj->resv is held, and fails to free msm_obj->sgt:

drivers/gpu/drm/drm_prime.c:drm_prime_gem_destroy()
	attach = obj->import_attach;
	if (sg)
		dma_buf_unmap_attachment_unlocked(attach, sg, DMA_BIDIRECTIONAL);
	dma_buf = attach->dmabuf;

Should the error path instead free msm_obj->sgt with
sg_free_table(msm_obj->sgt) and kfree(msm_obj->sgt) to match put_pages()?

>  	msm_obj->sgt = NULL;
>  
>  	drm_gem_put_pages(obj, msm_obj->pages, false, false);
> @@ -260,6 +271,11 @@ static void put_pages(struct drm_gem_object *obj)
>  		if (msm_obj->flags & MSM_BO_WC)
>  			sync_for_cpu(msm_obj);
>  
> +		dma_unmap_sgtable(drm_dev_dma_dev(obj->dev),
> +				  msm_obj->sgt,
> +				  DMA_BIDIRECTIONAL,
> +				  DMA_ATTR_SKIP_CPU_SYNC);

[Severity: High]
Does this map and unmap msm_obj->sgt twice whenever MSM_BO_WC is set?

In get_pages(), right after dma_map_sgtable() maps msm_obj->sgt with
DMA_ATTR_SKIP_CPU_SYNC, objects with MSM_BO_WC call sync_for_device():

drivers/gpu/drm/msm/msm_gem.c:get_pages()
		if (msm_obj->flags & MSM_BO_WC)
			sync_for_device(msm_obj);

And in put_pages(), before the new dma_unmap_sgtable() call, MSM_BO_WC
objects call sync_for_cpu(). However, sync_for_device() and sync_for_cpu()
themselves call dma_map_sgtable() and dma_unmap_sgtable() on the same
msm_obj->sgt and device:

drivers/gpu/drm/msm/msm_gem.c:
static void sync_for_device(struct msm_gem_object *msm_obj)
{
	struct device *dev = msm_obj->base.dev->dev;

	dma_map_sgtable(dev, msm_obj->sgt, DMA_BIDIRECTIONAL, 0);
}

static void sync_for_cpu(struct msm_gem_object *msm_obj)
{
	struct device *dev = msm_obj->base.dev->dev;

	dma_unmap_sgtable(dev, msm_obj->sgt, DMA_BIDIRECTIONAL, 0);
}

On platforms where the top-level DRM device uses IOMMU DMA ops or SWIOTLB,
the second dma_map_sgtable() call in sync_for_device() overwrites
sg_dma_address and sg_dma_len, leaking the first IOVA or bounce mapping,
while put_pages() unmaps and frees the second range twice. With
CONFIG_DMA_API_DEBUG enabled, this also triggers overlapping mapping and
double unmap warnings.

Should sync_for_device() and sync_for_cpu() use dma_sync_sgtable_for_device()
and dma_sync_sgtable_for_cpu() (or fold the CPU sync attribute into the map
and unmap calls in get_pages() and put_pages()) instead of mapping and
unmapping msm_obj->sgt a second time?

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