usb hostdev passthrough generates the wrong apparmor rules

[Christian Ehrhardt ]

[Impact]

 * USB Host devices fail to add statically

 * The reason is that libvirt has not yet initialized usb devices

 * Fix by back-porting small upstream change

[Test Case]

 * Create a VM Guest (e.g. via uvtool)
 * Shut down the guest
 * virsh edit <guestname>
 * Add a usb hostdev from your System (check lsusb for IDs)
 * See the original description below for XML examples
 * Starting the guest will create a wrong rule
     "/dev/bus/usb/000/000" rw,
   And due to that fails to start.

[Regression Potential]

 * The change is small and only makes certain values available to libvirt

 * The only thing I could think of regressing is if that
   virHostdevFindUSBDevice would crash on some systems, but then it would
   fail later on in the lifecycle even without the patch - so we should be
   safe IMHO.

[Other Info]

 * I waited to be accepted upstream to be more confident which is
   partially why this took so long but provides some extra confidence.

---

Libvirt-aa-helper seems to have a bug when adding usb passthrough devices statically.

On hotplug with:
$ cat sandisk-usb.xml
<hostdev mode='subsystem' type='usb' managed='yes'>
    <source>
        <!--
          idVendor 0x0781 SanDisk Corp.
          idProduct 0x5580 SDCZ80 Flash Drive
        -->
        <vendor id='0x0781'/>
        <product id='0x5580'/>
    </source>
</hostdev>

$ virsh attach-device z-test1 sandisk-usb.xml

It generates correctly:
"/dev/bus/usb/003/003" rw,

But if adding the same XML part to the guest xml itself it generates:
"/dev/bus/usb/000/000" rw,

And as a follow on issue the guest start fails with:
libusb: error [_get_usbfs_fd] libusb couldn't open USB device /dev/bus/usb/003/003: Permission denied
Due to:
apparmor="DENIED" operation="open" profile="libvirt-adc578cb-905f-41fc-9be2-9fb81f6a6073" name="/dev/bus/usb/003/003" pid=22879 comm="qemu-system-x86" requested_mask="wr" denied_mask="wr" fsuid=123 ouid=123

[Christian Ehrhardt  (paelzer)]

Actually if possible this should become so good that we can drop the /dev/usb addition to apparmor that we have.

[Christian Ehrhardt  (paelzer)]

I don't seem to find time shortly, so I drop server-next tag.
But I ensured it is tracked to remind me and make me feel bad :-/

[Christian Ehrhardt  (paelzer)]

Documenting the workarounds we have until this is solved properly are as following:
- use virsh attach (hot-add) instead of adding it to the guest right away
- If you happen to use the same USB port every time the IDs won't change and you can check what was blocked in dmesg and add just that to the guests (or if you need for more guests libvirt-qemu profile) apparmor profile
- Open up USB more or less in general in libvirt-qemu profile (least recommended for security reasons)

It is yet unclear why:
1. it depends on hot-add vs initial device add (virt-aa-helper makes no difference on these two)
2. it works on some devices but not on others

Please add in every case of this here that you have found with:
1. guest xml
2. lsusb
3. dmesg showing the blocker
(Mine is the initial description with a flash drive)

[Christian Ehrhardt  (paelzer)]

Apparmor rule is:
  "/dev/bus/usb/000/000" rw,

But then while virt-aa-helper is running it really is 0/0
p ctl->def->hostdevs[0]->source.subsys.u.usb
$12 = {autoAddress = false, bus = 0, device = 0, vendor = 1921, product = 21888}

Stack is like this:
  virDomainHostdevSubsysUSBDefParseXML at conf/domain_conf.c:6521
  virDomainHostdevDefParseXMLSubsys conf/domain_conf.c:7213
  virDomainHostdevDefParseXML conf/domain_conf.c:14287
  virDomainDefParseXML conf/domain_conf.c:18968
  virDomainDefParseNode conf/domain_conf.c:19460
  virDomainDefParse conf/domain_conf.c:19404
  virDomainDefParseString conf/domain_conf.c:19420
  get_definition security/virt-aa-helper.c:710

So if the initial XML parsing really only does XML parsing.
Which code fills in the bus/device details.
It does so later on when running before calling virt-aa-helper on the live case.
I confirmed once more that in the live case it correctly creates:
  "/dev/bus/usb/002/011" rw,

