virt-aa-helper to learn about VF devspec paths

[Christian Ehrhardt ]

When on ppc64el attaching a Virtual function there is an error seen in dmesg.

[ 1124.853295] audit: type=1400 audit(1491468789.604:37): apparmor="DENIED" operation="open" profile="libvirt-88b15add-b290-431d-9e49-fa771588f2f5" name="/sys/devices/pci0005:00/0005:00:00.0/0005:01:01.3/devspec" pid=10779 comm="qemu-system-ppc" requested_mask="r" denied_mask="r" fsuid=64055 ouid=0

This seems to be multiple levels of wrong, but good messages first - it is non Fatal for the cases I've seen so far.

On ppc when attaching virtual functions it passes the function spapr_phb_vfio_get_loc_code.
In there is the offending call:
  g_strdup_printf("/sys/bus/pci/devices/%s/devspec", host);
Host here is the pci device that is about to be attached.

I'm 98% convinced that even if would passing that the following call is broken then still.
g_strdup_printf("/proc/device-tree%s/ibm,loc-code", buf);
IMHO should be
g_strdup_printf("/proc/device-tree/%s/ibm,loc-code", buf);

Assuming the likely bug above would be fixed along the commit the one fixing it should also extend virt-aa-helper.c to generate entries for the two paths that will be accessed:

So overall three todos:
1. fix device-tree path string
2. add device tree path to virt-aa-helper
3. add loc-code path to virt-aa-helper

Once that is upstream I'm happy to pick and backport if applicable.

So far this is non fatal since spapr_phb_get_loc_code has fallback code.

And IMHO - due to the latter issue even without the apparmor block so far always the fallback code is used. If that is true and really working everywhere one might consider dropping all the logic and only leaving the fallback?

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2017-04-13 21:09 EDT-------
Please, reverse mirror LP1680386 (virt-aa-helper to learn about VF devspec paths).

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2017-09-15 01:12 EDT-------
When passing through a pci-device, seeing these messages in guest, though pass-through is successful.

Host: Ubuntu 17.10
# uname -a
Linux pkvmhab010 4.12.0-12-generic #13-Ubuntu SMP Thu Aug 17 16:10:48 UTC 2017 ppc64le ppc64le ppc64le GNU/Linux

Guest: Ubuntu 17.10

Guest Dmesg:
-------------------
[Thu Sep 14 00:51:55 2017] audit: type=1400 audit(1505368316.084:3): apparmor="STATUS" operation="profile_load" profile="unconfined" name="/usr/lib/snapd/snap-confine" pid=1349 comm="apparmor_parser"
[Thu Sep 14 00:51:55 2017] audit: type=1400 audit(1505368316.084:4): apparmor="STATUS" operation="profile_load" profile="unconfined" name="/usr/lib/snapd/snap-confine//mount-namespace-capture-helper" pid=1349 comm="apparmor_parser"
[Thu Sep 14 00:51:55 2017] audit: type=1400 audit(1505368316.092:5): apparmor="STATUS" operation="profile_load" profile="unconfined" name="/sbin/dhclient" pid=1347 comm="apparmor_parser"
[Thu Sep 14 00:51:55 2017] audit: type=1400 audit(1505368316.092:6): apparmor="STATUS" operation="profile_load" profile="unconfined" name="/usr/lib/NetworkManager/nm-dhcp-client.action" pid=1347 comm="apparmor_parser"
[Thu Sep 14 00:51:55 2017] audit: type=1400 audit(1505368316.092:7): apparmor="STATUS" operation="profile_load" profile="unconfined" name="/usr/lib/NetworkManager/nm-dhcp-helper" pid=1347 comm="apparmor_parser"
[Thu Sep 14 00:51:55 2017] audit: type=1400 audit(1505368316.092:8): apparmor="STATUS" operation="profile_load" profile="unconfined" name="/usr/lib/connman/scripts/dhclient-script" pid=1347 comm="apparmor_parser"
[Thu Sep 14 00:51:55 2017] audit: type=1400 audit(1505368316.092:9): apparmor="STATUS" operation="profile_load" profile="unconfined" name="/usr/sbin/tcpdump" pid=1376 comm="apparmor_parser"
[Thu Sep 14 00:51:55 2017] audit: type=1400 audit(1505368316.092:10): apparmor="STATUS" operation="profile_load" profile="unconfined" name="/usr/bin/lxc-start" pid=1348 comm="apparmor_parser"

Thanks,
Harish S

[Christian Ehrhardt  (paelzer)]

Hi,
in this cae guest message are not important.
Also I already said that it is non fatal - so yes it works, still it is wrong.

Either fix or remove the code whatever is more appropriate.
Please check the code that I referred to in qemu.

hw/ppc/spapr_pci.c:775
 774 /* Construct and read from host device tree the loc-code */
 775 path = g_strdup_printf("/proc/device-tree%s/ibm,loc-code", buf);
 776 g_free(buf);

I'm rather convinced there should be an extra "/" in there.

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2017-10-30 00:15 EDT-------
I moved out of Ubuntu 17.10 squad and assigning this to Rodrigo after discussion with him.
Rodrigo please re-assign it to the appropriate person if you can't work on it.

And changing status to 'open'.

