Re: [PATCH v5 3/6] iommu/dma: Catch scatterlist length overflows

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

Pre-existing issues:
- [High] Error path `out_restore_sg` corrupts unprocessed elements of reused scatterlists.
- [High] Accumulator `iova_len` silently overflows on 32-bit systems for mappings exceeding 4GB.
--

--- Patch [3]: [PATCH v5 3/6] iommu/dma: Catch scatterlist length overflows ---
commit 955c8f50877d1e47058e0056fd63e5f20f4d62b9
Author: Krzysztof Karas <[email protected]>
Subject: iommu/dma: Catch scatterlist length overflows

This commit introduces overflow detection for scatterlist length fields to
prevent silent wrapping when very large mappings use only one scatterlist.
It adds check_add_overflow() and overflows_type() checks to
iommu_dma_map_sg(), failing early if limits are exceeded.

> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 9a07eb39336eb..61cb64bcbd3cc 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
[ ... ]
> @@ -1476,7 +1478,22 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
>  		sg_dma_address(s) = s_iova_off;
>  		sg_dma_len(s) = s_length;
>  		s->offset -= s_iova_off;
> -		s_length = iova_align(iovad, s_length + s_iova_off);
> +
> +		if (check_add_overflow(s_length, s_iova_off, &s_length_tmp)) {
> +			ret = -EOVERFLOW;
> +			goto out_restore_sg;
> +		}

[Severity: High]
This isn't a bug introduced by this patch, but these newly added early return
paths make it much more likely to trigger a pre-existing issue in
__invalidate_sg().

If a driver reuses a scatterlist without calling sg_init_table(), and the
subsequent mapping fails here during element iteration, __invalidate_sg() will
loop over all elements rather than just the processed ones.

For unvisited elements, sg_dma_address(s) retains the stale IOVA from a
previous mapping. __invalidate_sg() erroneously adds this stale IOVA to
s->offset:

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

Will this corrupted offset lead to wild physical address calculations in
sg_phys() upon retry or buffer access, resulting in arbitrary DMA memory
corruption?

[ ... ]
> @@ -1493,7 +1510,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_prev_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_prev_len)) {
> +				ret = -EOVERFLOW;
> +				goto out_restore_sg;
> +			}
> +
> +			prev->length = new_prev_len;
>  			iova_len += pad_len;

[Severity: High]
This is a pre-existing issue, but since the patch addresses scatterlist length
overflows, could iova_len silently overflow here on 32-bit systems for
mappings exceeding 4GB?

If the size_t iova_len accumulator wraps around when adding pad_len or
s_length, it would lead to an undersized IOVA allocation. iommu_map_sg()
would then map the full physical memory range, overrunning the allocated IOVA
space and corrupting IOMMU page tables.

Should there be overflow checks for iova_len accumulation as well?

>  		}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3