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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.06.2026 17:18, Roger Pau Monné wrote:
> 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.

I share the concern, yet the Fixes: tags suggest this (and the subsequent
change) wants backporting. Which means we'll "gain" the behavioral change
in minor releases then anyway. IOW - I'm of two minds here.

> On Wed, Jun 17, 2026 at 11:30:04AM +0200, Jan Beulich wrote:
>> ---
>> 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?


Hence the remark, and the desire to get input from Anthony. Aiui there
either already is an issue there as of 0561e1f01e87 ("xen/common: do not
implicitly permit access to mapped I/O memory"), from over 10 years ago.
Or there is none here either.

>> --- 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 ..."

Sure, and then also ...

>> +     port range in question.  XEN_DOMCTL_ioport_permission now needs 
>> invoking
>> +     up front.

... "up front / afterwards."

>> --- 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;

For the comment below, note that there hasn't been any log message here.
Hence ...

>> @@ -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?

... I'm pretty uncertain towards emitting one here. Similarly
XEN_DOMCTL_memory_mapping doesn't emit a log message if either of the
two iomem_access_permitted() fail.

Jan