[RFC PATCH 0/2] Fix forward type=hostdev nets for apparmor
I'm working on a fix for 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 I'm not sure if the attached patches are sufficient. When I compare the apparmor permissions file for a guest with a VF shared via forward type='hostdev' vs. the same guest with a VF shared via a PCI hostdev device, the latter has extra apparmor permissions for files like: "/sys/devices/pci0000:00/0000:00:1d.0/0000:03:10.0/resource3_wc" rw, "/sys/devices/pci0000:00/0000:00:1d.0/0000:03:10.0/resource0_wc" rw, "/sys/devices/pci0000:00/0000:00:1d.0/0000:03:10.0/resource3" rw, "/sys/devices/pci0000:00/0000:00:1d.0/0000:03:10.0/vendor" rw, "/sys/devices/pci0000:00/0000:00:1d.0/0000:03:10.0/reset" rw, "/sys/devices/pci0000:00/0000:00:1d.0/0000:03:10.0/resource" rw, "/sys/devices/pci0000:00/0000:00:1d.0/0000:03:10.0/device" rw, "/sys/devices/pci0000:00/0000:00:1d.0/0000:03:10.0/resource0" rw, "/sys/devices/pci0000:00/0000:00:1d.0/0000:03:10.0/config" rw, ... however both guests appear to function in my test environment (Debian 13, 6.12.22-amd64) - i.e. both with and without those entries. So I don't know if those permissions are unneeded, or if they are granted at runtime instead. If those permissions are needed, then I'd appreciate any hints on how to modify virt-aa-helper to discover the PCI address. I appreciate that might not actually be possible because that is dynamically allocated, and so might race - so some other solution might be required. Many Thanks, Tim. Tim Small (2): virt-aa-helper: refactor for readability virt-aa-helper: Allow SR-IOV VF PCI for hostdev networks .../apparmor/usr.lib.libvirt.virt-aa-helper.in | 3 +++ src/security/virt-aa-helper.c | 18 ++++++++++++++---- 2 files changed, 17 insertions(+), 4 deletions(-) -- 2.47.2
Signed-off-by: Tim Small <tim@seoss.co.uk> --- src/security/virt-aa-helper.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index e3802c18be..fa69245324 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1134,10 +1134,9 @@ 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 Tue, May 06, 2025 at 17:00:10 +0100, Tim Small via Devel wrote:
Signed-off-by: Tim Small <tim@seoss.co.uk> --- src/security/virt-aa-helper.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
Note my comment about git config for authorship on 2/2; Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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 Downstream bug https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=993856 Signed-off-by: Tim Small <tim@seoss.co.uk> --- .../apparmor/usr.lib.libvirt.virt-aa-helper.in | 3 +++ src/security/virt-aa-helper.c | 11 +++++++++++ 2 files changed, 14 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..3b3d733b5e 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,9 @@ profile virt-aa-helper @libexecdir@/virt-aa-helper { @sysconfdir@/apparmor.d/libvirt/* r, @sysconfdir@/apparmor.d/libvirt/libvirt-@{UUID}* rw, + # allow network type lookup to check for forward type=hostdev networks + @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 fa69245324..7228292358 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1142,6 +1142,12 @@ get_files(vahControl * ctl) vhu->type) != 0) goto cleanup; } + /* Grant vfio for SR-IOV PCI VFs shared via <forward type='hostdev'> networks */ + if (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++) { @@ -1306,6 +1312,11 @@ get_files(vahControl * ctl) if (!virDomainNetIsVirtioModel(net)) continue; } + if (net && + net->type == VIR_DOMAIN_NET_TYPE_NETWORK && + virDomainNetResolveActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + continue; + } needsvhost = true; } } -- 2.47.2
On Tue, May 06, 2025 at 17:00:11 +0100, Tim Small via Devel wrote:
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
Downstream bug https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=993856
Signed-off-by: Tim Small <tim@seoss.co.uk>
---
Your email domain uses DMARC so our mailing list applied countermeasures. This means that your authorship of the patch would be wrong. Please configure git to always add the extra 'From:' field: https://www.libvirt.org/submitting-patches.html#git-configuration Notably: $ git config --global format.forceInBodyFrom true
.../apparmor/usr.lib.libvirt.virt-aa-helper.in | 3 +++ src/security/virt-aa-helper.c | 11 +++++++++++ 2 files changed, 14 insertions(+)
Also note that I'm no apparmor expert; I'm merely mentioning what I've noticed.
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..3b3d733b5e 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,9 @@ profile virt-aa-helper @libexecdir@/virt-aa-helper { @sysconfdir@/apparmor.d/libvirt/* r, @sysconfdir@/apparmor.d/libvirt/libvirt-@{UUID}* rw,
+ # allow network type lookup to check for forward type=hostdev networks
I presume this is needed to open outbound connections to the appropriate sub-daemon; so not specific to hostdev lookup. Preferrably reword it so that it states that the helper might be needing client connections.
+ @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 fa69245324..7228292358 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1142,6 +1142,12 @@ get_files(vahControl * ctl) vhu->type) != 0) goto cleanup; } + /* Grant vfio for SR-IOV PCI VFs shared via <forward type='hostdev'> networks */ + if (net && + net->type == VIR_DOMAIN_NET_TYPE_NETWORK && + virDomainNetResolveActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
The indentation is broken here. As 'virDomainNetResolveActualType' opens connection and does a pretty expensive lookup I'd suggest trying to check if given network is a hostdev only when 'needsVfio' is still false. Unfortunately this will need to be done for all the cases when this is a non-hostdev ...
+ needsVfio = true; + } }
for (i = 0; i < ctl->def->nmems; i++) { @@ -1306,6 +1312,11 @@ get_files(vahControl * ctl) if (!virDomainNetIsVirtioModel(net)) continue; } + if (net && + net->type == VIR_DOMAIN_NET_TYPE_NETWORK && + virDomainNetResolveActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
here too
+ continue;
IIUC this shouldn't be needed. By definition hostdev-based networks can't be 'virito' so they should have been already skipped by the above condition.
+ } needsvhost = true; } } -- 2.47.2
On 07/05/2025 08:47, Peter Krempa wrote:
On Tue, May 06, 2025 at 17:00:11 +0100, Tim Small via Devel wrote:
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
Hi Peter, Thanks for the review. I'll send a revised patch series shortly. Together with the comment below, they should address the points raised in your review I think...
@@ -1306,6 +1312,11 @@ get_files(vahControl * ctl) if (!virDomainNetIsVirtioModel(net)) continue; } + if (net && + net->type == VIR_DOMAIN_NET_TYPE_NETWORK && + virDomainNetResolveActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
here too
+ continue;
IIUC this shouldn't be needed. By definition hostdev-based networks can't be 'virito' so they should have been already skipped by the above condition.
An earlier check for virDomainNetGetModelString(net) returns false for conditional NIC virtual functions shared HOSTDEV type networks, so the virDomainNetIsVirtioModel(net) test isn't reached, and the continue is skipped. Updated formatting in the revised patch has resulted in the call to virDomainNetGetModelString(net) showing up in the context now. Thanks, Tim.
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. 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
Signed-off-by: Tim Small <tim@seoss.co.uk> --- 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
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> --- .../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
participants (2)
-
Peter Krempa -
Tim Small