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

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

Hi Julien

On 1/9/24 12:14 PM, 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.
> 
> Do you have a Device-Tree where the issue occurs?
> 
> That said, I agree that the code could be hardened.
>

After reading your comment, I looked into it again and it appears you're
right. It turns out that I was hitting this issue not because my device
tree had 0 entries or was corrupt, but because the function was being
called with an invalid device tree pointer to begin with!

Specifically, bootfdt.c:early_print_info calls fdt_num_mem_rsv using the
global `device_tree_flattened` variable which PPC's setup.c did not
properly initialize:

    nr_rsvd = fdt_num_mem_rsv(device_tree_flattened);

Thanks for the sanity check. I'll fix this in the next revision of the
patchset.

Despite my misunderstanding of how the bug was triggered in my case, it
definitely is still something that should be fixed. Following yours and
Michal's suggestion, I'll change the patch to panic() instead and submit
a follow-up (likely standalone and not as a part of this series).

> Cheers,

Thanks,
Shawn