7 p.m.
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
7 p.m.
New subject: [RFC PATCH 1/2] virt-aa-helper: refactor for readability
Signed-off-by: Tim Small <tim(a)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
10:50 a.m.
New subject: [RFC PATCH 1/2] virt-aa-helper: refactor for readability
On Tue, May 06, 2025 at 17:00:10 +0100, Tim Small via Devel wrote:
Signed-off-by: Tim Small <tim(a)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(a)redhat.com>
7 p.m.
New subject: [RFC PATCH 2/2] virt-aa-helper: Allow SR-IOV VF PCI for hostdev networks
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(a)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(a)/apparmor.d/libvirt/* r,
@sysconfdir@/apparmor.d/libvirt/libvirt-(a){UUID}* rw,
+ # allow network type lookup to check for forward type=hostdev networks
+ @sysconfdir(a)/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
10:47 a.m.
New subject: [RFC PATCH 2/2] virt-aa-helper: Allow SR-IOV VF PCI for hostdev
networks
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(a)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(a)/apparmor.d/libvirt/* r,
@sysconfdir@/apparmor.d/libvirt/libvirt-(a){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(a)/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
6:12 p.m.
New subject: [RFC PATCH 2/2] virt-aa-helper: Allow SR-IOV VF PCI for hostdev
networks
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.
6:12 p.m.
New subject: [PATCH v2 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.
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
6:12 p.m.
New subject: [PATCH v2 1/2] virt-aa-helper: refactor for readability
Signed-off-by: Tim Small <tim(a)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
6:12 p.m.
New subject: [PATCH v2 2/2] virt-aa-helper: Allow SR-IOV VF PCI for hostdev networks
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(a)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(a)/apparmor.d/libvirt/* r,
@sysconfdir@/apparmor.d/libvirt/libvirt-(a){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(a)/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
49
days inactive
73
days old
8 comments
2 participants
participants (2)
-
Peter Krempa -
Tim Small