Re: [PATCH v3 2/5] iommu/dma: Catch scatterlist length overflows

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] `__invalidate_sg()` corrupts unprocessed scatterlist segments on mapping failure if the scatterlist was previously mapped.
- [High] Silent truncation of `s->length` during IOVA alignment leads to IOMMU page table memory leak.
--

--- Patch [2]: [PATCH v3 2/5] iommu/dma: Catch scatterlist length overflows ---
commit 9f9c6b9481d1f01d396391f99642a7f233aa6651
Author: Krzysztof Karas <krzysztof.karas@intel.com>

iommu/dma: Catch scatterlist length overflows

This commit adds an overflow detection for the scatterlist length field
padding in iommu_dma_map_sg(). When a very large mapping uses only one
scatterlist, padding could overflow the length field resulting in small
mappings and left-over bytes in memory.

> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 9abaec0703efb..39da61b4e549e 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
[ ... ]
> @@ -1493,7 +1493,18 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
>  		 *   time through here (i.e. before it has a meaningful value).
>  		 */
>  		if (pad_len && pad_len < s_length - 1) {
> -			prev->length += pad_len;
> +			unsigned int new_pad_len;
> +			/*
> +			 * For large mappings spanning multiple GBs we
> +			 * may not be able to fit all needed padding into
> +			 * sg->length.
> +			 */
> +			if (check_add_overflow(prev->length, pad_len, &new_pad_len)) {
> +				ret = -EOVERFLOW;
> +				goto out_restore_sg;

[Severity: High]
This is a pre-existing issue, but does jumping to out_restore_sg here
corrupt unprocessed scatterlist segments if the list was previously mapped?

If a driver maps, unmaps, and then re-maps a scatterlist, the unmap step
does not reset sg_dma_address() to DMA_MAPPING_ERROR. When this new error
path triggers, __invalidate_sg() will iterate over all segments. For the
unprocessed segments that still contain valid DMA addresses from the previous
mapping, __invalidate_sg() will incorrectly assume they were modified:

drivers/iommu/dma-iommu.c:__invalidate_sg() {
    ...
			if (sg_dma_address(s) != DMA_MAPPING_ERROR)
				s->offset += sg_dma_address(s);
			if (sg_dma_len(s))
				s->length = sg_dma_len(s);
    ...
}

Could this overwrite the physical boundaries of the segments, potentially
leading to memory corruption on subsequent DMA operations?

> +			}
> +
> +			prev->length = new_pad_len;
>  			iova_len += pad_len;
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but while handling large mappings
spanning multiple GBs, can s->length be silently truncated earlier in this
loop?

Before the padding step, the size_t s_length is aligned and assigned back to
s->length (which is a 32-bit unsigned int):

drivers/iommu/dma-iommu.c:iommu_dma_map_sg() {
    ...
		s_length = iova_align(iovad, s_length + s_iova_off);
		s->length = s_length;
    ...
}

If the aligned size_t exceeds UINT_MAX, it truncates. The iova_len
calculation uses the non-truncated size_t, allocating correct IOVA space,
but iommu_map_sg() will use the truncated s->length, mapping fewer bytes.

Since iommu_map_sg() succeeds in mapping the smaller size, it returns a
positive value. This triggers the error path:

drivers/iommu/dma-iommu.c:iommu_dma_map_sg() {
    ...
	ret = iommu_map_sg(domain, iova, sg, nents, prot, GFP_ATOMIC);
	if (ret < 0 || ret < iova_len)
		goto out_free_iova;
    ...
out_free_iova:
	iommu_dma_free_iova(domain, iova, iova_len, NULL);
    ...
}

Because out_free_iova only frees the IOVA allocator region and does not call
iommu_unmap(), does this permanently leak the hardware IOMMU page table
entries?

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