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

From: Julien Grall <julien@xen.org>
To: Shawn Anastasio <sanastasio@raptorengineering.com>,
	xen-devel@lists.xenproject.org
Cc: Timothy Pearson <tpearson@raptorengineering.com>,
	Jan Beulich <jbeulich@suse.com>,
	Stefano Stabellini <sstabellini@kernel.org>
Subject: Re: [PATCH v2 4/7] xen/device-tree: Fix bootfdt.c to tolerate 0 reserved regions
Date: Tue, 9 Jan 2024 18:14:56 +0000	[thread overview]
Message-ID: <c1c6ee29-7fa1-4b39-aa7a-1b8f750cd893@xen.org> (raw)
In-Reply-To: <1c6f0f94f4ea2b773f960d88bd02e2168ac28abb.1702607884.git.sanastasio@raptorengineering.com>

(+ 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.

> 
> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
> ---
>   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.

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

Cheers,

-- 
Julien Grall