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