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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

Jan