| Message ID | 20231206090623.1932275-3-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Follow-up static shared memory PART I | expand |
Hi Penny, On 06/12/2023 10:06, Penny Zheng wrote: > > > Putting overlap and overflow checking in the loop is causing repetitive > operation, so this commit extracts both checking outside the loop. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> In general the patch looks good to me: Reviewed-by: Michal Orzel <michal.orzel@amd.com> That said, there are 2 things I realized during review. > --- > v6: > new commit > --- > xen/arch/arm/static-shmem.c | 39 +++++++++++++++---------------------- > 1 file changed, 16 insertions(+), 23 deletions(-) > > diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c > index cb268cd2ed..1a1a9386e4 100644 > --- a/xen/arch/arm/static-shmem.c > +++ b/xen/arch/arm/static-shmem.c > @@ -349,7 +349,7 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, > { > const struct fdt_property *prop, *prop_id, *prop_role; > const __be32 *cell; > - paddr_t paddr, gaddr, size; > + paddr_t paddr, gaddr, size, end; > struct meminfo *mem = &bootinfo.reserved_mem; > unsigned int i; > int len; > @@ -422,6 +422,13 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, > return -EINVAL; > } > > + end = paddr + size; > + if ( end <= paddr ) > + { > + printk("fdt: static shared memory region %s overflow\n", shm_id); > + return -EINVAL; > + } > + > for ( i = 0; i < mem->nr_banks; i++ ) > { > /* > @@ -441,30 +448,13 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, > return -EINVAL; > } > } > + else if ( strcmp(shm_id, mem->bank[i].shm_id) != 0 ) > + continue; > else > { > - paddr_t end = paddr + size; > - paddr_t bank_end = mem->bank[i].start + mem->bank[i].size; > - > - if ( (end <= paddr) || (bank_end <= mem->bank[i].start) ) You are iterating over reserved memory regions in general, so apart from shmem regions there might be truly /reserved ones. It appears that we don't have overflow check in device_tree_get_meminfo, so this second check was the only place to detect that (protected by a feature, so not very useful) :) This is just an observation and I agree to drop it. We should be checking for an overflow in device_tree_get_meminfo. The second observation I made is that we don't seem to assign and check the return code from device_tree_for_each_node. This means, that any error while parsing the early fdt (e.g. static shm issues) does not stop Xen from booting, which might result in strange behavior later on. If others agree, I'm ok to send a fix for that. ~Michal
Hi Michal On 2023/12/6 19:35, Michal Orzel wrote: > Hi Penny, > > On 06/12/2023 10:06, Penny Zheng wrote: >> >> >> Putting overlap and overflow checking in the loop is causing repetitive >> operation, so this commit extracts both checking outside the loop. >> >> Signed-off-by: Penny Zheng <penny.zheng@arm.com> > In general the patch looks good to me: > Reviewed-by: Michal Orzel <michal.orzel@amd.com> > Thx~ > That said, there are 2 things I realized during review. > >> --- >> v6: >> new commit >> --- >> xen/arch/arm/static-shmem.c | 39 +++++++++++++++---------------------- >> 1 file changed, 16 insertions(+), 23 deletions(-) >> >> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c >> index cb268cd2ed..1a1a9386e4 100644 >> --- a/xen/arch/arm/static-shmem.c >> +++ b/xen/arch/arm/static-shmem.c >> @@ -349,7 +349,7 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, >> { >> const struct fdt_property *prop, *prop_id, *prop_role; >> const __be32 *cell; >> - paddr_t paddr, gaddr, size; >> + paddr_t paddr, gaddr, size, end; >> struct meminfo *mem = &bootinfo.reserved_mem; >> unsigned int i; >> int len; >> @@ -422,6 +422,13 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, >> return -EINVAL; >> } >> >> + end = paddr + size; >> + if ( end <= paddr ) >> + { >> + printk("fdt: static shared memory region %s overflow\n", shm_id); >> + return -EINVAL; >> + } >> + >> for ( i = 0; i < mem->nr_banks; i++ ) >> { >> /* >> @@ -441,30 +448,13 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, >> return -EINVAL; >> } >> } >> + else if ( strcmp(shm_id, mem->bank[i].shm_id) != 0 ) >> + continue; >> else >> { >> - paddr_t end = paddr + size; >> - paddr_t bank_end = mem->bank[i].start + mem->bank[i].size; >> - >> - if ( (end <= paddr) || (bank_end <= mem->bank[i].start) ) > You are iterating over reserved memory regions in general, so apart from shmem regions there might be truly /reserved ones. > It appears that we don't have overflow check in device_tree_get_meminfo, so this second check was the only place to detect that > (protected by a feature, so not very useful) :) This is just an observation and I agree to drop it. We should be checking for an > overflow in device_tree_get_meminfo. > True~ > The second observation I made is that we don't seem to assign and check the return code from device_tree_for_each_node. > This means, that any error while parsing the early fdt (e.g. static shm issues) does not stop Xen from booting, which might result in strange behavior later on. > If others agree, I'm ok to send a fix for that. > > ~Michal Penny Zheng Many thanks
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index cb268cd2ed..1a1a9386e4 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -349,7 +349,7 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, { const struct fdt_property *prop, *prop_id, *prop_role; const __be32 *cell; - paddr_t paddr, gaddr, size; + paddr_t paddr, gaddr, size, end; struct meminfo *mem = &bootinfo.reserved_mem; unsigned int i; int len; @@ -422,6 +422,13 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, return -EINVAL; } + end = paddr + size; + if ( end <= paddr ) + { + printk("fdt: static shared memory region %s overflow\n", shm_id); + return -EINVAL; + } + for ( i = 0; i < mem->nr_banks; i++ ) { /* @@ -441,30 +448,13 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, return -EINVAL; } } + else if ( strcmp(shm_id, mem->bank[i].shm_id) != 0 ) + continue; else { - paddr_t end = paddr + size; - paddr_t bank_end = mem->bank[i].start + mem->bank[i].size; - - if ( (end <= paddr) || (bank_end <= mem->bank[i].start) ) - { - printk("fdt: static shared memory region %s overflow\n", shm_id); - return -EINVAL; - } - - if ( check_reserved_regions_overlap(paddr, size) ) - return -EINVAL; - else - { - if ( strcmp(shm_id, mem->bank[i].shm_id) != 0 ) - continue; - else - { - printk("fdt: different shared memory region could not share the same shm ID %s\n", - shm_id); - return -EINVAL; - } - } + printk("fdt: different shared memory region could not share the same shm ID %s\n", + shm_id); + return -EINVAL; } } @@ -472,6 +462,9 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, { if ( i < NR_MEM_BANKS ) { + if ( check_reserved_regions_overlap(paddr, size) ) + return -EINVAL; + /* Static shared memory shall be reserved from any other use. */ safe_strcpy(mem->bank[mem->nr_banks].shm_id, shm_id); mem->bank[mem->nr_banks].start = paddr;
Putting overlap and overflow checking in the loop is causing repetitive operation, so this commit extracts both checking outside the loop. Signed-off-by: Penny Zheng <penny.zheng@arm.com> --- v6: new commit --- xen/arch/arm/static-shmem.c | 39 +++++++++++++++---------------------- 1 file changed, 16 insertions(+), 23 deletions(-)