At that point (live) it is fully detected, dumpxml confirms:
      <source>
        <vendor id='0x0781'/>
        <product id='0x5580'/>
        <address bus='3' device='2'/>
      </source>

I checked if this might have been a regression of some sort we could find with bisecting.
But even back to trusty with libvirt 1.2.2 this behaves the same way.

There is a set of functions that look good:
virhostdev.c virHostdevFindUSBDevice
virsubusb.c virUSBDeviceFindByVendor, virUSBDeviceFind

But these are only called from virHostdevPrepareUSBDevices which is not on the "parsing" path, but only the actual attaching.

Signature from virhostdev.h:
 virHostdevFindUSBDevice(virDomainHostdevDefPtr hostdev,
                         bool mandatory,
                         virUSBDevicePtr *usb)

To derive the mandatory flag from virt-aa-helper seems to be like guessing at best.
So I set it to false which makes it info-print but not break if it can't find the device.
It would depend on being optional startup policy (that we could check), but we have no way to know if we are coldbooting.
I can decide to make it too strict

Following the thoughts of:
commit e658daeb58ec88deec5dd60fd3279f4feb4eac5b
Author: Jiri Denemark <email address hidden>
Date: Tue Oct 2 15:14:02 2012 +0200

    conf: Add support for startupPolicy for USB devices

    USB devices can disappear without OS being mad about it, which makes
    them ideal for startupPolicy. With this attribute, USB devices can be
    configured to be mandatory (the default), requisite (will disappear
    during migration if they cannot be found), or completely optional.

TL;DR - if we can't find the device we can't add it to the rules.
We do not have to break the world f they are missing but it is "fair" to add something about the error to the log and skip adding a rule for them.
We can easily check if they are explicitly optional and set required false in those cases, but still the result would be a broken apparmor rule (so nothing useful in any way).
Therefore not worth to add logic, instead static "mandatory" and if not a good rc not adding a rule is probably the best.

[Christian Ehrhardt  (paelzer)]

That was a lot of text, the TL;DR is that I have a fix to upstream.
But I'll work on some other virt-aa-helper changes first to submit them together.

[Christian Ehrhardt  (paelzer)]

Related changes upstream now, will be picked no next merge.
Likely consider picking in advance as soon as BB opens up.

[Christian Ehrhardt  (paelzer)]

Prepped the SRU Template for Artful as it is released now.
Also passed (the now fully running, due to the fixes) regression tests - so ready for SRU review.

Note: Bionic Beaver is not yet around, so uploading to Artful with a normal version increment should still be the right thing to do - if there was a race with BB, please let me know so that I upload it there asap to fix it in Artful.

[Robie Basak (racb)]

SRU +1 for what is currently in the queue.

[Andy Whitcroft (apw)]

Hello ChristianEhrhardt, or anyone else affected,

Accepted libvirt into artful-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/libvirt/3.6.0-1ubuntu6 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed.Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-artful to verification-done-artful. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-artful. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

[Christian Ehrhardt  (paelzer)]

Use a guest XML that already combines the USB Hostdev in it.

# cat testguest.xml
<domain type='kvm'>
    <name>testguest</name>
    <uuid>deadbeef-dead-beef-dead-beefdeadbeef</uuid>
    <memory unit='KiB'>1024</memory>
    <vcpu placement='static'>1</vcpu>
    <os>
        <type arch='x86_64' machine='pc-i440fx-zesty'>hvm</type>
        <boot dev='hd'/>
    </os>
    <features>
        <acpi/>
        <apic/>
        <pae/>
    </features>
    <devices>
        <emulator>/usr/bin/kvm-spice</emulator>
        <disk type='file' device='disk'>
            <driver name='qemu'/>
            <source file='/var/lib/libvirt/images/A.img'/>
            <target dev='vda'/>
        </disk>
    <hostdev mode='subsystem' type='usb' managed='yes'>
        <source>
            <vendor id='0x046d'/>
            <product id='0x0825'/>
        </source>
    </hostdev>
    </devices>
    <seclabel type='dynamic' model='apparmor' relabel='yes'/>
</domain>

root@ubuntu:~# virsh define testguest.xml
Domain testguest defined from testguest.xml

root@ubuntu:~# virsh start testguest
error: Failed to start domain testguest
error: internal error: process exited while connecting to monitor: warning: host doesn't support requested feature: CPUID.80000001H:ECX.svm [bit 2]
2017-10-25T10:31:34.412297Z qemu-system-x86_64: -device usb-host,hostbus=2,hostaddr=10,id=hostdev0,bus=usb.0,port=1: failed to find host usb device 2:10

