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

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

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