virt-aa-helper does not whitelist actual <source dev='...'> paths for domain <disk type='volume'>

[Tero Marttila]

Release: 14.04
Package: libvirt-bin
Version: 1.2.2-0ubuntu13.1.1

For a normal block-based LVM disk definition

    <disk type='block' device='disk'>
      <driver name='qemu' type='raw'/>
      <source dev='/dev/host-vg/guest.img'/>
      <target dev='vda' bus='virtio'/>
    </disk>

virt-aa-helper will generate "/dev/dm-X rw" rules in the /etc/apparmor.d/libvirt/libvirt-*.files

  "/dev/dm-10" rw,

However, using a storage pool:

<pool type='logical'>
  <name>lvm</name>
  <source>
    <name>host-vg</name>
  </source>
  <target>
    <path>/dev/host-vg</path>
  </target>
</pool>

to create the volume:

<volume>
    <name>guest.img</name>
    <capacity>....</capacity>
</volume>

and attempting to use the equivalent:

    <disk type='volume' device='disk'>
      <driver name='qemu' type='raw'/>
      <source pool='lvm' volume='guest.img'/>
      <target dev='vda' bus='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
    </disk>

Results in the following with `virsh start guest`

error: Failed to start domain guest
error: internal error: process exited while connecting to monitor: qemu-system-x86_64: -drive file=/dev/host-vg/guest.img,if=none,id=drive-virtio-disk0,format=raw: could not open disk image /dev/host-vg/guest.img: Could not open '/dev/host-vg/guest.img': Permission denied

And:

[164096.938448] type=1400 audit(1405596016.664:100): apparmor="DENIED" operation="open" profile="libvirt-fdd84027-cb8e-42d5-bca1-a662871d97bb" name="/dev/dm-10" pid=26835 comm="qemu-system-x86" requested_mask="r" denied_mask="r" fsuid=109 ouid=109
[164096.938472] type=1400 audit(1405596016.664:101): apparmor="DENIED" operation="open" profile="libvirt-fdd84027-cb8e-42d5-bca1-a662871d97bb" name="/dev/dm-10" pid=26835 comm="qemu-system-x86" requested_mask="r" denied_mask="r" fsuid=109 ouid=109
[164096.938515] type=1400 audit(1405596016.664:102): apparmor="DENIED" operation="open" profile="libvirt-fdd84027-cb8e-42d5-bca1-a662871d97bb" name="/dev/dm-10" pid=26835 comm="qemu-system-x86" requested_mask="rw" denied_mask="rw" fsuid=109 ouid=109

The apparmor libvirt-*.files does not contain any /dev/dm-* rules.

I'm not familar enough with the virAppArmorSecurityDriver code to know if the load_profile() call to virDomainDefFormat() will give the persistent or live xml config, but when testing with virt-aa-helper manually, feeding it the inactive config (i.e. `virsh dumpxml` while the domain is stopped) will cause get_files() to call virDomainDiskDefForeachPath() with a virDomainDiskDefPtr of type=VIR_DOMAIN_DISK_TYPE_VOLUME and src=NULL, so it never iters over the disk. I suspect that virt-aa-helper should instead be fed the active config, i.e. one where the <disk type='volume'> has been fed through
qemuTranslateDiskSourcePool() to resolve it into the actual <disk type='block'><source dev='...' /></disk>?

[Serge Hallyn (serge-hallyn)]

Thanks - reproduced that here on utopic. Since there is an obvious
workaround, I'll mark this medium, not high, priority.

 status: triaged
 importance: medium

[Serge Hallyn (serge-hallyn)]

This patch appears to fix the issue for me. I'll propose it (cleaned up) on the mailing list.

[Tero Marttila (terom-u)]

A type=VIR_STORAGE_TYPE_VOLUME disk is not necessarily a virStoragePoolDefPtr.type=VIR_STORAGE_POOL_LOGICAL, and may or may not involve a /dev/... path; the logic for translating them into src paths seems to be somewhere like qemuTranslateDiskSourcePool() via virStorageVolGetPath(), and seems to also depend on the mode?

AFAICT the significant difference is between the persistent config and the "live" running config for a domain; the later contains the actual translated <source dev='...' /> paths directly. Ideally virt-aa-helper would be based off of the translated config?

[Serge Hallyn (serge-hallyn)]

Quoting Tero Marttila (<email address hidden>):
> A type=VIR_STORAGE_TYPE_VOLUME disk is not necessarily a
> virStoragePoolDefPtr.type=VIR_STORAGE_POOL_LOGICAL, and may or may not
> involve a /dev/... path; the logic for translating them into src paths
> seems to be somewhere like qemuTranslateDiskSourcePool() via
> virStorageVolGetPath(), and seems to also depend on the mode?

I did see that code and was worried that might be the case.

> AFAICT the significant difference is between the persistent config and
> the "live" running config for a domain; the later contains the actual
> translated <source dev='...' /> paths directly. Ideally virt-aa-helper
> would be based off of the translated config?

Doesn't seem to be, though there may be some way of doing that that I'm
not aware of. Current virt-aa-helper just seems to only read the domain's
xml and works purely based on that.

[Darragh Bailey (dbailey-k)]

Could virt-aa-helper simply ask libvirt for what the pool path would be in the case of the XML definition specifying a volume type? Then join the volume name and pool path given and treat that as though it was given a full file path?

Should just mean parsing some more XML to extract an additional piece of information in the case of using a volume type.

[Christian Ehrhardt  (paelzer)]

It has access to the libvirt view of datastructures already - it uses the same function libvirt does to parse the xml and then has the structures libvirt would use.

[Christian Ehrhardt  (paelzer)]

I didn't retry if this is any better these days, but I'm collecting a bunch of virt-aa-helper bugs together so that they reach a critical mass some day to spend the effort of a implementation and upstreaming session.

Tagging this accordingly now.

If one could spend the time to recheck this on something new like zesty that would be great.

[Christian Ehrhardt  (paelzer)]

Interestingly enough working on another issue I realized that these two are actually the same.
And I even ended up at the same conclusions e.g. it would need the translate call.

I've worked a bit further on that in bug 1677398 and outlined the problems libvirt currently has essentially by working off the static xml.
Asking libvirt as suggested by Darragh in c#5 could be an option - I have some POC shell code that does so with dumpxml + parsing.
But doing so from virt-aa-helper has the same constraints that my current approach with getting a virtConnectPtr has (Details in the other bug).

Since the fix is "support pools" and neither dm nor xfs specific - and that I worked on it on the other bug already I'll dup this bug onto the other and merge the case.