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, so
combined with the great urgency to get rid of scatterlists, I hardly see
it being a big deal... This will always have returned an error due to
the "ret < iova_len" condition at the end, the fix just means we can now
error out earlier without passing a mangled list to iommu_map_sg() and
corrupting the pagetable in the process.
Certainly if a list was initially malformed with an individual segment
longer than the segment boundary mask then it's not going to do any
favours 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. Note the
off-by-an-exact-multiple-of-4GB error in Krzysztof's logging from the v1
thread:
[ 79.915571] iommu_dma_map_sg: ret = 7138717696, iova_len = 20023619584
Quite simply this code was never written with the expectation of mapping
4GB+ of _physically contiguous_ memory in one go. We could possibly try
adding a bunch of special-case complexity to allow treating UINT_MAX as
4GB through iommu_map_sg() if there was a real need to make something
which never worked start working, but going back to my initial point, it
really doesn't seem worth the bother.
Thanks,
Robin.
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 {
+ prev->length += pad_len;
+ iova_len += pad_len;
+ }
It is better to use
check_add_overflow(prev->length, pad_len, &prev->length)
instead of overflows_type()..
Jason
