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.