Hi Robin,
thanks for a quick response!
On 2026-07-01 at 12:59:29 +0100, Robin Murphy wrote:
> On 01/07/2026 11:44 am, Krzysztof Karas wrote:
> > It is possible, when a very large mapping uses a single
> > scatterlist, that padding overflows scatterlist's length field.
> > This results in:
> > 1) silently wrapping the value
> > 2) smaller than desired mappings produced by iommu_map_sg
> > 3) leaving mapped bytes in memory (no iommu_unmap)
> >
> > Address this issue by adding overflow detection for previous
> > scatterlist length field.
>
> Awesome, thanks for figuring it out! Looks like this must date all the way
> back:
>
> Fixes: 809eac54cdd6 ("iommu/dma: Implement scatterlist segment merging")
Okay, thanks for pinpointing this commit, I'll add the tag in
the next version of this series.
>
> > Signed-off-by: Krzysztof Karas <[email protected]>
> > ---
> > v2:
> > * Address overflows instead of unmapping erroneously mapped
> > memory (Robin).
> > * Put this patch last for easier reproduction of the issue.
> >
> > drivers/iommu/dma-iommu.c | 14 ++++++++++++--
> > 1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index 9abaec0703ef..c403057577df 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)) {
> > + /*
> > + * 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;
> > + } else {
>
> Nit: we don't really need an "else" after a goto, but it's hardly a big deal
> (however if you did want to respin, note also that the preferred title tag
> here is "iommu/dma: ...").
I'll be making another version anyway, so I can remove the
"else" and change the title :)
> Either way,
>
> Reviewed-by: Robin Murphy <[email protected]>
Thanks!
would this r-b still hold after above minor changes, or would
you prefer to have one final look at the finished change and
then decide whether you give your r-b?
>
> I'd imagine Joerg can take this as an IOMMU fix, but FWIW if you did want an
> ack to take it through drm-fixes to keep it with the i915 patches, I
> wouldn't foresee any significant risk of conflicts.
>
> Thanks,
> Robin.
>
> > + prev->length += pad_len;
> > + iova_len += pad_len;
> > + }
> > }
> > iova_len += s_length;
>
--
Best Regards,
Krzysztof
