[PATCH v3 0/2] x86: remaining XSA-492 and -491 follow-on

[PATCH v3 0/2] x86: remaining XSA-492 and -491 follow-on

[Jan Beulich]

1: x86/domctl: don't imply I/O port permissions from I/O port mapping
2: x86/HVM: more checking for XEN_DOMCTL_ioport_mapping

Jan

[PATCH v3 1/2] x86/domctl: don't imply I/O port permissions from I/O port mapping

[Jan Beulich]

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@suse.com>
---
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.
---
v3: Further extend ChangeLog entry.
v2: Avoid double evaluation of "add". Add ChangeLog entry.

--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,11 @@ The format is based on [Keep a Changelog
 ## [4.23.0 UNRELEASED](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=staging) - TBD
 
 ### Changed
+ - On x86:
+   - XEN_DOMCTL_ioport_mapping no longer implicitly grants or revokes
+     permissions for the port range in question.
+     XEN_DOMCTL_ioport_permission now needs invoking up front /
+     afterwards.
 
 ### Added
 
--- 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;
 
-        iocaps_double_unlock(d, true);
+        iocaps_double_unlock(d, false);
         break;
     }
 

[PATCH v3 2/2] x86/HVM: more checking for XEN_DOMCTL_ioport_mapping

[Jan Beulich]

When adding ranges, only alter existing ones when there is an exact match.
Don't accept ranges overlapping existing ones.

When removing ranges, only remove a range if there's an exact match.
Return an error when the range isn't found.

Fixes: 192c4dabc344 ("domctl and p2m changes for PCI passthru")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
---
Should "exact match" perhaps also include the guest port number? I'm
uncertain here as that kind of conflicts with "add" being treated as
"change" when the host port (and now count) match.
---
v2: Re-base over change to earlier patch.

--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -728,12 +728,14 @@ long arch_do_domctl(
                    "ioport_map:remove: %pd gport=%x mport=%x nr=%x\n",
                    d, fgp, fmp, np);
 
+            ret = -ENOENT;
             write_lock(&hvm->g2m_ioport_lock);
             list_for_each_entry(g2m_ioport, &hvm->g2m_ioport_list, list)
-                if ( g2m_ioport->mport == fmp )
+                if ( g2m_ioport->mport == fmp && g2m_ioport->np == np )
                 {
                     list_del(&g2m_ioport->list);
                     xfree(g2m_ioport);
+                    ret = 0;
                     break;
                 }
             write_unlock(&hvm->g2m_ioport_lock);
