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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The newly added scatterlist length overflow check `(s_length & UINT_MAX) == 0` is mathematically flawed. It fails to catch integer overflow on both 32-bit and 64-bit systems, leaving the critical truncation bug unpatched, and introduces a regression by falsely rejecting legitimate zero-length mappings.
--

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

iommu/dma: Catch scatterlist length overflows

It is possible, when a very large mapping uses only one scatterlist,
that padding overflows scatterlist's length field. This results in
silently wrapping the value, smaller than desired mappings, and leaving
mapped bytes in memory. This commit addresses this issue by adding
overflow detection.

> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 9abaec0703efb..5be5d1f3c9f82 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -1477,6 +1478,12 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
>  		sg_dma_len(s) = s_length;
>  		s->offset -= s_iova_off;
>  		s_length = iova_align(iovad, s_length + s_iova_off);
> +		if ((s_length & UINT_MAX) == 0) {

[Severity: Critical]
Does this check actually catch integer overflows?

On 64-bit systems, s_length is a size_t, so this bitwise AND merely checks
if the lowest 32 bits are exactly zero. It will fail to detect an overflow
unless the aligned size is an exact multiple of 4GB.

On 32-bit systems, s_length + s_iova_off wraps before this check, so
s_length would just be a small wrapped value and bypass the check entirely.

Additionally, won't this incorrectly reject legitimate zero-length mappings?

If the overflow check is bypassed on a 64-bit system, s_length is truncated
to 32 bits when assigned to s->length below. iommu_map_sg() will then map
the truncated size while iova_len accumulates the full 64-bit size. The
partial mapping will trigger the error path later in the function:

	ret = iommu_map_sg(domain, iova, sg, nents, prot, GFP_ATOMIC);
	if (ret < 0 || ret < iova_len)
		goto out_free_iova;

	return __finalise_sg(dev, sg, nents, iova);

out_free_iova:
	iommu_dma_free_iova(domain, iova, iova_len, NULL);

This frees the IOVA back to the allocator without calling iommu_unmap(),
leaving the pages mapped and potentially causing device isolation breakdown
when the IOVA is reallocated.

> +			/* Prevent possible wrapping upon assignment to s->length. */
> +			ret = -EOVERFLOW;
> +			goto out_restore_sg;
> +		}
> +
>  		s->length = s_length;

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