[PATCH for-4.22] dom0less: Prevent division by zero in handle_passthrough_prop()

[PATCH for-4.22] dom0less: Prevent division by zero in handle_passthrough_prop()

[Dmytro Prokopchuk1]

A malformed provided partial DTB specifying both '#address-cells = <0>'
and '#size-cells = <0>' causes '(address_cells * 2 + size_cells)' to
evaluate to 0. This sum is subsequently used as a divisor when calculating
the number of regions in the 'xen,reg' property:

    len = fdt32_to_cpu(xen_reg->len) / ((address_cells * 2 + size_cells) *
                                        sizeof(uint32_t));

This leads to a division by zero exception in the Xen hypervisor during
boot, causing a hypervisor panic/crash.

Fix this by validating that '(address_cells * 2 + size_cells)' is greater
than zero before performing the division. If it is zero, log an error
message and return -EINVAL.

Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
---
 xen/common/device-tree/dom0less-build.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
index eacfd93087..6796851844 100644
--- a/xen/common/device-tree/dom0less-build.c
+++ b/xen/common/device-tree/dom0less-build.c
@@ -154,6 +154,13 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
 
     /* xen,reg specifies where to map the MMIO region */
     cell = (const __be32 *)xen_reg->data;
+
+    if ( (address_cells * 2 + size_cells) == 0 )
+    {
+        printk(XENLOG_ERR "Invalid address/size cells combination (both 0)\n");
+        return -EINVAL;
+    }
+
     len = fdt32_to_cpu(xen_reg->len) / ((address_cells * 2 + size_cells) *
                                         sizeof(uint32_t));
 
-- 
2.43.0

Re: [PATCH for-4.22] dom0less: Prevent division by zero in handle_passthrough_prop()

[Oleksii Kurochko]

On 7/7/26 5:16 PM, Dmytro Prokopchuk1 wrote:
> A malformed provided partial DTB specifying both '#address-cells = <0>'
> and '#size-cells = <0>' causes '(address_cells * 2 + size_cells)' to
> evaluate to 0. This sum is subsequently used as a divisor when calculating
> the number of regions in the 'xen,reg' property:
> 
>      len = fdt32_to_cpu(xen_reg->len) / ((address_cells * 2 + size_cells) *
>                                          sizeof(uint32_t));
> 
> This leads to a division by zero exception in the Xen hypervisor during
> boot, causing a hypervisor panic/crash.
> 
> Fix this by validating that '(address_cells * 2 + size_cells)' is greater
> than zero before performing the division. If it is zero, log an error
> message and return -EINVAL.
> 

Fix tag is missed.
> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
> ---
>   xen/common/device-tree/dom0less-build.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
> index eacfd93087..6796851844 100644
> --- a/xen/common/device-tree/dom0less-build.c
> +++ b/xen/common/device-tree/dom0less-build.c
> @@ -154,6 +154,13 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
>   
>       /* xen,reg specifies where to map the MMIO region */
>       cell = (const __be32 *)xen_reg->data;
> +
> +    if ( (address_cells * 2 + size_cells) == 0 )

Considering that this calculation happens second time here ...

> +    {
> +        printk(XENLOG_ERR "Invalid address/size cells combination (both 0)\n");
> +        return -EINVAL;
> +    }
> +
>       len = fdt32_to_cpu(xen_reg->len) / ((address_cells * 2 + size_cells) *
>                                           sizeof(uint32_t));

... I think it would be nice to calculate that once.

Generally I am okay to not declare local separate variable for these 
calculations.

With adding Fix tag:
  Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
  Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii

Re: [PATCH for-4.22] dom0less: Prevent division by zero in handle_passthrough_prop()

[Jan Beulich]

On 07.07.2026 18:08, Oleksii Kurochko wrote:
> On 7/7/26 5:16 PM, Dmytro Prokopchuk1 wrote:
>> --- a/xen/common/device-tree/dom0less-build.c
>> +++ b/xen/common/device-tree/dom0less-build.c
>> @@ -154,6 +154,13 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
>>   
>>       /* xen,reg specifies where to map the MMIO region */
>>       cell = (const __be32 *)xen_reg->data;
>> +
>> +    if ( (address_cells * 2 + size_cells) == 0 )
> 
> Considering that this calculation happens second time here ...
> 
>> +    {
>> +        printk(XENLOG_ERR "Invalid address/size cells combination (both 0)\n");
>> +        return -EINVAL;
>> +    }
>> +
>>       len = fdt32_to_cpu(xen_reg->len) / ((address_cells * 2 + size_cells) *
>>                                           sizeof(uint32_t));
> 
> ... I think it would be nice to calculate that once.

