Re: [PATCH v2 3/3] drivers/iommu: Catch scatterlist length overflows

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The newly added `overflows_type` check fails to detect integer overflow on 32-bit architectures.
- [Medium] Explicitly assigned error code `-EOVERFLOW` is dead code because it is unconditionally clobbered by the cleanup path.
--

commit 97ce5598e53bd4b56cac94c57bc68e63d7d39019
Author: Krzysztof Karas <krzysztof.karas@intel.com>

drivers/iommu: Catch scatterlist length overflows

This commit adds an overflow check to prevent scatterlist length fields
from silently wrapping when adding padding for large mappings. This helps
avoid truncated mappings and memory leaks in the IOMMU layer.

> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index e8d4c2dac4da8..02ff84e9c0427 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -1493,8 +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;
> -			iova_len += pad_len;
> +			if (overflows_type(prev->length + pad_len, prev->length)) {

[Severity: High]
Does this check work correctly on 32-bit architectures?

Because prev->length + pad_len is evaluated before the macro checks it, on a
32-bit architecture this addition will silently wrap before overflows_type
receives it. The macro would then check if the already-wrapped 32-bit value
fits into a 32-bit integer, which trivially succeeds.

Would it be better to use check_add_overflow(prev->length, pad_len,
&prev->length) here instead?

> +				/*
> +				 * For large mappings spanning multiple GBs we
> +				 * may not be able to fit all needed padding into
> +				 * sg->length.
> +				 */
> +				ret = -EOVERFLOW;
> +				goto out_restore_sg;

[Severity: Medium]
Will this -EOVERFLOW error code ever actually be returned?

Looking at the error handling path at the end of iommu_dma_map_sg():

out_restore_sg:
	__invalidate_sg(sg, nents);
out:
	if (ret != -ENOMEM && ret != -EREMOTEIO)
		return -EINVAL;
	return ret;

It appears the explicitly assigned -EOVERFLOW is unconditionally clobbered
and replaced by -EINVAL.

> +			} else {
> +				prev->length += pad_len;
> +				iova_len += pad_len;
> +			}
>  		}
>  
>  		iova_len += s_length;

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