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 <[email protected]> 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/[email protected]?part=2
