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@amd.com>
---

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",
+               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));
     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)
 {
     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);
+        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

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@amd.com>
> ---
> 
> 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

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@amd.com>
>> ---
>>
>> 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,
>

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@amd.com>
> ---
> 
> 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

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@amd.com>
>> ---
>>
>> 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