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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Ayan,

On 13/04/2023 19:37, 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 page frame numbers are saved using unsigned long.
Maybe better to say "expressed" rather than "saved"

> 
> 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.
> 
> Also, the end address is computed once and used when required. And replaced 
> u64
> with uint64_t.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> ---
> 
> Changes from :-
> v1...v4 - NA. New patch introduced in v5.
> 
>  xen/arch/arm/domain_build.c | 30 +++++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 7d28b75517..b98ee506a8 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1637,15 +1637,23 @@ out:
>  }
> 
>  static int __init handle_pci_range(const struct dt_device_node *dev,
> -                                   u64 addr, u64 len, void *data)
> +                                   uint64_t addr, uint64_t len, void *data)
>  {
>      struct rangeset *mem_holes = data;
>      paddr_t start, end;
>      int res;
> +    uint64_t end_addr = addr + len - 1;
> +
> +    if ( addr != (paddr_t)addr || end_addr != (paddr_t)end_addr )
> +    {
> +        printk(XENLOG_ERR "addr (0x%"PRIx64") or end_addr (0x%"PRIx64") 
> exceeds the maximum allowed width (%d bits) for physical address\n",
I don't think it is wise to print variable names (end_addr) to the user. Better 
would be to say explicitly: start, end address.
Also to make the message shorter you could write: ... exceeds the maximum 
allowed PA width (%u bits)

> +               addr, end_addr, CONFIG_PADDR_BITS);
Why CONFIG_PADDR_BITS if you already introduced PADDR_BITS macro

> +        return -ERANGE;
> +    }
> 
>      start = addr & PAGE_MASK;
> -    end = PAGE_ALIGN(addr + len);
> -    res = rangeset_remove_range(mem_holes, PFN_DOWN(start), PFN_DOWN(end - 
> 1));
> +    end = PAGE_ALIGN(end_addr);
> +    res = rangeset_remove_range(mem_holes, PFN_DOWN(start), PFN_DOWN(end));
I doubt PFN_DOWN(end) is the same as PFN_DOWN(end - 1), so I think you should 
keep the behavior as it was

>      if ( res )
>      {
>          printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
> @@ -2330,11 +2338,19 @@ static int __init map_dt_irq_to_domain(const struct 
> dt_device_node *dev,
>  }
> 
>  int __init map_range_to_domain(const struct dt_device_node *dev,
> -                               u64 addr, u64 len, void *data)
> +                               uint64_t addr, uint64_t len, void *data)
You changed u64 to uint64_t in a definition but not in a prototype. Please fix.

>  {
>      struct map_range_data *mr_data = data;
>      struct domain *d = mr_data->d;
>      int res;
> +    uint64_t end_addr = addr + len - 1;
> +
> +    if ( addr != (paddr_t)addr || end_addr != (paddr_t)end_addr )
> +    {
> +        printk(XENLOG_ERR "addr (0x%"PRIx64") or end_addr (0x%"PRIx64") 
> exceeds the maximum allowed width (%d bits) for physical address\n",
> +               addr, end_addr, CONFIG_PADDR_BITS);
please see the remarks above about this code

> +        return -ERANGE;
> +    }
> 
>      /*
>       * reserved-memory regions are RAM carved out for a special purpose.
> @@ -2345,13 +2361,13 @@ int __init map_range_to_domain(const struct 
> dt_device_node *dev,
>                       strlen("/reserved-memory/")) != 0 )
>      {
>          res = iomem_permit_access(d, paddr_to_pfn(addr),
> -                paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
> +                paddr_to_pfn(PAGE_ALIGN(end_addr)));
>          if ( res )
>          {
>              printk(XENLOG_ERR "Unable to permit to dom%d access to"
>                      " 0x%"PRIx64" - 0x%"PRIx64"\n",
>                      d->domain_id,
> -                    addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
> +                    addr & PAGE_MASK, PAGE_ALIGN(end_addr) - 1);
>              return res;
>          }
>      }
> @@ -2368,7 +2384,7 @@ int __init map_range_to_domain(const struct 
> dt_device_node *dev,
>          {
>              printk(XENLOG_ERR "Unable to map 0x%"PRIx64
>                     " - 0x%"PRIx64" in domain %d\n",
> -                   addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
> +                   addr & PAGE_MASK, PAGE_ALIGN(end_addr) - 1,
>                     d->domain_id);
>              return res;
>          }
> --
> 2.17.1
> 
> 

~Michal