Hmm, originally I meant to simply stay silent here. But now that you say this,
I'd like to express that I find this 2nd calculation of the same expression
bogus. If the goal is to deal with both values being zero at the same time,
check that (and nothing else). If instead the goal is to truly prevent the
divisor expression from ending up 0, that (and not a shorter surrogate) would
need checking. In particular, the multiplication by sizeof(uint32_t) can
convert non-zero to zero.

At that point the question then would be whether overflow (and hence
truncation) in any of the involved expressions shouldn't also be detected /
rejected.

Finally, as we're already touching on this code: sizeof(uint32_t) also is
bogus, and it is a good example of why sizeof(<expression>) is to be
preferred over sizeof(<type>): What's meant here is - afaict - sizeof(*cell),
i.e. sizeof(__be32). (Imo using sizeof() with the wrong type is worse than
writing simply 4.)

Jan

Re: [PATCH for-4.22] dom0less: Prevent division by zero in handle_passthrough_prop()

[Dmytro Prokopchuk1]

Hello Jan,

On 7/8/26 09:15, Jan Beulich wrote:
> On 07.07.2026 18:08, Oleksii Kurochko wrote:
>> On 7/7/26 5:16 PM, Dmytro Prokopchuk1 wrote:
>>> --- a/xen/common/device-tree/dom0less-build.c
>>> +++ b/xen/common/device-tree/dom0less-build.c
>>> @@ -154,6 +154,13 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
>>>    
>>>        /* xen,reg specifies where to map the MMIO region */
>>>        cell = (const __be32 *)xen_reg->data;
>>> +
>>> +    if ( (address_cells * 2 + size_cells) == 0 )
>>
>> Considering that this calculation happens second time here ...
>>
>>> +    {
>>> +        printk(XENLOG_ERR "Invalid address/size cells combination (both 0)\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>>        len = fdt32_to_cpu(xen_reg->len) / ((address_cells * 2 + size_cells) *
>>>                                            sizeof(uint32_t));
>>
>> ... I think it would be nice to calculate that once.
> 
> Hmm, originally I meant to simply stay silent here. But now that you say this,
> I'd like to express that I find this 2nd calculation of the same expression
> bogus. If the goal is to deal with both values being zero at the same time,
> check that (and nothing else). If instead the goal is to truly prevent the
> divisor expression from ending up 0, that (and not a shorter surrogate) would
> need checking. In particular, the multiplication by sizeof(uint32_t) can
> convert non-zero to zero.
Yes, you are right. Need to check whole expression.
> 
> At that point the question then would be whether overflow (and hence
> truncation) in any of the involved expressions shouldn't also be detected /
> rejected.
Testing zero is useful, but not enough - the expression (address_cells * 
2 + size_cells) * sizeof(*cell) can overflow and wrap around to a small, 
non-zero number. Source code analyze showed that Xen only supports cell 
sizes of 1 or 2, and there is a ASSERT_UNREACHABLE() in dt_read_number() 
which prevents from using wrong cell values in DEBUG builds.

I would propose the next checking:

     if ( address_cells < 1 || address_cells > 2 ||
          size_cells < 1 || size_cells > 2 )
     {
         printk(XENLOG_ERR "Invalid address/size cells combination\n");
         return -EINVAL;
     }

This will cover zero check, and overflows.
> 
> Finally, as we're already touching on this code: sizeof(uint32_t) also is
> bogus, and it is a good example of why sizeof(<expression>) is to be
> preferred over sizeof(<type>): What's meant here is - afaict - sizeof(*cell),
> i.e. sizeof(__be32). (Imo using sizeof() with the wrong type is worse than
> writing simply 4.)
Ack.
> 
> Jan

BR, Dmytro.

Re: [PATCH for-4.22] dom0less: Prevent division by zero in handle_passthrough_prop()

[Jan Beulich]

On 08.07.2026 19:04, Dmytro Prokopchuk1 wrote:
> Hello Jan,
> 
> On 7/8/26 09:15, Jan Beulich wrote:
>> On 07.07.2026 18:08, Oleksii Kurochko wrote:
>>> On 7/7/26 5:16 PM, Dmytro Prokopchuk1 wrote:
>>>> --- a/xen/common/device-tree/dom0less-build.c
>>>> +++ b/xen/common/device-tree/dom0less-build.c
>>>> @@ -154,6 +154,13 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
>>>>    
>>>>        /* xen,reg specifies where to map the MMIO region */
>>>>        cell = (const __be32 *)xen_reg->data;
>>>> +
>>>> +    if ( (address_cells * 2 + size_cells) == 0 )
>>>
>>> Considering that this calculation happens second time here ...
>>>
>>>> +    {
>>>> +        printk(XENLOG_ERR "Invalid address/size cells combination (both 0)\n");
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>>        len = fdt32_to_cpu(xen_reg->len) / ((address_cells * 2 + size_cells) *
>>>>                                            sizeof(uint32_t));
>>>
>>> ... I think it would be nice to calculate that once.
>>
>> Hmm, originally I meant to simply stay silent here. But now that you say this,
>> I'd like to express that I find this 2nd calculation of the same expression
>> bogus. If the goal is to deal with both values being zero at the same time,
>> check that (and nothing else). If instead the goal is to truly prevent the
>> divisor expression from ending up 0, that (and not a shorter surrogate) would
>> need checking. In particular, the multiplication by sizeof(uint32_t) can
>> convert non-zero to zero.
> Yes, you are right. Need to check whole expression.
>>
>> At that point the question then would be whether overflow (and hence
>> truncation) in any of the involved expressions shouldn't also be detected /
>> rejected.
> Testing zero is useful, but not enough - the expression (address_cells * 
> 2 + size_cells) * sizeof(*cell) can overflow and wrap around to a small, 
> non-zero number. Source code analyze showed that Xen only supports cell 
> sizes of 1 or 2, and there is a ASSERT_UNREACHABLE() in dt_read_number() 
> which prevents from using wrong cell values in DEBUG builds.
> 
> I would propose the next checking:
> 
>      if ( address_cells < 1 || address_cells > 2 ||
>           size_cells < 1 || size_cells > 2 )
>      {
>          printk(XENLOG_ERR "Invalid address/size cells combination\n");
>          return -EINVAL;
>      }
> 
> This will cover zero check, and overflows.

It'll need to be the maintainers of this code to judge whether this is
appropriate here.

Jan

Re: [PATCH for-4.22] dom0less: Prevent division by zero in handle_passthrough_prop()

[Orzel, Michal]

On 09-Jul-26 08:57, Jan Beulich wrote:
> On 08.07.2026 19:04, Dmytro Prokopchuk1 wrote:
>> Hello Jan,
>>
>> On 7/8/26 09:15, Jan Beulich wrote:
>>> On 07.07.2026 18:08, Oleksii Kurochko wrote:
>>>> On 7/7/26 5:16 PM, Dmytro Prokopchuk1 wrote:
>>>>> --- a/xen/common/device-tree/dom0less-build.c
>>>>> +++ b/xen/common/device-tree/dom0less-build.c
>>>>> @@ -154,6 +154,13 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
>>>>>    
>>>>>        /* xen,reg specifies where to map the MMIO region */
>>>>>        cell = (const __be32 *)xen_reg->data;
>>>>> +
>>>>> +    if ( (address_cells * 2 + size_cells) == 0 )
>>>>
>>>> Considering that this calculation happens second time here ...
>>>>
>>>>> +    {
>>>>> +        printk(XENLOG_ERR "Invalid address/size cells combination (both 0)\n");
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +
>>>>>        len = fdt32_to_cpu(xen_reg->len) / ((address_cells * 2 + size_cells) *
>>>>>                                            sizeof(uint32_t));
>>>>
>>>> ... I think it would be nice to calculate that once.
>>>
>>> Hmm, originally I meant to simply stay silent here. But now that you say this,
>>> I'd like to express that I find this 2nd calculation of the same expression
>>> bogus. If the goal is to deal with both values being zero at the same time,
>>> check that (and nothing else). If instead the goal is to truly prevent the
>>> divisor expression from ending up 0, that (and not a shorter surrogate) would
>>> need checking. In particular, the multiplication by sizeof(uint32_t) can
>>> convert non-zero to zero.
>> Yes, you are right. Need to check whole expression.
>>>
>>> At that point the question then would be whether overflow (and hence
>>> truncation) in any of the involved expressions shouldn't also be detected /
>>> rejected.
>> Testing zero is useful, but not enough - the expression (address_cells * 
>> 2 + size_cells) * sizeof(*cell) can overflow and wrap around to a small, 
>> non-zero number. Source code analyze showed that Xen only supports cell 
>> sizes of 1 or 2, and there is a ASSERT_UNREACHABLE() in dt_read_number() 
>> which prevents from using wrong cell values in DEBUG builds.
>>
>> I would propose the next checking:
>>
>>      if ( address_cells < 1 || address_cells > 2 ||
>>           size_cells < 1 || size_cells > 2 )
>>      {
>>          printk(XENLOG_ERR "Invalid address/size cells combination\n");
>>          return -EINVAL;
>>      }
>>
>> This will cover zero check, and overflows.
> 
> It'll need to be the maintainers of this code to judge whether this is
> appropriate here.
It is but I would prefer to put it at the read side, not at the use side. Place
it in scan_pfdt_node() after `size_cells = device_tree_get_u32`.
The first call to scan_pfdt_node() passes defaults, so it is ok.

~Michal

Re: [PATCH for-4.22] dom0less: Prevent division by zero in handle_passthrough_prop()

[Dmytro Prokopchuk1]

On 7/9/26 10:51, Orzel, Michal wrote:
> 
> 
> On 09-Jul-26 08:57, Jan Beulich wrote:
>> On 08.07.2026 19:04, Dmytro Prokopchuk1 wrote:
>>> Hello Jan,
>>>
>>> On 7/8/26 09:15, Jan Beulich wrote:
>>>> On 07.07.2026 18:08, Oleksii Kurochko wrote:
>>>>> On 7/7/26 5:16 PM, Dmytro Prokopchuk1 wrote:
>>>>>> --- a/xen/common/device-tree/dom0less-build.c
>>>>>> +++ b/xen/common/device-tree/dom0less-build.c
>>>>>> @@ -154,6 +154,13 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
>>>>>>     
>>>>>>         /* xen,reg specifies where to map the MMIO region */
>>>>>>         cell = (const __be32 *)xen_reg->data;
>>>>>> +
>>>>>> +    if ( (address_cells * 2 + size_cells) == 0 )
>>>>>
>>>>> Considering that this calculation happens second time here ...
>>>>>
>>>>>> +    {
>>>>>> +        printk(XENLOG_ERR "Invalid address/size cells combination (both 0)\n");
>>>>>> +        return -EINVAL;
>>>>>> +    }
>>>>>> +
>>>>>>         len = fdt32_to_cpu(xen_reg->len) / ((address_cells * 2 + size_cells) *
>>>>>>                                             sizeof(uint32_t));
>>>>>
>>>>> ... I think it would be nice to calculate that once.
>>>>
>>>> Hmm, originally I meant to simply stay silent here. But now that you say this,
>>>> I'd like to express that I find this 2nd calculation of the same expression
>>>> bogus. If the goal is to deal with both values being zero at the same time,
>>>> check that (and nothing else). If instead the goal is to truly prevent the
>>>> divisor expression from ending up 0, that (and not a shorter surrogate) would
>>>> need checking. In particular, the multiplication by sizeof(uint32_t) can
>>>> convert non-zero to zero.
>>> Yes, you are right. Need to check whole expression.
>>>>
>>>> At that point the question then would be whether overflow (and hence
>>>> truncation) in any of the involved expressions shouldn't also be detected /
>>>> rejected.
>>> Testing zero is useful, but not enough - the expression (address_cells *
>>> 2 + size_cells) * sizeof(*cell) can overflow and wrap around to a small,
>>> non-zero number. Source code analyze showed that Xen only supports cell
>>> sizes of 1 or 2, and there is a ASSERT_UNREACHABLE() in dt_read_number()
>>> which prevents from using wrong cell values in DEBUG builds.
>>>
>>> I would propose the next checking:
>>>
>>>       if ( address_cells < 1 || address_cells > 2 ||
>>>            size_cells < 1 || size_cells > 2 )
>>>       {
>>>           printk(XENLOG_ERR "Invalid address/size cells combination\n");
>>>           return -EINVAL;
>>>       }
>>>
>>> This will cover zero check, and overflows.
>>
>> It'll need to be the maintainers of this code to judge whether this is
>> appropriate here.
> It is but I would prefer to put it at the read side, not at the use side. Place
> it in scan_pfdt_node() after `size_cells = device_tree_get_u32`.
> The first call to scan_pfdt_node() passes defaults, so it is ok.
> 
> ~Michal
> 

Hi Michal,

Nice idea! I'll create v3 for this.

BR, Dmytro.

Re: [PATCH for-4.22] dom0less: Prevent division by zero in handle_passthrough_prop()

[Orzel, Michal]

On 09-Jul-26 11:39, Dmytro Prokopchuk1 wrote:
> 
> 
> On 7/9/26 10:51, Orzel, Michal wrote:
>>
>>
>> On 09-Jul-26 08:57, Jan Beulich wrote:
>>> On 08.07.2026 19:04, Dmytro Prokopchuk1 wrote:
>>>> Hello Jan,
>>>>
>>>> On 7/8/26 09:15, Jan Beulich wrote:
>>>>> On 07.07.2026 18:08, Oleksii Kurochko wrote:
>>>>>> On 7/7/26 5:16 PM, Dmytro Prokopchuk1 wrote:
>>>>>>> --- a/xen/common/device-tree/dom0less-build.c
>>>>>>> +++ b/xen/common/device-tree/dom0less-build.c
>>>>>>> @@ -154,6 +154,13 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
>>>>>>>     
>>>>>>>         /* xen,reg specifies where to map the MMIO region */
>>>>>>>         cell = (const __be32 *)xen_reg->data;
>>>>>>> +
>>>>>>> +    if ( (address_cells * 2 + size_cells) == 0 )
>>>>>>
>>>>>> Considering that this calculation happens second time here ...
>>>>>>
>>>>>>> +    {
>>>>>>> +        printk(XENLOG_ERR "Invalid address/size cells combination (both 0)\n");
>>>>>>> +        return -EINVAL;
>>>>>>> +    }
>>>>>>> +
>>>>>>>         len = fdt32_to_cpu(xen_reg->len) / ((address_cells * 2 + size_cells) *
>>>>>>>                                             sizeof(uint32_t));
>>>>>>
>>>>>> ... I think it would be nice to calculate that once.
>>>>>
>>>>> Hmm, originally I meant to simply stay silent here. But now that you say this,
>>>>> I'd like to express that I find this 2nd calculation of the same expression
>>>>> bogus. If the goal is to deal with both values being zero at the same time,
>>>>> check that (and nothing else). If instead the goal is to truly prevent the
>>>>> divisor expression from ending up 0, that (and not a shorter surrogate) would
>>>>> need checking. In particular, the multiplication by sizeof(uint32_t) can
>>>>> convert non-zero to zero.
>>>> Yes, you are right. Need to check whole expression.
>>>>>
>>>>> At that point the question then would be whether overflow (and hence
>>>>> truncation) in any of the involved expressions shouldn't also be detected /
>>>>> rejected.
>>>> Testing zero is useful, but not enough - the expression (address_cells *
>>>> 2 + size_cells) * sizeof(*cell) can overflow and wrap around to a small,
>>>> non-zero number. Source code analyze showed that Xen only supports cell
>>>> sizes of 1 or 2, and there is a ASSERT_UNREACHABLE() in dt_read_number()
>>>> which prevents from using wrong cell values in DEBUG builds.
>>>>
>>>> I would propose the next checking:
>>>>
>>>>       if ( address_cells < 1 || address_cells > 2 ||
>>>>            size_cells < 1 || size_cells > 2 )
>>>>       {
>>>>           printk(XENLOG_ERR "Invalid address/size cells combination\n");
>>>>           return -EINVAL;
>>>>       }
>>>>
>>>> This will cover zero check, and overflows.
>>>
>>> It'll need to be the maintainers of this code to judge whether this is
>>> appropriate here.
>> It is but I would prefer to put it at the read side, not at the use side. Place
>> it in scan_pfdt_node() after `size_cells = device_tree_get_u32`.
>> The first call to scan_pfdt_node() passes defaults, so it is ok.
>>
>> ~Michal
>>
> 
> Hi Michal,
> 
> Nice idea! I'll create v3 for this.
Also, to avoid respins, please put the expressions in brackets.

~Michal