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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 24/04/2023 09:26, Julien Grall wrote:
> Hi,
Hi Julien,
> 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))
It should be

if ( ... || (((paddr_t)~0 - addr) < len) )

{

/* print error */

}

> > +    {
> > +        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.
I think this should be fine ?

printk(XENLOG_ERR "[%#"PRIx64", "#PRIx64"] exceeds the maximum allowed PA width (%u 
bits)", start, end, CONFIG_PADDR_BITS);

- Ayan

> > +               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,