Hello,
On 2026-08-17 at 10:11:10 +0000, [email protected] wrote:
> 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?
Fixing this issue is outside the scope of this patch series and
will be addressed separately.
>
> [ ... ]
> > @@ -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?
Changing this is outside of the scope of this series and this
patch already got r-b from Robin without this check, so it is
not desired.
>
> 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
--
Best Regards,
Krzysztof
