| Message ID | 20230428175543.11902-11-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for 32-bit physical address | expand |
Hi, On 28/04/2023 18:55, Ayan Kumar Halder wrote: > handle_pci_range() and map_range_to_domain() take addr and len as uint64_t > parameters. Then frame numbers are obtained from addr and len by right shifting > with PAGE_SHIFT. The frame numbers are expressed using unsigned long. > > Now if 64-bit >> PAGE_SHIFT, the result will have 52-bits as valid. On a 32-bit > system, 'unsigned long' is 32-bits. Thus, there is a potential loss of value > when the result is stored as 'unsigned long'. > > To mitigate this issue, we check if the starting and end address can be > contained within the range of physical address supported on the system. If not, > then an appropriate error is returned. > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> Reviewed-by: Julien Grall <jgrall@amazon.com> Cheers,
On 28/04/2023 19:55, Ayan Kumar Halder wrote: > > > handle_pci_range() and map_range_to_domain() take addr and len as uint64_t > parameters. Then frame numbers are obtained from addr and len by right shifting > with PAGE_SHIFT. The frame numbers are expressed using unsigned long. > > Now if 64-bit >> PAGE_SHIFT, the result will have 52-bits as valid. On a 32-bit > system, 'unsigned long' is 32-bits. Thus, there is a potential loss of value > when the result is stored as 'unsigned long'. > > To mitigate this issue, we check if the starting and end address can be > contained within the range of physical address supported on the system. If not, > then an appropriate error is returned. > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> > --- > Changes from :- > v1...v4 - NA. New patch introduced in v5. > > v5 - 1. Updated the error message > 2. Used "(((paddr_t)~0 - addr) < len)" to check the limit on len. > 3. Changes in the prototype of "map_range_to_domain()" has been > addressed by the patch 8. > > xen/arch/arm/domain_build.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 9865340eac..719bb09845 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -1643,6 +1643,13 @@ static int __init handle_pci_range(const struct dt_device_node *dev, > paddr_t start, end; > int res; > > + if ( addr != (paddr_t)addr || (((paddr_t)~0 - addr) < len) ) Given that you enclose the second condition in parenthesis, I would expect the same for the first. > + { > + printk(XENLOG_ERR "%s: [0x%"PRIx64", 0x%"PRIx64"] exceeds the maximum allowed PA width (%u bits)", > + dt_node_full_name(dev), addr, (addr + len), PADDR_BITS); > + return -ERANGE; > + } > + > start = addr & PAGE_MASK; > end = PAGE_ALIGN(addr + len); > res = rangeset_remove_range(mem_holes, PFN_DOWN(start), PFN_DOWN(end - 1)); > @@ -2337,6 +2344,13 @@ int __init map_range_to_domain(const struct dt_device_node *dev, > struct domain *d = mr_data->d; > int res; > > + if ( addr != (paddr_t)addr || (((paddr_t)~0 - addr) < len) ) Same here. Other than that: Reviewed-by: Michal Orzel <michal.orzel@amd.com> ~Michal
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 9865340eac..719bb09845 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1643,6 +1643,13 @@ static int __init handle_pci_range(const struct dt_device_node *dev, paddr_t start, end; int res; + if ( addr != (paddr_t)addr || (((paddr_t)~0 - addr) < len) ) + { + printk(XENLOG_ERR "%s: [0x%"PRIx64", 0x%"PRIx64"] exceeds the maximum allowed PA width (%u bits)", + dt_node_full_name(dev), addr, (addr + len), PADDR_BITS); + return -ERANGE; + } + start = addr & PAGE_MASK; end = PAGE_ALIGN(addr + len); res = rangeset_remove_range(mem_holes, PFN_DOWN(start), PFN_DOWN(end - 1)); @@ -2337,6 +2344,13 @@ int __init map_range_to_domain(const struct dt_device_node *dev, struct domain *d = mr_data->d; int res; + if ( addr != (paddr_t)addr || (((paddr_t)~0 - addr) < len) ) + { + printk(XENLOG_ERR "%s: [0x%"PRIx64", 0x%"PRIx64"] exceeds the maximum allowed PA width (%u bits)", + dt_node_full_name(dev), addr, (addr + len), PADDR_BITS); + return -ERANGE; + } + /* * reserved-memory regions are RAM carved out for a special purpose. * They are not MMIO and therefore a domain should not be able to
handle_pci_range() and map_range_to_domain() take addr and len as uint64_t parameters. Then frame numbers are obtained from addr and len by right shifting with PAGE_SHIFT. The frame numbers are expressed using unsigned long. Now if 64-bit >> PAGE_SHIFT, the result will have 52-bits as valid. On a 32-bit system, 'unsigned long' is 32-bits. Thus, there is a potential loss of value when the result is stored as 'unsigned long'. To mitigate this issue, we check if the starting and end address can be contained within the range of physical address supported on the system. If not, then an appropriate error is returned. Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> --- Changes from :- v1...v4 - NA. New patch introduced in v5. v5 - 1. Updated the error message 2. Used "(((paddr_t)~0 - addr) < len)" to check the limit on len. 3. Changes in the prototype of "map_range_to_domain()" has been addressed by the patch 8. xen/arch/arm/domain_build.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)