Along that there are Apparmor denials:
[ 2260.676741] audit: type=1400 audit(1508927494.409:129): apparmor="DENIED" operation="open" profile="libvirt-deadbeef-dead-beef-dead-beefdeadbeef" name="/run/udev/data/c189:133" pid=9571 comm="qemu-system-x86" requested_mask="r" denied_mask="r" fsuid=64055 ouid=0
[ 2260.677046] audit: type=1400 audit(1508927494.409:132): apparmor="DENIED" operation="open" profile="libvirt-deadbeef-dead-beef-dead-beefdeadbeef" name="/run/udev/data/c189:256" pid=9571 comm="qemu-system-x86" requested_mask="r" denied_mask="r" fsuid=64055 ouid=0
[ 2260.677424] audit: type=1400 audit(1508927494.410:135): apparmor="DENIED" operation="open" profile="libvirt-deadbeef-dead-beef-dead-beefdeadbeef" name="/run/udev/data/c189:129" pid=9571 comm="qemu-system-x86" requested_mask="r" denied_mask="r" fsuid=64055 ouid=0
[ 2260.677733] audit: type=1400 audit(1508927494.410:137): apparmor="DENIED" operation="open" profile="libvirt-deadbeef-dead-beef-dead-beefdeadbeef" name="/run/udev/data/c189:0" pid=9571 comm="qemu-system-x86" requested_mask="r" denied_mask="r" fsuid=64055 ouid=0

The failed Profile for the guest has the wrong rule:
root@ubuntu:~# grep usb /etc/apparmor.d/libvirt/libvirt-deadbeef-dead-beef-dead-beefdeadbeef.files
  "/dev/bus/usb/000/000" rw,

# After upgrading to proposed no more errors while doing that.

root@ubuntu:~# apt install libvirt-daemon-system=3.6.0-1ubuntu6 libvirt-clients=3.6.0-1ubuntu6 libvirt-daemon=3.6.0-1ubuntu6 libvirt0=3.6.0-1ubuntu6
Reading package lists... Done
Building dependency tree
Reading state information... Done
Suggested packages:
  numad radvd auditd systemtap nfs-common zfsutils pm-utils
The following packages will be upgraded:
  libvirt-clients libvirt-daemon libvirt-daemon-system libvirt0
...

[Launchpad Janitor (janitor)]

This bug was fixed in the package libvirt - 3.6.0-1ubuntu6

---------------
libvirt (3.6.0-1ubuntu6) artful; urgency=medium

  * d/p/ubuntu-aa/0037-virt-aa-helper...: grant locking permission on append
    files (LP: #1726804)
  * d/p/ubuntu-aa/0038-virt-aa-helper-fix-paths-for-usb-hostdevs.patch:
    fix path generation for USB host devices (LP: #1552241)
  * d/p/ubuntu-aa/0039-virt-aa-helper-fix-libusb-access-to-udev-usb-data.patch:
    generate valid rules on usb passthrough (LP: #1686324)

 -- Christian Ehrhardt <email address hidden> Tue, 24 Oct 2017 14:30:34 +0200

[Chris Halse Rogers (raof)]

The verification of the Stable Release Update for libvirt has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

[Launchpad Janitor (janitor)]

This bug was fixed in the package libvirt - 3.6.0-1ubuntu6

---------------
libvirt (3.6.0-1ubuntu6) artful; urgency=medium

  * d/p/ubuntu-aa/0037-virt-aa-helper...: grant locking permission on append
    files (LP: #1726804)
  * d/p/ubuntu-aa/0038-virt-aa-helper-fix-paths-for-usb-hostdevs.patch:
    fix path generation for USB host devices (LP: #1552241)
  * d/p/ubuntu-aa/0039-virt-aa-helper-fix-libusb-access-to-udev-usb-data.patch:
    generate valid rules on usb passthrough (LP: #1686324)

 -- Christian Ehrhardt <email address hidden> Tue, 24 Oct 2017 14:30:34 +0200

[Christian Ehrhardt  (paelzer)]

While clearing old bugs I found this one and priority for Xenila/Zesty backports never was important to anyone. Nowadays those are on ESM support and since there is a workaround (rule overrides) and this isn't a security issue I'll set Won't Fix for those.