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

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

On 02/07/2026 7:22 am, Krzysztof Karas wrote:
> 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 <krzysztof.karas@intel.com>
>>> ---
>>> 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 :)

Cool :)

>> Either way,
>>
>> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> 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?

Indeed if it's just those tweaks then feel free to keep it, that's 
usually what I mean by "either way".

Cheers,
Robin.

> 
>>
>> 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;
>>
>