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

[Jan Beulich <jbeulich@xxxxxxxx>]

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