Series comparison

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

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 XXXXXXX..XXXXXXX 100644
--- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
+++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -XXX,XX +XXX,XX @@ 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++) {
@@ -XXX,XX +XXX,XX @@ 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

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 XXXXXXX..XXXXXXX 100644
--- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
+++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -XXX,XX +XXX,XX @@ 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++) {
@@ -XXX,XX +XXX,XX @@ 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

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 XXXXXXX..XXXXXXX 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -XXX,XX +XXX,XX @@ 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

From: Tim Small <tim@seoss.co.uk>

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(+)