@@ -746,14 +748,21 @@ long arch_do_domctl(
 
             write_lock(&hvm->g2m_ioport_lock);
             list_for_each_entry(g2m_ioport, &hvm->g2m_ioport_list, list)
-                if (g2m_ioport->mport == fmp )
+            {
+                if ( g2m_ioport->mport == fmp && g2m_ioport->np == np )
                 {
                     g2m_ioport->gport = fgp;
-                    g2m_ioport->np = np;
                     found = 1;
                     break;
                 }
-            if ( !found )
+                if ( fmp + np >= g2m_ioport->mport &&
+                     g2m_ioport->mport + g2m_ioport->np >= fmp )
+                {
+                    ret = -EBUSY;
+                    break;
+                }
+            }
+            if ( !found && !ret )
             {
                 g2m_ioport = xmalloc(struct g2m_ioport);
                 if ( !g2m_ioport )

Re: [PATCH v3 1/2] x86/domctl: don't imply I/O port permissions from I/O port mapping

[Roger Pau Monné]

On Tue, Jun 30, 2026 at 03:54:04PM +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@suse.com>

It's difficult to not think about someone appearing in 3 or 4 years
complaining that this change in behavior has caused them a security
issue, but I do agree that the previous behavior was inconsistent at
best:

Acked-by: Roger Pau Monné <roger@xenproject.org>

However you need to bump XEN_DOMCTL_INTERFACE_VERSION to note the ABI
change as we are in 4.23 now.

We possibly need to sort out the questions below, in case there are
changes required to either libxl or QEMU.

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

Those possibly need to be answered by Anthony, but he isn't on Cc?

Thanks, Roger.

Re: [PATCH v3 1/2] x86/domctl: don't imply I/O port permissions from I/O port mapping

[Jan Beulich]

On 21.07.2026 16:49, Roger Pau Monné wrote:
> On Tue, Jun 30, 2026 at 03:54:04PM +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@suse.com>
> 
> It's difficult to not think about someone appearing in 3 or 4 years
> complaining that this change in behavior has caused them a security
> issue, but I do agree that the previous behavior was inconsistent at
> best:

A positive aspect here is that the similar change for MMIO went without
any fallout, as far as I'm aware.

> Acked-by: Roger Pau Monné <roger@xenproject.org>

Thanks, but ...

> However you need to bump XEN_DOMCTL_INTERFACE_VERSION to note the ABI
> change as we are in 4.23 now.

... there's no interface change here. A bump therefore won't buy us
anything. I'm hesitant to take your ack as long as we disagree here.

> We possibly need to sort out the questions below, in case there are
> changes required to either libxl or QEMU.

Indeed.

>> ---
>> 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.
> 
> Those possibly need to be answered by Anthony, but he isn't on Cc?

Oh, I had him Cc-ed on v2, but forgot to on v3. Anthony, your input is much
appreciated.

Jan

Re: [PATCH v3 1/2] x86/domctl: don't imply I/O port permissions from I/O port mapping

[Roger Pau Monné]

On Tue, Jul 21, 2026 at 04:57:36PM +0200, Jan Beulich wrote:
> On 21.07.2026 16:49, Roger Pau Monné wrote:
> > On Tue, Jun 30, 2026 at 03:54:04PM +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@suse.com>
> > 
> > It's difficult to not think about someone appearing in 3 or 4 years
> > complaining that this change in behavior has caused them a security
> > issue, but I do agree that the previous behavior was inconsistent at
> > best:
> 
> A positive aspect here is that the similar change for MMIO went without
> any fallout, as far as I'm aware.
> 
> > Acked-by: Roger Pau Monné <roger@xenproject.org>
> 
> Thanks, but ...
> 
> > However you need to bump XEN_DOMCTL_INTERFACE_VERSION to note the ABI
> > change as we are in 4.23 now.
> 
> ... there's no interface change here. A bump therefore won't buy us
> anything. I'm hesitant to take your ack as long as we disagree here.

It's not an interface change, but it's a change in behavior of an
existing hypercall in a security-relevant way, so people would
generally need to be aware of it, just in case.

Thanks, Roger.

Re: [PATCH v3 1/2] x86/domctl: don't imply I/O port permissions from I/O port mapping

[Jan Beulich]

On 21.07.2026 17:07, Roger Pau Monné wrote:
> On Tue, Jul 21, 2026 at 04:57:36PM +0200, Jan Beulich wrote:
>> On 21.07.2026 16:49, Roger Pau Monné wrote:
>>> On Tue, Jun 30, 2026 at 03:54:04PM +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@suse.com>
>>>
>>> It's difficult to not think about someone appearing in 3 or 4 years
>>> complaining that this change in behavior has caused them a security
>>> issue, but I do agree that the previous behavior was inconsistent at
>>> best:
>>
>> A positive aspect here is that the similar change for MMIO went without
>> any fallout, as far as I'm aware.
>>
>>> Acked-by: Roger Pau Monné <roger@xenproject.org>
>>
>> Thanks, but ...
>>
>>> However you need to bump XEN_DOMCTL_INTERFACE_VERSION to note the ABI
>>> change as we are in 4.23 now.
>>
>> ... there's no interface change here. A bump therefore won't buy us
>> anything. I'm hesitant to take your ack as long as we disagree here.
> 
> It's not an interface change, but it's a change in behavior of an
> existing hypercall in a security-relevant way, so people would
> generally need to be aware of it, just in case.

But the interface version bump won't necessarily make anyone aware. Things
will continue to build as before, and there's also no binary incompatibility.
The best way of making people aware is the ChangeLog entry that iirc you had
asked for (and that's now there).

Jan

Re: [PATCH v3 1/2] x86/domctl: don't imply I/O port permissions from I/O port mapping

[Jan Beulich]

On 21.07.2026 17:34, Jan Beulich wrote:
> On 21.07.2026 17:07, Roger Pau Monné wrote:
>> On Tue, Jul 21, 2026 at 04:57:36PM +0200, Jan Beulich wrote:
>>> On 21.07.2026 16:49, Roger Pau Monné wrote:
>>>> On Tue, Jun 30, 2026 at 03:54:04PM +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@suse.com>
>>>>
>>>> It's difficult to not think about someone appearing in 3 or 4 years
>>>> complaining that this change in behavior has caused them a security
>>>> issue, but I do agree that the previous behavior was inconsistent at
>>>> best:
>>>
>>> A positive aspect here is that the similar change for MMIO went without
>>> any fallout, as far as I'm aware.
>>>
>>>> Acked-by: Roger Pau Monné <roger@xenproject.org>
>>>
>>> Thanks, but ...
>>>
>>>> However you need to bump XEN_DOMCTL_INTERFACE_VERSION to note the ABI
>>>> change as we are in 4.23 now.
>>>
>>> ... there's no interface change here. A bump therefore won't buy us
>>> anything. I'm hesitant to take your ack as long as we disagree here.
>>
>> It's not an interface change, but it's a change in behavior of an
>> existing hypercall in a security-relevant way, so people would
>> generally need to be aware of it, just in case.
> 
> But the interface version bump won't necessarily make anyone aware. Things
> will continue to build as before, and there's also no binary incompatibility.
> The best way of making people aware is the ChangeLog entry that iirc you had
> asked for (and that's now there).

I should probably add that bumping the interface version now also wouldn't do
any harm; there's at least one other series pending which wants to do this
anyway in this starting release cycle. Yet I still think the bumping doesn't
quite belong here.

Jan

Re: [PATCH v3 1/2] x86/domctl: don't imply I/O port permissions from I/O port mapping

[Roger Pau Monné]

On Tue, Jul 21, 2026 at 05:36:37PM +0200, Jan Beulich wrote:
> On 21.07.2026 17:34, Jan Beulich wrote:
> > On 21.07.2026 17:07, Roger Pau Monné wrote:
> >> On Tue, Jul 21, 2026 at 04:57:36PM +0200, Jan Beulich wrote:
> >>> On 21.07.2026 16:49, Roger Pau Monné wrote:
> >>>> On Tue, Jun 30, 2026 at 03:54:04PM +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@suse.com>
> >>>>
> >>>> It's difficult to not think about someone appearing in 3 or 4 years
> >>>> complaining that this change in behavior has caused them a security
> >>>> issue, but I do agree that the previous behavior was inconsistent at
> >>>> best:
> >>>
> >>> A positive aspect here is that the similar change for MMIO went without
> >>> any fallout, as far as I'm aware.
> >>>
> >>>> Acked-by: Roger Pau Monné <roger@xenproject.org>
> >>>
> >>> Thanks, but ...
> >>>
> >>>> However you need to bump XEN_DOMCTL_INTERFACE_VERSION to note the ABI
> >>>> change as we are in 4.23 now.
> >>>
> >>> ... there's no interface change here. A bump therefore won't buy us
> >>> anything. I'm hesitant to take your ack as long as we disagree here.
> >>
> >> It's not an interface change, but it's a change in behavior of an
> >> existing hypercall in a security-relevant way, so people would
> >> generally need to be aware of it, just in case.
> > 
> > But the interface version bump won't necessarily make anyone aware. Things
> > will continue to build as before, and there's also no binary incompatibility.
> > The best way of making people aware is the ChangeLog entry that iirc you had
> > asked for (and that's now there).
> 
> I should probably add that bumping the interface version now also wouldn't do
> any harm; there's at least one other series pending which wants to do this
> anyway in this starting release cycle. Yet I still think the bumping doesn't
> quite belong here.

I was going to say - it's very likely the interface will be bumped
anyway.  I'm not going to insist, the text in domctl.h does mention
that the interface should only be bumped "in case the interface is
modified in an incompatible way", and you might argue there's no
interface change here at all, as the interface stays the same, what
changes is the underlying implementation.

Thanks, Roger.