Re: [PATCH v2 3/3] drivers/iommu: Catch scatterlist length overflows

[Robin Murphy <robin.murphy@arm.com>]

On 2026-07-03 9:35 pm, Jason Gunthorpe wrote:
> On Fri, Jul 03, 2026 at 07:58:32PM +0100, Robin Murphy wrote:
>> On 03/07/2026 5:22 pm, Jason Gunthorpe wrote:
>>> On Wed, Jul 01, 2026 at 10:44:37AM +0000, 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.
>>>
>>> Urk, this is unfortunate, it means we cannot map certain kinds of
>>> scatterlists? Meaning there is a condition that makes a scatterlist
>>> ill formed?
>>>
>>> This seems like something that needs to be more clearly documented and
>>> we need to ensure at least the common scatterlist builders don't hit
>>> it..
>>
>> Well, it's taken 10 years to be caught by a test which seemingly expects the
>> mapping of a single absurdly giant scatterlist to fail anyway,
> 
> If your server has 0.5TB of ram a 4G IO isn't actually that large.
> Randomly getting a few contiguous 1G hugetlbfs pages is not even that
> unlikely. Something like FSDAX has a very high chance of getting high
> contiguity pages in files.

Sure, but how many servers had 0.5TB of RAM in 2015? And how many of 
those were running the arm64 DMA mapping code? As I said, both this 
merging logic and the iommu_map_sg() interface itself were essentially 
written to support media buffers on Android phones which didn't even 
have 4GB of RAM in total. Yes, things have moved on by now, but 
correspondingly it has also been decided that the ~30-year-old 
scatterlist design also doesn't scale to modern use-cases anyway, and is 
being replaced, so limitations of a "legacy" API that don't have any 
meaningful impact to its existing users are hardly something to panic 
about. If DRM does want to be able to *reliably* map massive amounts of 
RAM then it can adopt the new IOMMU API, for this and all the other 
reasons that that new API was promised to be "better".

> So I wouldn't be quite so dismissive that this is not something a real
> user can hit.

I'm not being dismissive - clearly it can be hit. My point is that 
anyone who *does* hit it can only expect it to fail (as indeed this 
particular IGT test seems to), because it has never worked. You cannot 
encode a 4GB scatterlist segment, because it overflows UINT_MAX. And if 
you did try to bodge it and pass a UINT_MAX length segment to 
iommu_map_sg() then that will also fail because it's not aligned to an 
IOMMU page size. It's purely the matter of *how* exactly it fails which 
could do with fixing.

>> here, but I suspect this is likely just regular iova_granule rounding
>> overflowing when the segment boundary is the maximum 4GB, since the largest
>> representable segment length is 4GB - 1.
> 
> It looks like the iommu_dma_map_sg() algorithm only works reliably if
> the scatterlist entry size is less than UINT_MAX/2, otherwise it can
> risk overflowing when it pads.

AFAICS, the overflow can only happen with a 4GB boundary mask, and for 
two conditions:

- rounding up the current segment, when s->length + s->offset > 4GB - 
iova_granule (but still <= 4GB otherwise it's bogus anyway)
- padding the previous segment in the case where the current segment 
would otherwise cross the next boundary, when prev->offset = 0 and 
prev->length + s->length >= 4GB

So yes, limiting any individual segment to <=2GB would end up avoiding 
both those conditions, but it would also impact plenty of cases that 
*do* currently work fine, e.g. 1GB+3GB+3GB. The limitation is really 
that you can't have two consecutive segments where the first starts 
exactly on a 4GB boundary and the sum of both their sizes >=4GB.

> API wise I expect any arbitary input to sg_alloc_table_from_pages() to
> result in a scatterlist that iommu_dma_map_sg() will map. This
> patch highlights there are cornere cases where that isn't true, it
> should be fixed..

Technically sg_alloc_table_from_pages() carries no such assumption, only 
sg_alloc_table_from_pages_segment() (or __sg_alloc_table_from_pages()) 
with the correct dma_seg_boundary value for the given device. But even 
then in the worst case, they should still end up splitting segments at 
4GB-PAGE_SIZE due to the fundamental int limitation, and so only be at 
risk of putting two such segments back-to-back.

> I agree we shouldn't overcomplicate iommu_dma_map_sg(), so the
> simplest fix is to introduce a SG_MAX_LENGTH set to UINT_MAX/2,
> justified by the logic in iommu_dma_map_sg(). Fixup the core sg_alloc
> code to respect that. WARN_ON in iommu_dma_map_sg() if a malformed
> scatterlist entry is presented. Add a WARN_ON under DMA debugging
> kconfig as well for the physical path.

Again, it's not "malformed", it's just an edge case of certain 
otherwise-valid scatterlist layouts that are not supported in this one 
DMA API implementation. Nothing in the DMA API ever guarantees that any 
particular mapping must succeed. Furthermore I don't see that anyone's 
asking for this to actually be supported, just to fail cleanly and 
correctly without inadvertently corrupting state.

Thanks,
Robin.