[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Michal Orzel <michal.orzel@xxxxxxx>
  • Date: Mon, 24 Apr 2023 09:44:04 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=lists.xenproject.org smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=grNcrikzXvK4eARuWWUjkz9IC2bJ7kOp0Z+dkBHWOH8=; b=Bnz8JYaOcRfyOd8fg6saAQ20K9/auu1p4zuYmdqu6hkGCMtB612iwjFqrlEJslLWSCdVScnKYtp2XuzDyHGpeurV9CADH8F6t2ZoQCnEQvIMQdjSltJ4X9NuVLl7rQxZgdZ1MQ/CL1PWvsrQqAVX74CyhQRsGQyUObyWdmEUxcLE+Flv/upRM66tVFkJecgfGW6OPAV5ux4YBBHflC/P7XKgJT4K8Oz8ayLAjmAT0b+qbs6wo3ZRt0x4vcK8Z0n9hgA/P17CymzPCxcjiu0SBUGPGsZFj++O6Og7whmy+a/CS+/K7c0wuI7ljOKtiBY2vjSt4oVwNvsCEPCZ0GomeQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=eHPFNujeeqlLsaVHxsahf2KkpvQxHjnZIOOIO+/ALo5r4Bi7ISo3OjbqLBTFvQJT7W/5pUREqxv7RO1Au6RoMSwEsEpboDtxEvC1+XnZ3BPCHCygmTG36x3ggEMyWd+e4tikPQgB0VeTpbNy1Sz9ed+5yRZbWwn/daGnINNpSCDM1dvCf0LQqT/t725MiIq5UxS/uz+5bojQvEl9esiyP0ukhYvVZN+zbJSIJ1iE7v7OPF/AGsQ4LK8SzzEC8FD+4dmkZhHFjmt0V2oY9MsGU4menSFkuIHtVW1YcL2EI9Ua+fRNkZbII1JckAEPqi/UnY3xHcgLm05fPfrQHhgS5A==
  • Cc: <sstabellini@xxxxxxxxxx>, <stefano.stabellini@xxxxxxx>, <julien@xxxxxxx>, <Volodymyr_Babchuk@xxxxxxxx>, <bertrand.marquis@xxxxxxx>, <andrew.cooper3@xxxxxxxxxx>, <george.dunlap@xxxxxxxxxx>, <jbeulich@xxxxxxxx>, <wl@xxxxxxx>, <rahul.singh@xxxxxxx>
  • Delivery-date: Mon, 24 Apr 2023 07:44:31 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.