Re: [PATCH v5 04/13] iommu: of_iommu: Add support for "iommu-ranges" on a device node

[Vishnu Reddy <busanna.reddy@oss.qualcomm.com>]

On 10/8/2026 3:30 PM, Dmitry Baryshkov wrote:
> On Sat, Sep 26, 2026 at 01:23:06PM +0530, Vishnu Reddy wrote:
>>
>> On 9/26/2026 12:17 PM, sashiko-bot@kernel.org wrote:
>>> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>>> - [High] iommu: size_t truncation of derived reserved region length on 32-bit systems
>>> - [Medium] iommu: DT ABI breakage by rejecting iommu-addresses without reg
>>> - [Medium] iommu: OOB read in of_iommu_derive_resv_regions when parsing malformed iommu-ranges
>>> --
>>>
>>> commit 08a09989ffefcbf5d38171159dcc911ca993ff18
>>> Author: Vikash Garodia <vikash.garodia@oss.qualcomm.com>
>>>
>>> iommu: of_iommu: Add support for "iommu-ranges" on a device node
>>>
>>> Add support for "iommu-ranges" on device nodes to specify usable IOVA
>>> ranges directly when there is no backing "reg". Introduce logic to derive
>>> reserved regions outside of "iommu-ranges" and restrict "iommu-addresses"
>>> to be paired with a "reg" property.
>>>
>>> Link: https://lore.kernel.org/all/662f7093-fb0a-4564-9ca0-98e03e68ba9a@kernel.org
>>>
>>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
>>>> index a18bb60f6f3df..9766a6944d0e1 100644
>>>> --- a/drivers/iommu/of_iommu.c
>>>> +++ b/drivers/iommu/of_iommu.c
>>> [ ... ]
>>>> +static void of_iommu_derive_resv_regions(struct device *dev, struct list_head *list)
>>>> +{
>>> [ ... ]
>>>> +	maps = of_get_property(dev->of_node, "iommu-ranges", &size);
>>>> +	if (!maps)
>>>> +		return;
>>>> +
>>>> +	end = maps + size / sizeof(__be32);
>>>> +
>>>> +	while (maps < end) {
>>>> +		phys_addr_t iova, iova_end;
>>>> +		size_t length;
>>>> +
>>>> +		maps = of_translate_dma_region(dev->of_node, maps, &iova, &length);
>>> [Severity: Medium]
>>> Could this result in an out-of-bounds read if the "iommu-ranges" property is
>>> malformed?
>>>
>>> The loop condition checks `maps < end`, but of_translate_dma_region()
>>> implicitly reads `na + ns` cells from the maps array.
>>>
>>> Does this loop need to ensure there are at least `na + ns` cells remaining
>>> before calling the translation function to prevent reading past the buffer?
>>>
>> This function intention is not to validate the address and size cells here and
>> it's not valid to read those properties here. If the out of bound ranges are
>> there, then it's part of dtc checks that dt node contains invalid ranges.
>>
>>> [ ... ]
>>>> +	list_for_each_entry_safe(pos, next_range, &ranges, node) {
>>>> +		if (pos->start > next) {
>>>> +			region = iommu_alloc_resv_region(next, pos->start - next, prot,
>>>> +							 IOMMU_RESV_RESERVED, GFP_KERNEL);
>>> [Severity: High]
>>> Can the length argument overflow on 32-bit systems?
>>>
>>> On 32-bit systems with 64-bit physical addresses (like ARM LPAE), address
>>> differences can easily exceed 4GB. Because iommu_alloc_resv_region() takes
>>> a 32-bit size_t length, passing a 64-bit gap size causes it to truncate.
>>>
>>> Will this truncation result in a reservation of at most 4GB, leaving the
>>> IOVA address space above that unprotected?
>>>
>>> [ ... ]
>>>> +	region = iommu_alloc_resv_region(next, PHYS_ADDR_MAX - next + 1,
>>>> +					 prot, IOMMU_RESV_RESERVED, GFP_KERNEL);
>>> [Severity: High]
>>> Similarly, can passing `PHYS_ADDR_MAX - next + 1` here truncate the length
>>> to its lower 32 bits on 32-bit systems?
>>>
>>> If the remaining address space exceeds 4GB, this truncation could leave high
>>> IOVA addresses unprotected, allowing the IOMMU allocator to hand out reserved
>>> IOVAs.
>>>
>> size_t can hold up to 32 bit range in LPAE platforms. The relevant driver has to
>> set the dma_limit max range in that case.
> As this is generic code, could you please adapt the code to work
> correctly on LPAE systems?

This is no different than the existing logic where client tries to reserve
greater than 32bit length on the 32bit LPAE platforms.

I'm not convinced why sashiko claims this is introduced in this patch.

>