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
