Re: [PATCH v3 2/6] 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 pointer dereference in get_pages() on dma_map_sgtable() error
- [High] drm/msm/gem: dma_map_sgtable() maps via iommu-dma when ddev->dev has iommus
--

commit 4102c003f615c93e1ee62bff745ef3edfcbebd16
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/gem: dma_map/unmap_sgtable()

This commit replaces the cache sync helpers in msm_gem with unconditional
dma_map_sgtable() and dma_unmap_sgtable() calls in get_pages() and
put_pages() to populate scatterlist dma addresses and lengths. It also adds
sgtable cleanup to the error path in get_pages().

> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 181b9ad326729..c97395e4ea2be 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c

[ ... ]

> @@ -219,13 +192,20 @@ static struct page **get_pages(struct drm_gem_object *obj)
>  			goto error;
>  		}
>  
> -		update_device_mem(dev->dev_private, obj->size);
> -
> -		/* For non-cached buffers, ensure the new pages are clean
> -		 * because display controller, GPU, etc. are not coherent:
> -		 */
> +		/* For non-cached buffers, ensure the new pages are clean. */
>  		if (msm_obj->flags & MSM_BO_WC)
> -			sync_for_device(msm_obj);
> +			map_attrs = 0;
> +
> +		ret = dma_map_sgtable(drm_dev_dma_dev(obj->dev),
> +				      msm_obj->sgt,
> +				      DMA_BIDIRECTIONAL,
> +				      map_attrs);

[Severity: High]
Does calling dma_map_sgtable() on drm_dev_dma_dev(obj->dev) always take the
dma-direct path when dma_map_ops is null?

In kernel/dma/mapping.c:dma_go_direct(), use_dma_iommu(dev) is checked before
testing whether ops is null:

kernel/dma/mapping.c:dma_go_direct() {
	if (use_dma_iommu(dev))
		return false;

	if (likely(!ops))
		return true;
    ...
}

On platforms where the iommus property in device tree is attached directly to
the mdp device (obj->dev->dev) rather than the parent mdss node (as handled
in drivers/gpu/drm/msm/msm_kms.c:msm_kms_init_vm() when checking
device_iommu_mapped(mdp_dev)), such as qcom_iommu platforms (msm8916,
msm8939, msm8953, etc.) and qcom,mdp5 arm-smmu-qcom platforms (msm8996,
sdm630, sdm660) that are not matched in qcom_smmu_client_of_match[], the
iommu core allocates a default dma domain and sets DEV_FLAG_DMA_IOMMU on
obj->dev->dev.

When msm_iommu_new() later attaches an unmanaged domain via
iommu_attach_device(), DEV_FLAG_DMA_IOMMU and group->default_domain remain on
the device. As a result, get_pages() -> dma_map_sgtable() ->
__dma_map_sg_attrs() -> iommu_dma_map_sg() maps every buffer into the unused
default_domain and populates sg_dma_address(sg) with default_domain dma
addresses instead of physical addresses.

When paired with the subsequent change in this series ("drm/msm: Convert map
helper to use dma-address") where __do_map() passes sg_dma_address(sg) as the
physical address to pgtable->map(), won't this program the gpu and display
page tables with default_domain dma addresses instead of cpu physical
addresses on those platforms?

> +		if (ret) {
> +			DRM_DEV_ERROR(dev->dev, "could not map sgtable: %d\n", ret);
> +			goto error;
> +		}
> +
> +		update_device_mem(dev->dev_private, obj->size);
>  
>  		update_lru(obj);
>  	}
> @@ -233,6 +213,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() in the get_pages() error path cause a
null pointer dereference and leak msm_obj->sgt when dma_map_sgtable() fails?

At the start of drivers/gpu/drm/msm/msm_gem.c:get_pages(), imported objects
are rejected:

	if (drm_WARN_ON_ONCE(obj->dev, drm_gem_is_imported(obj)))
		return ERR_PTR(-EINVAL);

so obj->import_attach is always null here, 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 pointer on a non-imported object:

drivers/gpu/drm/drm_prime.c:drm_prime_gem_destroy() {
	struct dma_buf_attachment *attach;
	struct dma_buf *dma_buf;

	attach = obj->import_attach;
	if (sg)
		dma_buf_unmap_attachment_unlocked(attach, sg, DMA_BIDIRECTIONAL);
	dma_buf = attach->dmabuf;
    ...
}

Since attach is null, dma_buf_unmap_attachment_unlocked() triggers a warning
and attach->dmabuf dereferences a null pointer while leaving msm_obj->sgt
unfreed. Should this error path free msm_obj->sgt using sg_free_table() and
kfree() as done in put_pages()?

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