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.hal...@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.or...@amd.com>
~Michal
