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

On Wed, Aug 12, 2026 at 08:28:50AM +0200, Jan Beulich wrote:
> 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.

Hm, let's do that in a separate patch, I don't want to merge it with
the work here.

> >  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).

I can also take care of the comment there.

Thanks, Roger.