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

[Julien Grall <julien@xxxxxxx>]

On 24/04/2023 08:44, Michal Orzel wrote:
> 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.
This function is used as a callback. So if you want to switch from u64 
to uint64_t then you should also update dt_for_each_range().
Such changes would need to be done in a separate patch.

Cheers,

--
Julien Grall