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

[Dmytro Prokopchuk1 <dmytro_prokopchuk1@xxxxxxxx>]

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.
> 
> Finally, as we're already touching on this code: sizeof(uint32_t) also is
> bogus, and it is a good example of why sizeof(<expression>) is to be
> preferred over sizeof(<type>): What's meant here is - afaict - sizeof(*cell),
> i.e. sizeof(__be32). (Imo using sizeof() with the wrong type is worse than
> writing simply 4.)
Ack.
> 
> Jan

BR, Dmytro.