[PATCH v3 0/2] Fix forward type=hostdev nets for apparmor
Fixes a bug whereby apparmor permissions aren't granted to allow a PCI SR-IOV virtual function to be used in a kvm guest when the VF is defined via a forward type='hostdev' network (as per the 'hostdev' option documented here: https://libvirt.org/formatnetwork.html#connectivity ). Downstream bug here: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=993856 qemu accesses these PCI virtual functions using the vfio API, so no additional permissions to access to the PCI device resources etc. via /sys/devices/pci[...]/resource et al. are necessary. This is a resend with fixed From in body for the patch emails, and change notes in patch emails. Thanks, Tim. Tim Small (2): virt-aa-helper: refactor for readability virt-aa-helper: Allow SR-IOV VF PCI for hostdev networks .../usr.lib.libvirt.virt-aa-helper.in | 4 +++ src/security/virt-aa-helper.c | 28 ++++++++++++++++--- 2 files changed, 28 insertions(+), 4 deletions(-) -- 2.47.2
From: Tim Small <tim@seoss.co.uk> Signed-off-by: Tim Small <tim@seoss.co.uk> --- Changes since earlier patch versions: Since V2: . Fix missing from line in patch body . Add this narrative Since V1: . Formatting - ref Peter Krempa's feedback src/security/virt-aa-helper.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index e3802c18be..6481e9cfd7 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1134,10 +1134,10 @@ get_files(vahControl * ctl) } for (i = 0; i < ctl->def->nnets; i++) { - if (ctl->def->nets[i] && - ctl->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER && - ctl->def->nets[i]->data.vhostuser) { - virDomainChrSourceDef *vhu = ctl->def->nets[i]->data.vhostuser; + virDomainNetDef *net = ctl->def->nets[i]; + + if (net && net->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER && net->data.vhostuser) { + virDomainChrSourceDef *vhu = net->data.vhostuser; if (vah_add_file_chardev(&buf, vhu->data.nix.path, "rw", vhu->type) != 0) -- 2.47.2
On 6/9/25 13:59, Tim Small via Devel wrote:
From: Tim Small <tim@seoss.co.uk>
Signed-off-by: Tim Small <tim@seoss.co.uk> ---
Changes since earlier patch versions:
Since V2: . Fix missing from line in patch body . Add this narrative
Since V1: . Formatting - ref Peter Krempa's feedback
src/security/virt-aa-helper.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index e3802c18be..6481e9cfd7 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1134,10 +1134,10 @@ get_files(vahControl * ctl) }
for (i = 0; i < ctl->def->nnets; i++) { - if (ctl->def->nets[i] && - ctl->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER && - ctl->def->nets[i]->data.vhostuser) { - virDomainChrSourceDef *vhu = ctl->def->nets[i]->data.vhostuser; + virDomainNetDef *net = ctl->def->nets[i]; + + if (net && net->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER && net->data.vhostuser) {
Pre-existing, but I always wondered how can ctl->def->nets[i] be NULL when we're iterating over nnets. It can't. I'll probably post a patch that removes these non-NULL checks in the whole function.
+ virDomainChrSourceDef *vhu = net->data.vhostuser;
if (vah_add_file_chardev(&buf, vhu->data.nix.path, "rw", vhu->type) != 0)
Michal
From: Tim Small <tim@seoss.co.uk> Add check for <forward type='hostdev'> networks which were previously neglected (as opposed to explicit PCI hostdev devices), so that they can be granted the necessary permissions for PCI device access. The network type lookup in-turn requires the helper to read libvirt.conf See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=993856 Signed-off-by: Tim Small <tim@seoss.co.uk> --- Changes since earlier patch versions: Since V2: . Fix missing from line in patch body . Add this narrative Since V1: . Formatting - ref Peter Krempa's feedback . Comments - ref Peter Krempa's feedback . Minimise calls to virDomainNetResolveActualType() since it obtains info via IPC calls - ref Peter Krempa's feedback .../usr.lib.libvirt.virt-aa-helper.in | 4 ++++ src/security/virt-aa-helper.c | 20 +++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in index e209a8bff7..4cbad6986d 100644 --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in @@ -49,6 +49,10 @@ profile virt-aa-helper @libexecdir@/virt-aa-helper { @sysconfdir@/apparmor.d/libvirt/* r, @sysconfdir@/apparmor.d/libvirt/libvirt-@{UUID}* rw, + # The helper may read libvirt.conf in the course of connecting to a running + # libvirt deamon e.g. to resolve network configuration for a given domain + @sysconfdir@/libvirt/libvirt.conf r, + # for backingstore -- allow access to non-hidden files in @{HOME} as well # as storage pools audit deny @{HOME}/.* mrwkl, diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 6481e9cfd7..f1d8feee11 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1143,6 +1143,16 @@ get_files(vahControl * ctl) vhu->type) != 0) goto cleanup; } + /* + * Grant vfio for SR-IOV PCI VFs shared via <forward type='hostdev'> + * networks. Calling virDomainNetResolveActualType() results in IPC. + */ + if (!needsVfio && + net && + net->type == VIR_DOMAIN_NET_TYPE_NETWORK && + virDomainNetResolveActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + needsVfio = true; + } } for (i = 0; i < ctl->def->nmems; i++) { @@ -1301,12 +1311,22 @@ get_files(vahControl * ctl) if (ctl->def->virtType == VIR_DOMAIN_VIRT_KVM) { for (i = 0; i < ctl->def->nnets; i++) { virDomainNetDef *net = ctl->def->nets[i]; + if (net && virDomainNetGetModelString(net)) { if (net->driver.virtio.name == VIR_DOMAIN_NET_DRIVER_TYPE_QEMU) continue; if (!virDomainNetIsVirtioModel(net)) continue; } + + /* n.b. Calling virDomainNetResolveActualType() results in IPC. */ + if (!needsvhost && + net && + net->type == VIR_DOMAIN_NET_TYPE_NETWORK && + virDomainNetResolveActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + continue; + } + needsvhost = true; } } -- 2.47.2
On 6/9/25 13:59, Tim Small via Devel wrote:
From: Tim Small <tim@seoss.co.uk>
Add check for <forward type='hostdev'> networks which were previously neglected (as opposed to explicit PCI hostdev devices), so that they can be granted the necessary permissions for PCI device access. The network type lookup in-turn requires the helper to read libvirt.conf
See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=993856
Signed-off-by: Tim Small <tim@seoss.co.uk> ---
Changes since earlier patch versions:
Since V2: . Fix missing from line in patch body . Add this narrative
Since V1: . Formatting - ref Peter Krempa's feedback . Comments - ref Peter Krempa's feedback . Minimise calls to virDomainNetResolveActualType() since it obtains info via IPC calls - ref Peter Krempa's feedback
.../usr.lib.libvirt.virt-aa-helper.in | 4 ++++ src/security/virt-aa-helper.c | 20 +++++++++++++++++++ 2 files changed, 24 insertions(+)
diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in index e209a8bff7..4cbad6986d 100644 --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in @@ -49,6 +49,10 @@ profile virt-aa-helper @libexecdir@/virt-aa-helper { @sysconfdir@/apparmor.d/libvirt/* r, @sysconfdir@/apparmor.d/libvirt/libvirt-@{UUID}* rw,
+ # The helper may read libvirt.conf in the course of connecting to a running + # libvirt deamon e.g. to resolve network configuration for a given domain + @sysconfdir@/libvirt/libvirt.conf r, + # for backingstore -- allow access to non-hidden files in @{HOME} as well # as storage pools audit deny @{HOME}/.* mrwkl, diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 6481e9cfd7..f1d8feee11 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1143,6 +1143,16 @@ get_files(vahControl * ctl) vhu->type) != 0) goto cleanup; } + /*
Nit pick - put an empty line after the previous block and before starting this comment. Also, there's no need to start the comment with an empty line.
+ * Grant vfio for SR-IOV PCI VFs shared via <forward type='hostdev'> + * networks. Calling virDomainNetResolveActualType() results in IPC. + */ + if (!needsVfio && + net && + net->type == VIR_DOMAIN_NET_TYPE_NETWORK && + virDomainNetResolveActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + needsVfio = true; + } }
for (i = 0; i < ctl->def->nmems; i++) { @@ -1301,12 +1311,22 @@ get_files(vahControl * ctl) if (ctl->def->virtType == VIR_DOMAIN_VIRT_KVM) { for (i = 0; i < ctl->def->nnets; i++) { virDomainNetDef *net = ctl->def->nets[i]; + if (net && virDomainNetGetModelString(net)) { if (net->driver.virtio.name == VIR_DOMAIN_NET_DRIVER_TYPE_QEMU) continue; if (!virDomainNetIsVirtioModel(net)) continue; } + + /* n.b. Calling virDomainNetResolveActualType() results in IPC. */ + if (!needsvhost && + net && + net->type == VIR_DOMAIN_NET_TYPE_NETWORK && + virDomainNetResolveActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + continue; + } + needsvhost = true; } }
But this new IPC got me thinking - the XML we're parsing here in the aa helper is formatted by libvirtd who already know the actual type of each net. But looking into the code - this is lost when the daemon talks to the helper. I'm not on an AppArmor machine right now, so can you please check whether the following change (applied on the top of this very patch) works? It would save us from those IPCs. diff --git i/src/security/security_apparmor.c w/src/security/security_apparmor.c index 68ac39611f..90e4488b1f 100644 --- i/src/security/security_apparmor.c +++ w/src/security/security_apparmor.c @@ -156,6 +156,7 @@ load_profile(virSecurityManager *mgr G_GNUC_UNUSED, if (virDomainDefFormatInternal(def, NULL, &buf, VIR_DOMAIN_DEF_FORMAT_SECURE | + VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET | VIR_DOMAIN_DEF_FORMAT_VOLUME_TRANSLATED) < 0) return -1; diff --git i/src/security/virt-aa-helper.c w/src/security/virt-aa-helper.c index f1d8feee11..00cea448db 100644 --- i/src/security/virt-aa-helper.c +++ w/src/security/virt-aa-helper.c @@ -643,6 +643,7 @@ get_definition(vahControl * ctl, const char *xmlStr) ctl->xmlopt, NULL, VIR_DOMAIN_DEF_PARSE_SKIP_SECLABEL | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE | + VIR_DOMAIN_DEF_PARSE_ACTUAL_NET | VIR_DOMAIN_DEF_PARSE_VOLUME_TRANSLATED); if (ctl->def == NULL) { @@ -1143,14 +1144,13 @@ get_files(vahControl * ctl) vhu->type) != 0) goto cleanup; } - /* - * Grant vfio for SR-IOV PCI VFs shared via <forward type='hostdev'> + + /* Grant vfio for SR-IOV PCI VFs shared via <forward type='hostdev'> * networks. Calling virDomainNetResolveActualType() results in IPC. */ if (!needsVfio && net && - net->type == VIR_DOMAIN_NET_TYPE_NETWORK && - virDomainNetResolveActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { needsVfio = true; } } @@ -1319,11 +1319,10 @@ get_files(vahControl * ctl) continue; } - /* n.b. Calling virDomainNetResolveActualType() results in IPC. */ + /* N.B. calling virDomainNetResolveActualType() results in IPC. */ if (!needsvhost && net && - net->type == VIR_DOMAIN_NET_TYPE_NETWORK && - virDomainNetResolveActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV){ continue; } Michal
participants (2)
-
Michal Prívozník -
Tim Small