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

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

Overall I would defer this change to the start of the 4.23 development
window, and commit it then.  It's IMO a bit risky to change the
interface behavior so late in the development process.

On Wed, Jun 17, 2026 at 11:30:04AM +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.
> 
> 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.
> 
> While there switch to using %pd in the two log messages.
> 
> 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.

Hm, I'm unsure that code will work correctly after the change here, as
xen_pt_register_vga_regions() doesn't grant access to the IO/memory
regions to the remote domain ahead of assigning them?

> ---
> v2: Avoid double evaluation of "add". Add ChangeLog entry.
> 
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -14,6 +14,9 @@ The format is based on [Keep a Changelog
>   - On x86:
>     - Enable pf-fixup option by default for PVH dom0.
>     - The libxenguest bzImage loader now uses the system liblz4 library.
> +   - XEN_DOMCTL_ioport_mapping no longer implicitly grants permissions for 
> the

I would explicitly mention access revocation also, FTAOD:

"XEN_DOMCTL_ioport_mapping no longer implicitly grants or revokes
permissions ..."

> +     port range in question.  XEN_DOMCTL_ioport_permission now needs invoking
> +     up front.
>  
>  ### Added
>   - Support for per-domain Xenstore quota in C xenstored (includes
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -714,15 +714,35 @@ 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) )
>              ret = -EPERM;
> -        else if ( add )
> +        else if ( !add )
>          {
>              printk(XENLOG_G_INFO
> -                   "ioport_map:add: dom%d gport=%x mport=%x nr=%x\n",
> -                   d->domain_id, fgp, fmp, np);
> +                   "ioport_map:remove: %pd gport=%x mport=%x nr=%x\n",
> +                   d, fgp, fmp, np);
> +
> +            write_lock(&hvm->g2m_ioport_lock);
> +            list_for_each_entry(g2m_ioport, &hvm->g2m_ioport_list, list)
> +                if ( g2m_ioport->mport == fmp )
> +                {
> +                    list_del(&g2m_ioport->list);
> +                    xfree(g2m_ioport);
> +                    break;
> +                }
> +            write_unlock(&hvm->g2m_ioport_lock);
> +        }
> +        else if ( ioports_access_permitted(d, fmp, fmp + np - 1) )
> +        {
> +            printk(XENLOG_G_INFO
> +                   "ioport_map:add: %pd gport=%x mport=%x nr=%x\n",
> +                   d, fgp, fmp, np);
>  
>              write_lock(&hvm->g2m_ioport_lock);
>              list_for_each_entry(g2m_ioport, &hvm->g2m_ioport_list, list)
> @@ -747,40 +767,11 @@ 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
> -        {
> -            printk(XENLOG_G_INFO
> -                   "ioport_map:remove: dom%d gport=%x mport=%x nr=%x\n",
> -                   d->domain_id, fgp, fmp, np);
> -
> -            write_lock(&hvm->g2m_ioport_lock);
> -            list_for_each_entry(g2m_ioport, &hvm->g2m_ioport_list, list)
> -                if ( g2m_ioport->mport == fmp )
> -                {
> -                    list_del(&g2m_ioport->list);
> -                    xfree(g2m_ioport);
> -                    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);
> -        }
> +            ret = -EPERM;

Should we add a dprintk here at least, to make it easy to identify
what has gone wrong from just looking at the dmesg?

Thanks, Roger.