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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 7/23/26 10:57 AM, Roger Pau Monné wrote:
> 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).
An XSM access check may return -EACCES as one of any number of error 
codes depending on which XSM module is loaded. I haven't traced this 
specific path but a quick grep for the direct FLASK hooks shows EPERM, 
EINVAL, ENOMEM, and ENOENT directly in the hooks themselves. This does 
not even include the error codes that could come from the AVC. This is a 
consist interface provided by all XSM calls and fairly standard behavior 
across any number of respectable kernels such as Linux.
v/r,
dps