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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

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