Re: [XEN v6 10/12] xen/arm: domain_build: Check if the address fits the range of physical address

[Michal Orzel <michal.orzel@xxxxxxx>]

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@xxxxxxx>
> ---
> 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@xxxxxxx>

~Michal