Re: [PATCH v5 1/9] vpci: move BAR mapping permissions checks

On Thu, Jul 23, 2026 at 04:44:26PM +0200, Jan Beulich wrote:
> On 08.07.2026 23:02, Stewart Hildebrand wrote:
> > There's no need to defer the permissions checks. Perform them right away
> > in modify_bars().
> > 
> > As a result of the move, the permissions checks will now cover the whole
> > BAR before any holes are removed.
> 
> Is this a good idea though? A really good toolstack would avoid granting
> access to e.g. the MSI-X table.
> 
> > Carry over the domain_crash() in the error path for domUs.
> 
> I don't think this is warranted.
> 
> > --- a/xen/drivers/vpci/header.c
> > +++ b/xen/drivers/vpci/header.c
> > @@ -58,24 +58,6 @@ static int cf_check map_range(
> >           * offset of the current address from the BAR start.
> >           */
> >          unsigned long map_mfn = start_mfn + s - start_gfn;
> > -        unsigned long m_end = map_mfn + size - 1;
> > -
> > -        if ( !iomem_access_permitted(map->d, map_mfn, m_end) )
> > -        {
> > -            printk(XENLOG_G_WARNING
> > -                   "%pd denied access to MMIO range [%#lx, %#lx]\n",
> > -                   map->d, map_mfn, m_end);
> > -            return -EPERM;
> > -        }
> > -
> > -        rc = xsm_iomem_mapping_vpci(XSM_HOOK, map->d, map_mfn, m_end, map->map);
> > -        if ( rc )
> > -        {
> > -            printk(XENLOG_G_WARNING
> > -                   "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
> > -                   map->d, map_mfn, m_end, rc);
> 
> I realize you literally only take what's here, but ...
> 
> > @@ -369,6 +351,28 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
> >              return -EINVAL;
> >          }
> >  
> > +        if ( !iomem_access_permitted(pdev->domain, start, end) )
> > +        {
> > +            printk(XENLOG_G_WARNING
> > +                   "%pd denied access to MMIO range [%#lx, %#lx]\n",
> > +                   pdev->domain, start, end);
> > +            if ( !is_hardware_domain(pdev->domain) )
> > +                domain_crash(pdev->domain);
> > +            return -EPERM;
> > +        }
> > +
> > +        rc = xsm_iomem_mapping_vpci(XSM_HOOK, pdev->domain, start, end,
> > +                                    !!(cmd & PCI_COMMAND_MEMORY));
> > +        if ( rc )
> > +        {
> > +            printk(XENLOG_G_WARNING
> > +                   "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
> > +                   pdev->domain, start, end, rc);
> 
> ... is the logging of rc actually useful? Afaict it's only ever going to be
> -EPERM.

I think Flask can also return -EACCES, not that it's very relevant I'm
afraid.  It's likely too late now, but those XSM checks should instead
return a bool to avoid this kind of confusion with the returned
error? (or whether diverse return codes are indeed expected).

Thanks, Roger.