Re: [PATCH for-4.22? 8/9] x86/domctl: don't imply I/O port permissions from I/O port mapping

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jun 16, 2026 at 11:36:53AM +0200, Jan Beulich wrote:
> On 16.06.2026 11:21, Roger Pau Monné wrote:
> > On Mon, Jun 15, 2026 at 04:16:11PM +0200, Jan Beulich wrote:
> >> Rather than granting permissions when mapping (an operation that DM-s are
> >> allowed to carry out, while they can't invoke ioport-permission), check
> >> whether permissions actually were granted when adding a mapping. This then
> >> also allows relaxing the necessary locking.
> >>
> >> Fixes: 192c4dabc344 ("domctl and p2m changes for PCI passthru")
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >> ---
> >> libxl has libxl__grant_vga_iomem_permission(), but I can't spot any I/O
> >> port equivalent (nor a revoke counterpart, btw). Everywhere else MMIO and
> >> I/O ports look to be treated equally.
> >>
> >> Qemu uses both xc_domain_{iomem_permission,memory_mapping}() in
> >> igd_write_opregion(), but only xc_domain_{memory,ioport}_mapping() in
> >> xen_pt_region_update() and xen_pt_{,un}register_vga_regions(). Is the IGD
> >> region special in any way? Clearly this can't work from a stubdom.
> 
> Both of these remarks are relevant to your response below. I realize I should
> have Cc-ed Anthony, for him to comment on them.

Partially yes, but those are only for the callers we know.  My comment
was thinking about possible out-of-tree users.

> >> --- a/xen/arch/x86/domctl.c
> >> +++ b/xen/arch/x86/domctl.c
> >> @@ -714,9 +714,14 @@ long arch_do_domctl(
> >>              break;
> >>  
> >>          hvm = &d->arch.hvm;
> >> -        iocaps_double_lock(d, true);
> >> +        /*
> >> +         * NB: The double lock isn't really needed when !add, but is used 
> >> anyway
> >> +         * to keep things simple.
> >> +         */
> >> +        iocaps_double_lock(d, false);
> >>  
> >> -        if ( !ioports_access_permitted(currd, fmp, fmp + np - 1) )
> >> +        if ( !ioports_access_permitted(currd, fmp, fmp + np - 1) ||
> >> +             (add && !ioports_access_permitted(d, fmp, fmp + np - 1)) )
> >>              ret = -EPERM;
> >>          else if ( add )
> >>          {
> >> @@ -747,15 +752,6 @@ long arch_do_domctl(
> >>                  list_add_tail(&g2m_ioport->list, &hvm->g2m_ioport_list);
> >>              }
> >>              write_unlock(&hvm->g2m_ioport_lock);
> >> -            if ( !ret )
> >> -                ret = ioports_permit_access(d, fmp, fmp + np - 1);
> >> -            if ( ret && !found && g2m_ioport )
> >> -            {
> >> -                write_lock(&hvm->g2m_ioport_lock);
> >> -                list_del(&g2m_ioport->list);
> >> -                write_unlock(&hvm->g2m_ioport_lock);
> >> -                xfree(g2m_ioport);
> >> -            }
> >>          }
> >>          else
> >>          {
> >> @@ -772,15 +768,9 @@ long arch_do_domctl(
> >>                      break;
> >>                  }
> >>              write_unlock(&hvm->g2m_ioport_lock);
> >> -
> >> -            ret = ioports_deny_access(d, fmp, fmp + np - 1);
> >> -            if ( ret && is_hardware_domain(currd) )
> >> -                printk(XENLOG_ERR
> >> -                       "ioport_map: error %ld denying dom%d access to 
> >> [%x,%x]\n",
> >> -                       ret, d->domain_id, fmp, fmp + np - 1);
> >>          }
> >>  
> >> -        iocaps_double_unlock(d, true);
> >> +        iocaps_double_unlock(d, false);
> > 
> > I think the new behavior is more sane, however the problematic aspect
> > of this change is the removal case IMO: we cannot be sure whether
> > existing callers rely on XEN_DOMCTL_ioport_mapping also removing the
> > permissions, and hence Xen no longer removing the permissions might
> > lead to leaks.
> > 
> > This is a risk we might be willing to take, but it must be stated in
> > the commit message.
> 
> I've added
> 
> "While no longer granting permissions upon mapping is "only" at risk of
>  breaking guests, no longer revoking permissions upon unmapping strictly
>  requires callers to additionally invoke XEN_DOMCTL_ioport_permission. Or
>  else a security issue would arise. In-tree code already does so."
> 
> >  And likely in a CHANGELOG entry so that external
> > consumers are aware of this change and can adjust as necessary.
> 
> Will do.

Thanks, Roger.