Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] `__invalidate_sg()` corrupts unprocessed scatterlist segments on mapping failure if the scatterlist was previously mapped. - [High] Silent truncation of `s->length` during IOVA alignment leads to IOMMU page table memory leak. -- --- Patch [2]: [PATCH v3 2/5] iommu/dma: Catch scatterlist length overflows --- commit 9f9c6b9481d1f01d396391f99642a7f233aa6651 Author: Krzysztof Karas <[email protected]> iommu/dma: Catch scatterlist length overflows This commit adds an overflow detection for the scatterlist length field padding in iommu_dma_map_sg(). When a very large mapping uses only one scatterlist, padding could overflow the length field resulting in small mappings and left-over bytes in memory. > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c > index 9abaec0703efb..39da61b4e549e 100644 > --- a/drivers/iommu/dma-iommu.c > +++ b/drivers/iommu/dma-iommu.c [ ... ] > @@ -1493,7 +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; > + unsigned int new_pad_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_pad_len)) { > + ret = -EOVERFLOW; > + goto out_restore_sg; [Severity: High] This is a pre-existing issue, but does jumping to out_restore_sg here corrupt unprocessed scatterlist segments if the list was previously mapped? If a driver maps, unmaps, and then re-maps a scatterlist, the unmap step does not reset sg_dma_address() to DMA_MAPPING_ERROR. When this new error path triggers, __invalidate_sg() will iterate over all segments. For the unprocessed segments that still contain valid DMA addresses from the previous mapping, __invalidate_sg() will incorrectly assume they were modified: drivers/iommu/dma-iommu.c:__invalidate_sg() { ... if (sg_dma_address(s) != DMA_MAPPING_ERROR) s->offset += sg_dma_address(s); if (sg_dma_len(s)) s->length = sg_dma_len(s); ... } Could this overwrite the physical boundaries of the segments, potentially leading to memory corruption on subsequent DMA operations? > + } > + > + prev->length = new_pad_len; > iova_len += pad_len; > } [Severity: High] This isn't a bug introduced by this patch, but while handling large mappings spanning multiple GBs, can s->length be silently truncated earlier in this loop? Before the padding step, the size_t s_length is aligned and assigned back to s->length (which is a 32-bit unsigned int): drivers/iommu/dma-iommu.c:iommu_dma_map_sg() { ... s_length = iova_align(iovad, s_length + s_iova_off); s->length = s_length; ... } If the aligned size_t exceeds UINT_MAX, it truncates. The iova_len calculation uses the non-truncated size_t, allocating correct IOVA space, but iommu_map_sg() will use the truncated s->length, mapping fewer bytes. Since iommu_map_sg() succeeds in mapping the smaller size, it returns a positive value. This triggers the error path: drivers/iommu/dma-iommu.c:iommu_dma_map_sg() { ... ret = iommu_map_sg(domain, iova, sg, nents, prot, GFP_ATOMIC); if (ret < 0 || ret < iova_len) goto out_free_iova; ... out_free_iova: iommu_dma_free_iova(domain, iova, iova_len, NULL); ... } Because out_free_iova only frees the IOVA allocator region and does not call iommu_unmap(), does this permanently leak the hardware IOMMU page table entries? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
