| Message ID | 1c6f0f94f4ea2b773f960d88bd02e2168ac28abb.1702607884.git.sanastasio@raptorengineering.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Early Boot Allocation on Power | expand |
(+ 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,
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@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. +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
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
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); - for ( i = 0; i < nr_rsvd; i++ ) + for ( i = 0; nr_rsvd > 0 && i < nr_rsvd; i++ ) { paddr_t s, e;
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. Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com> --- xen/common/device-tree/bootfdt.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)