Re: [PATCH 1/3] drivers/iommu: Unroll unsuccessful mapping

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] iommu_map_sg() fails to map accumulated scatterlist segments if they are followed by a physically contiguous bus address segment, leading to silent partial mapping drops. The patch mitigates the resulting resource leak in iommu_dma_map_sg() but leaves this root cause unaddressed.
--

commit 28ae7323f36f7853e142160b68651789c5fed2b2
Author: Krzysztof Karas <krzysztof.karas@intel.com>

drivers/iommu: Unroll unsuccessful mapping

This commit addresses an issue where partially successful IOMMU mappings
could leave untouched bytes unmapped, potentially leading to memory leaks.
It updates iommu_dma_map_sg() to unmap any partially mapped bytes before
freeing the IOVA and returning an error.

> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index e8d4c2dac4da8..78a12c12b5eae 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -1515,8 +1515,14 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
>  	 * implementation - it knows better than we do.
>  	 */
>  	ret = iommu_map_sg(domain, iova, sg, nents, prot, GFP_ATOMIC);
> -	if (ret < 0 || ret < iova_len)
> +	if (ret < 0 || ret < iova_len) {
> +		if (ret > 0) {
> +			/* Unmap partially mapped bytes before freeing IOVA */
> +			if (iommu_unmap(domain, iova, ret) != ret)
> +				ret = -EIO;
> +		}
>  		goto out_free_iova;
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but while this mitigates the
resulting resource leak in iommu_dma_map_sg() when a short map occurs, does
the underlying cause of the short map still exist in iommu_map_sg()?

Looking at iommu_map_sg() in drivers/iommu/iommu.c:

	while (i <= nents) {
		phys_addr_t s_phys = sg_phys(sg);

		if (len && s_phys != start + len) {
			ret = iommu_map_nosync(domain, iova + mapped, start,
					len, prot, gfp);
			if (ret)
				goto out_err;

			mapped += len;
			len = 0;
		}

		if (sg_dma_is_bus_address(sg))
			goto next;

If a normal scatterlist segment is followed by a P2PDMA bus address segment
that happens to be physically contiguous with it (s_phys == start + len),
the flush condition is bypassed. The bus segment is then skipped via goto next,
leaving the accumulated len of the normal segment trapped.

If this happens at the end of the list, the normal segment is never flushed
and silently dropped, leading to a short map where mapped < total_len.

Could this lead to valid DMA mapping requests failing or devices performing
DMA into unmapped IOVA space if callers don't strictly validate the mapped
length?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625134342.1102921-1-krzysztof.karas@intel.com?part=1