Thanks!!

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2017-11-07 08:52 EDT-------
(In reply to comment #7)
> Hi,
> in this cae guest message are not important.
> Also I already said that it is non fatal - so yes it works, still it is
> wrong.
>
> Either fix or remove the code whatever is more appropriate.
> Please check the code that I referred to in qemu.
>
> hw/ppc/spapr_pci.c:775
> 774 /* Construct and read from host device tree the loc-code */
> 775 path = g_strdup_printf("/proc/device-tree%s/ibm,loc-code", buf);
> 776 g_free(buf);
>
> I'm rather convinced there should be an extra "/" in there.

(In reply to comment #7)
> Hi,
> in this cae guest message are not important.
> Also I already said that it is non fatal - so yes it works, still it is
> wrong.
>
> Either fix or remove the code whatever is more appropriate.
> Please check the code that I referred to in qemu.
>
> hw/ppc/spapr_pci.c:775
> 774 /* Construct and read from host device tree the loc-code */
> 775 path = g_strdup_printf("/proc/device-tree%s/ibm,loc-code", buf);
> 776 g_free(buf);
>
> I'm rather convinced there should be an extra "/" in there.

Thanks Paelzer, We had submitted the patch based upon your suggestion and awaiting response.
.
Patch which we submitted will be fixing 1. device-tree path string to use /proc/device-tree/%s/ibm,loc-code.

[Christian Ehrhardt  (paelzer)]

thanks, will you also look at the related changes to virt-aa-helper in lbivirt once accepted?
In any way ping me once it got into qemu.

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2017-11-08 03:43 EDT-------
(In reply to comment #12)
> thanks, will you also look at the related changes to virt-aa-helper in
> lbivirt once accepted?
> In any way ping me once it got into qemu.

I will check with libvirt team about related changes in virt-aa-helper.
.
The patch of adding extra '/' in /proc/device-tree%s/ibm,loc-code will not work for the following reason.
.
The machine will have leading '/' in devspec as follows.

garrison2:~$ cat /sys/bus/pci/devices/0009:03:00.0/devspec && echo -e '\n'
/pciex@3fffe40500000/pci@0/pci@0/pci@1/usb-xhci@0

So we will no more need this patch.

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2017-11-13 01:53 EDT-------
(In reply to comment #13)
> (In reply to comment #12)
> > thanks, will you also look at the related changes to virt-aa-helper in
> > lbivirt once accepted?
> > In any way ping me once it got into qemu.
>
> I will check with libvirt team about related changes in virt-aa-helper.
> .
> The patch of adding extra '/' in /proc/device-tree%s/ibm,loc-code will not
> work for the following reason.
> .
> The machine will have leading '/' in devspec as follows.
>
> garrison2:~$ cat /sys/bus/pci/devices/0009:03:00.0/devspec && echo -e '\n'
> /pciex@3fffe40500000/pci@0/pci@0/pci@1/usb-xhci@0
>
> So we will no more need this patch.

Shiva/Madhu,

Can you please suggest what can be done from libvirt side about VF devspec paths ?
We currently don't have implementation for VF devspec path or loc in virt-aa-helper code.
Does it cause any security issues when we add those in virt-aa-helper code ? What are the other alternative way we can fix this ?

[Christian Ehrhardt  (paelzer)]

There are two ways to allow qemu to access something.
1. globally through the abstraction in /etc7apparmor.d/abstractions/libvirt-qemu
   That is for paths ALL qemu/geusts are supposed to use like /dev/kvm
2. per guest files generated based on the XML description in /etc/apparmor.d/libvirt/libvirt-<uuid>.files
   If you need paths like /sys/bus/pci/devices/0009:03:00.0/devspec to be accessible you should consider if you can derive the path from the XML and then let virt-aa-helper write a rule for it so that the guest can do so.

Finally later in the guest lifecycle further rules will be added via the labeling calls in the security code. E.g. if you add a device libvirt calls a set label function and this will add the new ruls (like for hotplug).
For the latter see virAppArmorSecurityDriver in src/security/security_apparmor.c

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2019-01-21 10:45 EDT-------
(In reply to comment #21)
> There are two ways to allow qemu to access something.
> 1. globally through the abstraction in
> /etc7apparmor.d/abstractions/libvirt-qemu
> That is for paths ALL qemu/geusts are supposed to use like /dev/kvm
> 2. per guest files generated based on the XML description in
> /etc/apparmor.d/libvirt/libvirt-<uuid>.files
> If you need paths like /sys/bus/pci/devices/0009:03:00.0/devspec to be
> accessible you should consider if you can derive the path from the XML and
> then let virt-aa-helper write a rule for it so that the guest can do so.
>
> Finally later in the guest lifecycle further rules will be added via the
> labeling calls in the security code. E.g. if you add a device libvirt calls
> a set label function and this will add the new ruls (like for hotplug).
> For the latter see virAppArmorSecurityDriver in
> src/security/security_apparmor.c

Leonardo, please help libvirt development to assess and decide if this can be implemented.

[bugproxy (bugproxy)]

------- Comment From <email address hidden> 2019-04-29 17:34 EDT-------
Both 17.04 and 17.10 reached EOL a while ago. Closing...

[Christian Ehrhardt  (paelzer)]

Due to the fallback code present and working neither IBM (for ppc64) nor anyone else found the time/need to work on this. While clearing old cases let us reflect that properly by marking it as incomplete.

[Launchpad Janitor (janitor)]

[Expired for libvirt (Ubuntu) because there has been no activity for 60 days.]