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

[Dmytro Prokopchuk1 <dmytro_prokopchuk1@xxxxxxxx>]

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.