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

Dmytro Prokopchuk1 posted 1 patch 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/500cc80a172570f7fdde3287185398dc49460b2b.1783590032.git.dmytro._5Fprokopchuk1@epam.com
There is a newer version of this series
xen/common/device-tree/dom0less-build.c | 7 +++++++
1 file changed, 7 insertions(+)
[PATCH for-4.22 v3] dom0less: Prevent division by zero in handle_passthrough_prop()
Posted by Dmytro Prokopchuk1 2 weeks ago
A malformed 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 inside
handle_passthrough_prop():

    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 both 'address_cells' and 'size_cells'
are within the valid range of [1, 2] at the read side in scan_pfdt_node()
immediately after they are parsed. Any invalid cell size combination is
safely rejected early with an error message and return -EINVAL.

Fixes: 9ce974c47588 ("xen/arm: assign devices to boot domains")
Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
---
Changes in v3:
 - use Michal's idea for placing that check into other place
 - reword commit message
---
 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..179a2b88aa 100644
--- a/xen/common/device-tree/dom0less-build.c
+++ b/xen/common/device-tree/dom0less-build.c
@@ -341,6 +341,13 @@ static int __init scan_pfdt_node(struct kernel_info *kinfo, const void *pfdt,
     size_cells = device_tree_get_u32(pfdt, nodeoff, "#size-cells",
                                      DT_ROOT_NODE_SIZE_CELLS_DEFAULT);
 
+    if ( address_cells < 1 || address_cells > 2 ||
+         size_cells < 1 || size_cells > 2 )
+    {
+        dprintk(XENLOG_ERR "Invalid address/size cells combination\n");
+        return -EINVAL;
+    }
+
     node_next = fdt_first_subnode(pfdt, nodeoff);
     while ( node_next > 0 )
     {
-- 
2.43.0
Re: [PATCH for-4.22 v3] dom0less: Prevent division by zero in handle_passthrough_prop()
Posted by Andrew Cooper 2 weeks ago
On 09/07/2026 10:44 am, Dmytro Prokopchuk1 wrote:
> diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
> index eacfd93087..179a2b88aa 100644
> --- a/xen/common/device-tree/dom0less-build.c
> +++ b/xen/common/device-tree/dom0less-build.c
> @@ -341,6 +341,13 @@ static int __init scan_pfdt_node(struct kernel_info *kinfo, const void *pfdt,
>      size_cells = device_tree_get_u32(pfdt, nodeoff, "#size-cells",
>                                       DT_ROOT_NODE_SIZE_CELLS_DEFAULT);
>  
> +    if ( address_cells < 1 || address_cells > 2 ||
> +         size_cells < 1 || size_cells > 2 )
> +    {
> +        dprintk(XENLOG_ERR "Invalid address/size cells combination\n");
> +        return -EINVAL;

As an aside, this is a terrible error message.  It's literally
"something went wrong, but I'm not going to tell you what".

A better error message would be "Invalid address_cells %u or size_cells
%u\n".

You should always identify what value you found if you're going to say
you don't like it.  For this example, it might be that the issue isn't
in the DTB at all; it could be memory corruption causing Xen to find
junk here.

~Andrew

Re: [PATCH for-4.22 v3] dom0less: Prevent division by zero in handle_passthrough_prop()
Posted by Jan Beulich 2 weeks ago
On 09.07.2026 11:44, Dmytro Prokopchuk1 wrote:
> A malformed 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 inside
> handle_passthrough_prop():
> 
>     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 both 'address_cells' and 'size_cells'
> are within the valid range of [1, 2] at the read side in scan_pfdt_node()
> immediately after they are parsed. Any invalid cell size combination is
> safely rejected early with an error message and return -EINVAL.
> 
> Fixes: 9ce974c47588 ("xen/arm: assign devices to boot domains")
> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
> ---
> Changes in v3:
>  - use Michal's idea for placing that check into other place
>  - reword commit message
> ---
>  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..179a2b88aa 100644
> --- a/xen/common/device-tree/dom0less-build.c
> +++ b/xen/common/device-tree/dom0less-build.c
> @@ -341,6 +341,13 @@ static int __init scan_pfdt_node(struct kernel_info *kinfo, const void *pfdt,
>      size_cells = device_tree_get_u32(pfdt, nodeoff, "#size-cells",
>                                       DT_ROOT_NODE_SIZE_CELLS_DEFAULT);
>  
> +    if ( address_cells < 1 || address_cells > 2 ||
> +         size_cells < 1 || size_cells > 2 )
> +    {
> +        dprintk(XENLOG_ERR "Invalid address/size cells combination\n");

Perhaps just for my own education: Is it really "invalid", or merely
"unsupported" / "unrecognized"? (I can see 0 being possibly invalid, but
it looks less clear for values above 2.)

Jan
Re: [PATCH for-4.22 v3] dom0less: Prevent division by zero in handle_passthrough_prop()
Posted by Orzel, Michal 2 weeks ago

On 09-Jul-26 11:50, Jan Beulich wrote:
> On 09.07.2026 11:44, Dmytro Prokopchuk1 wrote:
>> A malformed 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 inside
>> handle_passthrough_prop():
>>
>>     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 both 'address_cells' and 'size_cells'
>> are within the valid range of [1, 2] at the read side in scan_pfdt_node()
>> immediately after they are parsed. Any invalid cell size combination is
>> safely rejected early with an error message and return -EINVAL.
>>
>> Fixes: 9ce974c47588 ("xen/arm: assign devices to boot domains")
>> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
>> ---
>> Changes in v3:
>>  - use Michal's idea for placing that check into other place
>>  - reword commit message
>> ---
>>  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..179a2b88aa 100644
>> --- a/xen/common/device-tree/dom0less-build.c
>> +++ b/xen/common/device-tree/dom0less-build.c
>> @@ -341,6 +341,13 @@ static int __init scan_pfdt_node(struct kernel_info *kinfo, const void *pfdt,
>>      size_cells = device_tree_get_u32(pfdt, nodeoff, "#size-cells",
>>                                       DT_ROOT_NODE_SIZE_CELLS_DEFAULT);
>>  
>> +    if ( address_cells < 1 || address_cells > 2 ||
>> +         size_cells < 1 || size_cells > 2 )
>> +    {
>> +        dprintk(XENLOG_ERR "Invalid address/size cells combination\n");
> 
> Perhaps just for my own education: Is it really "invalid", or merely
> "unsupported" / "unrecognized"? (I can see 0 being possibly invalid, but
> it looks less clear for values above 2.)
AFAIR the DT spec does not mention the limit other than the type limit.
The libfdt defines limit as 4 (see `FDT_MAX_NCELLS` macro).
I've only seen #address-cells being 3 for PCI buses, where the first cell
defines the space type). For me, personally invalid vs unsupported is blurry
because if something is unsupported from Xen PoV, it is also invalid from Xen
PoV, so I tend not to comment on such details (though I'm not a native speaker).

@Dmytro, to prevent back and forth discussions:
 - add brackets
 - improve the message as Andrew suggested

~Michal
Re: [PATCH for-4.22 v3] dom0less: Prevent division by zero in handle_passthrough_prop()
Posted by Dmytro Prokopchuk1 2 weeks ago

On 7/9/26 12:50, Jan Beulich wrote:
> On 09.07.2026 11:44, Dmytro Prokopchuk1 wrote:
>> A malformed 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 inside
>> handle_passthrough_prop():
>>
>>      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 both 'address_cells' and 'size_cells'
>> are within the valid range of [1, 2] at the read side in scan_pfdt_node()
>> immediately after they are parsed. Any invalid cell size combination is
>> safely rejected early with an error message and return -EINVAL.
>>
>> Fixes: 9ce974c47588 ("xen/arm: assign devices to boot domains")
>> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
>> ---
>> Changes in v3:
>>   - use Michal's idea for placing that check into other place
>>   - reword commit message
>> ---
>>   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..179a2b88aa 100644
>> --- a/xen/common/device-tree/dom0less-build.c
>> +++ b/xen/common/device-tree/dom0less-build.c
>> @@ -341,6 +341,13 @@ static int __init scan_pfdt_node(struct kernel_info *kinfo, const void *pfdt,
>>       size_cells = device_tree_get_u32(pfdt, nodeoff, "#size-cells",
>>                                        DT_ROOT_NODE_SIZE_CELLS_DEFAULT);
>>   
>> +    if ( address_cells < 1 || address_cells > 2 ||
>> +         size_cells < 1 || size_cells > 2 )
>> +    {
>> +        dprintk(XENLOG_ERR "Invalid address/size cells combination\n");
> 
> Perhaps just for my own education: Is it really "invalid", or merely
> "unsupported" / "unrecognized"? (I can see 0 being possibly invalid, but
> it looks less clear for values above 2.)
> 
> Jan

Yeah... In DT Spec the values 0 and 3 are valid, actually.
Here it's Xen's limitation in implementation. So, "unsupported" is the 
most accurate description.

Well, also I see that this function has many silent returns. And only 
one resulting error message at the end:

     printk("Device tree generation failed (%d).\n", ret);

Maybe just drop that message?
Re: [PATCH for-4.22 v3] dom0less: Prevent division by zero in handle_passthrough_prop()
Posted by Andrew Cooper 2 weeks ago
On 09/07/2026 10:50 am, Jan Beulich wrote:
> On 09.07.2026 11:44, Dmytro Prokopchuk1 wrote:
>> A malformed 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 inside
>> handle_passthrough_prop():
>>
>>     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 both 'address_cells' and 'size_cells'
>> are within the valid range of [1, 2] at the read side in scan_pfdt_node()
>> immediately after they are parsed. Any invalid cell size combination is
>> safely rejected early with an error message and return -EINVAL.
>>
>> Fixes: 9ce974c47588 ("xen/arm: assign devices to boot domains")
>> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
>> ---
>> Changes in v3:
>>  - use Michal's idea for placing that check into other place
>>  - reword commit message
>> ---
>>  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..179a2b88aa 100644
>> --- a/xen/common/device-tree/dom0less-build.c
>> +++ b/xen/common/device-tree/dom0less-build.c
>> @@ -341,6 +341,13 @@ static int __init scan_pfdt_node(struct kernel_info *kinfo, const void *pfdt,
>>      size_cells = device_tree_get_u32(pfdt, nodeoff, "#size-cells",
>>                                       DT_ROOT_NODE_SIZE_CELLS_DEFAULT);
>>  
>> +    if ( address_cells < 1 || address_cells > 2 ||
>> +         size_cells < 1 || size_cells > 2 )
>> +    {
>> +        dprintk(XENLOG_ERR "Invalid address/size cells combination\n");
> Perhaps just for my own education: Is it really "invalid", or merely
> "unsupported" / "unrecognized"? (I can see 0 being possibly invalid, but
> it looks less clear for values above 2.)

It's an encoding for variable length numbers.  3 would be 96-bit
numbers, 4 would be 128-bit numbers.

64-bit numbers is plenty, and there are a whole bunch of fun bugs to be
had if you believe that nr_cells=-1 describes a good number.

~Andrew