From 3845d3854d699feabf637580f0aa9701f209b6a7 Mon Sep 17 00:00:00 2001 From: Jan Beulich Date: Wed, 22 Jul 2026 09:29:09 +0200 Subject: [PATCH] x86/domctl: don't imply I/O port permissions from I/O port mapping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Acked-by: Roger Pau Monné Acked-by: Oleksii Kurochko --- CHANGELOG.md | 5 ++++ xen/arch/x86/domctl.c | 61 ++++++++++++++++++------------------------- 2 files changed, 31 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 356be88351..1d3d7e0d87 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) ### Changed - XEN_DOMCTL_DEV_DT's, FLASK_[GS]ETBOOL's, and FLASK_DEVICETREE_LABEL's input string sizes need to include the nul terminator. + - 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 diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index 2ecd81d89b..3100f4283c 100644 --- 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); + ret = -EPERM; - 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); - } - - iocaps_double_unlock(d, true); + iocaps_double_unlock(d, false); break; }