Re: [PATCH v2 3/3] of: reserved_mem: only support one <base size> entry in reg property

[Wandun <chenwandun1@gmail.com>]

On 5/19/26 17:24, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Unconditional read of the first `reg` property entry without verifying that `len > 0` leads to an out-of-bounds FDT read for empty `reg` properties.
> --
>
> commit def0743ccda5f4a97f9b1275e989c8ea559363b4
> Author: Wandun Chen <chenwandun1@gmail.com>
>
> of: reserved_mem: only support one <base size> entry in reg property
>
> This commit simplifies reserved memory parsing by restricting support
> to a single base/size entry in the reg property. It removes the loop
> in the early pass and warns if multiple entries are present,
> unconditionally reading only the first entry.
>
> Link: https://lore.kernel.org/all/20260506014752.GA280279-robh@kernel.org/
>
>> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
>> index 462e7c3078a3a..0e91c4a71d89d 100644
>> --- a/drivers/of/of_reserved_mem.c
>> +++ b/drivers/of/of_reserved_mem.c
>> @@ -134,37 +134,38 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
> [ ... ]
>>   	prop = of_flat_dt_get_addr_size_prop(node, "reg", &len);
>>   	if (!prop)
>>   		return -ENOENT;
>>   
>> +	if (len > 1)
>> +		pr_warn("Reserved memory: node '%s' has %d <base size> entries, only the first is used\n",
>> +			uname, len);
>> +
>>   	nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
>>   
>>   	err = fdt_validate_reserved_mem_node(node, NULL);
>>   	if (err && err != -ENODEV)
>>   		return err;
>>   
>> -	for (i = 0; i < len; i++) {
>> -		u64 b, s;
>> -
>> -		of_flat_dt_read_addr_size(prop, i, &b, &s);
>> -
>> -		base = b;
>> -		size = s;
>> +	of_flat_dt_read_addr_size(prop, 0, &b, &s);
> If a device tree node contains an empty reg property,
> of_flat_dt_get_addr_size_prop() returns a valid pointer to the empty
> property data and sets len to 0.
>
> Since the loop was removed, is there a risk of an out-of-bounds read here
> because the code unconditionally calls of_flat_dt_read_addr_size()?
Yes, there is indeed an out-of-bounds read issue here, which will be
fixed in v3.
>
> This could fetch garbage values for the physical base and size. If the
> parsed garbage size happens to be non-zero, could the kernel attempt
> to reserve unintended physical memory regions via
> early_init_dt_reserve_memory()?
>
> Would it be safer to verify len > 0 before reading the first entry?
>
>> +	base = b;
>> +	size = s;
> [ ... ]
>> @@ -277,20 +278,24 @@ void __init fdt_scan_reserved_mem_late(void)
>>   	alloc_reserved_mem_array();
>>   
>>   	fdt_for_each_subnode(child, fdt, node) {
>> +		const __be32 *prop;
> [ ... ]
>> -		if (!of_flat_dt_get_addr_size(child, "reg", &b, &s))
>> +		prop = of_flat_dt_get_addr_size_prop(child, "reg", &len);
>> +		if (!prop)
>>   			continue;
>>   
>>   		ret = fdt_validate_reserved_mem_node(child, NULL);
>>   		if (ret && ret != -ENODEV)
>>   			continue;
>>   
>> +		of_flat_dt_read_addr_size(prop, 0, &b, &s);
> Does this have the same out-of-bounds read potential when len == 0?
>
> The previous of_flat_dt_get_addr_size() wrapper explicitly checked that
> entries != 1, which inherently skipped empty reg properties.
>
>>   		base = b;
>>   		size = s;
>>