Re: [PATCH v2 1/2] x86/pci: prevent cross-device accesses in pci_mmcfg_{read,write}()

On 07.08.2026 10:01, Roger Pau Monné wrote:
> On Fri, Aug 07, 2026 at 08:22:44AM +0200, Jan Beulich wrote:
>> On 06.08.2026 17:26, Roger Pau Monne wrote:
>>> Introduce a specific check that prevents an accesses from spilling across
>>> two devices.
>>>
>>> Signed-off-by: Roger Pau Monné <[email protected]>
>>
>> Reviewed-by: Jan Beulich <[email protected]>
>> albeit with a remark:
>>
>>> --- a/xen/arch/x86/x86_64/mmconfig_64.c
>>> +++ b/xen/arch/x86/x86_64/mmconfig_64.c
>>> @@ -61,7 +61,8 @@ int pci_mmcfg_read(unsigned int seg, unsigned int bus,
>>>      char __iomem *addr;
>>>  
>>>      /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
>>> -    if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095))) {
>>> +    if (unlikely((bus > 255) || (devfn > 255) ||
>>> +                 (reg + len > PCI_CFG_SPACE_EXP_SIZE))) {
>>>  err:        *value = -1;
>>>          return -EINVAL;
>>>      }
>>> @@ -91,7 +92,8 @@ int pci_mmcfg_write(unsigned int seg, unsigned int bus,
>>>      char __iomem *addr;
>>>  
>>>      /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
>>> -    if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095)))
>>> +    if (unlikely((bus > 255) || (devfn > 255) ||
>>> +                 (reg + len > PCI_CFG_SPACE_EXP_SIZE)))
>>>          return -EINVAL;
>>>  
>>>      addr = pci_dev_base(seg, bus, devfn);
>>
>> In both cases the unlikely() uses won't have the intended effect, from all I
>> know. They would help as used here only if the compiler managed to fold all
>> three parts of the ||-expression into a single conditional branch, which I
>> don't think it would end up doing.
> 
> I don't mind dropping the unlikely() while changing the line.  I tend
> to leave those alone if present, even when I'm not sure they are
> actually helpful.

I think unlikely() is actually reasonable to have here, yet it would need to
be four of them, not just one. Unless reachable from a DomU-accessible path,
converting to BUG_ON() (or ASSERT_UNREACHABLE()) instead would also be an
option.

>  The comment ahead of the check is also not very
> useful IMO, but I've decided to leave it alone.

Indeed, that's an odd thing to survive over 20 years (I think).

Jan