Re: [PATCH v2 4/7] xen/device-tree: Fix bootfdt.c to tolerate 0 reserved regions

[Michal Orzel <michal.orzel@xxxxxxx>]

On 09/01/2024 19:14, Julien Grall wrote:
> 
> 
> (+ Stefano)
> 
> Hi Shawn,
> 
> On 15/12/2023 02:43, Shawn Anastasio wrote:
>> The early_print_info routine in bootfdt.c incorrectly stores the result
>> of a call to fdt_num_mem_rsv() in an unsigned int, which results in the
>> negative error code being interpreted incorrectly in a subsequent loop
>> in the case where the device tree does not contain any memory reserve
>> map entries.
> 
> I have some trouble to reconciliate the code with your explanation.
> Looking at the implementation fdt_num_mem_rsv() should return 0 if there
> are no reserved regions. A negative value would only be returned if the
> device-tree is malformated.
I agree with Julien. The function takes an offset to reserve map and grabs 
blocks of type fdt_reserve_entry
from there. In case of no regions, there will be one entry with addr/size 0 
which always acts as a termination region.
The only way to return < 0 is when you have a buggy FDT.

> 
> Do you have a Device-Tree where the issue occurs?
> 
> That said, I agree that the code could be hardened.
> 
>>
>> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
>> ---
>>   xen/common/device-tree/bootfdt.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/common/device-tree/bootfdt.c 
>> b/xen/common/device-tree/bootfdt.c
>> index ae9fa1e3d6..796ac01c18 100644
>> --- a/xen/common/device-tree/bootfdt.c
>> +++ b/xen/common/device-tree/bootfdt.c
>> @@ -466,7 +466,8 @@ static void __init early_print_info(void)
>>       struct meminfo *mem_resv = &bootinfo.reserved_mem;
>>       struct bootmodules *mods = &bootinfo.modules;
>>       struct bootcmdlines *cmds = &bootinfo.cmdlines;
>> -    unsigned int i, j, nr_rsvd;
>> +    unsigned int i, j;
>> +    int nr_rsvd;
>>
>>       for ( i = 0; i < mi->nr_banks; i++ )
>>           printk("RAM: %"PRIpaddr" - %"PRIpaddr"\n",
>> @@ -481,7 +482,7 @@ static void __init early_print_info(void)
>>                   boot_module_kind_as_string(mods->module[i].kind));
>>
>>       nr_rsvd = fdt_num_mem_rsv(device_tree_flattened);
> 
> If I am correct above, then I think we should panic() rather than trying
> to continue with a buggy DT.
+1. Furthermore, we already call panic in such case in dt_unreserved_regions().

> 
>> -    for ( i = 0; i < nr_rsvd; i++ )
>> +    for ( i = 0; nr_rsvd > 0 && i < nr_rsvd; i++ )
>>       {
>>           paddr_t s, e;
>>
> 
> Cheers,
> 
> --
> Julien Grall
> 

~Michal