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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 13/04/2023 18: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.
>
> 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;
I find the difference between end and end_addr a bit confusing. How about...

> +
> +    if ( addr != (paddr_t)addr || end_addr != (paddr_t)end_addr )
... replace the second part with (((paddr_t)~0 - addr) > len))

> +    {
> +        printk(XENLOG_ERR "addr (0x%"PRIx64") or end_addr (0x%"PRIx64") exceeds the 
> maximum allowed width (%d bits) for physical address\n",
In addition to what Michal says, I would replace the "addr .... 
end_addr" with "[start, end]" to further reduce the message.
Also, please use %u rather than %d as the number of bits cannot be negative.

> +               addr, end_addr, CONFIG_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));
> +    end = PAGE_ALIGN(end_addr);
> +    res = rangeset_remove_range(mem_holes, PFN_DOWN(start), PFN_DOWN(end));
And this will not need to be changed.

>       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)
>   {
My comment on the previous function applies for this one as well.

Cheers,

--
Julien Grall