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 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.
> 
> --- 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.  And likely in a CHANGELOG entry so that external
consumers are aware of this change and can adjust as necessary.

Thanks, Roger.