Apparmor denials caused by virt-aa-helper trying to read zvol devices (/dev/zdX) should be silenced

Bug #1641618 reported by Simon Déziel on 2016-11-14
20
This bug affects 2 people
Affects Status Importance Assigned to Milestone
libvirt (Ubuntu)
Medium
Christian Ehrhardt 
Xenial
Low
Unassigned

Bug Description

When a qemu-kvm guest is using a zvol or a DRBD volume or a NVME partition, Apparmor denial messages are logged due to virt-aa-helper trying to access the volume/device. Those should be silenced as it's already done for Logical Volumes.

[Impact]

 * libvirt driving guests on more recent backing devices floods logs and
   dmesg due to non critical apparmor denials.

 * those can distract from real issues and therefore (as with similar
   cases in the past) should be silenced by explicit denials.

[Test Case]
1) Create a KVM guest
2) Edit the guest's XML profile to reference a zvol|DRBD volume|NVME partition
    <disk type='block' device='disk'>
      <driver name='qemu' type='raw' cache='none'/>
      <source dev='/dev/zvol/data/foo'/>
      <target dev='vda' bus='virtio'/>
    </disk>
3) Start the guest
4) Check dmesg for any Apparmor denials, there should be none with the patch

*Without* the patch, one would see those (or similar) denials:

audit: type=1400 audit(1479809919.223:4083): apparmor="DENIED" operation="open" profile="/usr/lib/libvirt/virt-aa-helper" name="/dev/zd0" pid=16715 comm="virt-aa-helper" requested_mask="r" denied_mask="r" fsuid=0 ouid=0

[Regression Potential]
Adding a couple of explicit denials to the virt-aa-helper profile shouldn't cause no harm because Apparmor already denies those, this is just about silencing this.

[Original description]
Libvirt qemu-kvm guests backed by zvols (ZFS volumes) generate useless noise due to virt-aa-helper trying to read the backing device in the host (/dev/zdX). Other host's devs are already denied in virt-aa-helper's profile:

  # for hostdev
  /sys/devices/ r,
  /sys/devices/** r,
  /sys/bus/usb/devices/ r,
  /sys/bus/usb/devices/** r,
  deny /dev/sd* r,
  deny /dev/dm-* r,
  deny /dev/mapper/ r,
  deny /dev/mapper/* r,

Adding "deny /dev/zd[0-9]* r," would silence Apparmor.

Hi Simon,
those changes (the whole part after "/sys/devices/** r,") never made it upstream and was actually dropped in the merge of libvirt 2.x for Yakkety.
Maybe that also moved and this is "only" needed in Xenial and actually fixed in >=Yakkety already?

And looking back - nobody complained on the yakkety merge that smb did - hrm ..?
Eventually (like some day) we want to get rid of apparmor delta and that was one step.

I also checked History backwards but things are a bit lost since for some time Ubuntu was ahead of Debian before switching to the more usual setup.
I almost couldn't find the past but I realized you know the history of this - as I found bug 912007 from you of 2012.

I checked a Yakkety that I had around which did not have the denies as I outlined before.
So following the old bug content I realized it might need special devices. So to reproduce on Yakkety (what I just had around) I added disks on lvm and nvme to see if I can find it:

    <disk type='file' device='disk'>
      <driver name='qemu' type='raw' cache='none'/>
      <source file='/dev/mapper/testvg-testlv'/>
      <target dev='vdc' bus='virtio'/>
    </disk>
    <disk type='file' device='disk'>
      <driver name='qemu' type='raw' cache='none'/>
      <source file='/dev/nvme0n1'/>
      <target dev='vdd' bus='virtio'/>
    </disk>

The LV was silent - although I failed to see what was done in the profile for it.
The nveme I found and I think that is because it is new and not covered yet, the same way zfs might not be there yet. I need to find the spot that makes the "ok" to the LVM as this is clearly the place to add it on newer versions.

Simon:
- Can you describe the "noise" it makes to you?
- Having the old rules you were on Xenial right?
- Does it match what I found?
- The old bug only has "Per discussion on irc, I'll add a deny rule to usr.lib.libvirt.virt-aa-helper", but I don't really get why it is a deny and not an allow - could you elaborate on that?

Changed in libvirt (Ubuntu):
status: New → Confirmed
importance: Undecided → Medium

To really be worth the "confirmed" I also checked the zfs case.

# create most basic zfs on the LV (because it had free space)
  sudo zpool create -f zfsp1 /dev/mapper/testvg-testlv--forzfs
  sudo zfs create -ps -V 10G zfsp1/zfsvol1
# which gives me
  /dev/zvol/zfsp1/zfsvol1 -> /dev/zd0

Added the matching XML
    <disk type='file' device='disk'>
      <driver name='qemu' type='raw' cache='none'/>
      <source file='/dev/zvol/zfsp1/zfsvol1'/>
      <target dev='vde' bus='virtio'/>
    </disk>

# Got just like you the zfs deny:
[2165239.463108] audit: type=1400 audit(1479809919.223:4083): apparmor="DENIED" operation="open" profile="/usr/lib/libvirt/virt-aa-helper" name="/dev/zd0" pid=16715 comm="virt-aa-helper" requested_mask="r" denied_mask="r" fsuid=0 ouid=0

Seeing that I expect any kind of special /dev might be affected. Thinking of special architectures like /dev/dasd on s390x.
I'd need to find where in the current profiles e.g. LVM is covered to add it there.
Waiting for Simon to answer the questions I outlined before.

Hello Christian,

On 2016-11-22 05:06 AM, ChristianEhrhardt wrote:
> those changes (the whole part after "/sys/devices/** r,") never made it upstream and was actually dropped in the merge of libvirt 2.x for Yakkety.
> Maybe that also moved and this is "only" needed in Xenial and actually fixed in >=Yakkety already?

Good point. I only use LTS releases so I don't know the answer to that.

> And looking back - nobody complained on the yakkety merge that smb did - hrm ..?
> Eventually (like some day) we want to get rid of apparmor delta and that was one step.

Yes and that's something I'd like to work on when time will permit.

> Simon:
> - Can you describe the "noise" it makes to you?

[Tue Oct 18 12:25:29 2016] audit: type=1400 audit(1476807929.293:265):
apparmor="DENIED" operation="open"
profile="/usr/lib/libvirt/virt-aa-helper" name="/dev/zd128" pid=5432
comm="virt-aa-helper" requested_mask="r" denied_mask="r" fsuid=0 ouid=0

> - Having the old rules you were on Xenial right?

Yes, Xenial here.

> - Does it match what I found?

Yes

> - The old bug only has "Per discussion on irc, I'll add a deny rule to usr.lib.libvirt.virt-aa-helper", but I don't really get why it is a deny and not an allow - could you elaborate on that?

I don't remember that IRC discussion and have no log of it either, sorry.

virt-aa-helper is already denied from accessing those backing files
Preventing virt-aa-helper from accessing the backing file doesn't cause
any problem as far as I could see. The guest's profile still get the
right "/dev/zdXX rw," line added to it's libvirt-UUID.files file. Same
goes for LVs where a "/dev/dm-X rw," is added.

I don't know why virt-aa-helper tries to read those backing files.
Logically, I would expect such read access for backing files that are
stack-able like QCOW2. Reading the file referenced in the .xml would let
virt-aa-helper learn about any other backing file that needs to be
exposed to the VM. Maybe that is why read access to all .qcow2 is
granted? That's just a wild guess though.

Thanks for your clarifications Simon.

I found where it is managed today. While it is not upstream it comes in in various ways.

This all is abit complex as smb started a task to rely on upstream profiles, to one day drop much of the delta. But since it is WIP it is currently in a complex states.

So atm we get the profiles as this:
1. take upstream profiles
2. Apply Debian delta to upstream profiles
3. moved modified upstream profiles to .in files
4. initial add Ubuntu delta to .in
5. later fixes onto Ubuntu delta to .in
Finally that is generated - the reasons to this is in the different apparmor features per versions between Debian and Ubuntu. SMB and I plan to discuss and agree on a plan of action when we meet in a few weeks.

Until this is sorted out and synced, we continue to fix at stage #5 for now.

The way e.g. dm-* isn't an issue today is by:
1. Added by Debian Allow-access-to-libnl-3-config-files.patch (Step #2)
2. Moved by Ubuntu ubuntu/0001-apparmor-add-feature-parsing.patch (Step #3)
3. slightly modified in ubuntu/0002-apparmor-apply-ubuntu-delta.patch (Step #5)

Some of the History on this is in:
- https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=786650
- https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=796088
This covers the lost IRC discussion I asked about before.

Until the mentioned cleanup/sync happened the way for now is to add more to Step #5.
That would be:
- bring back /dev/vd* (was in Debian)
- add /dev/zd[0-9]*
- add /dev/nvme*

I wonder if there would be a an abstraction for disk devices that covers that and doesn't need an update every time a new disk device occurs.

Changed in libvirt (Ubuntu):
status: Confirmed → Triaged

Hi Simon,
I made a ppa for early verification available.
It works for my case, but a second test would be very kind - do you think you could take a look at that on zesty?
=> https://launchpad.net/~paelzer/+archive/ubuntu/libvirt-bug-1546674-1615550

P.S. I've used one ppa for this and bug 1546674 trying to save you some work on those checks

Changed in libvirt (Ubuntu):
assignee: nobody → ChristianEhrhardt (paelzer)

The new dup is the same issue for:
 /dev/drbd** r,

I already added on my own the s390x device type
 /dev/dasd* r,

Simon Déziel (sdeziel) wrote :

On 2016-11-28 09:53 AM, ChristianEhrhardt wrote:
> The new dup is the same issue for:
> /dev/drbd** r,

DRBD devices are always followed by digit(s) so you could use this if
you haven't done the change already:

  /dev/drbd[0-9]* r,

Philipp Marek (philipp-marek) wrote :

> DRBD devices are always followed by digit(s) so you could use this if
> you haven't done the change already:
>
> /dev/drbd[0-9]* r,
If they are referenced by name, instead of by minor, a DRBD path might look
like this, too:

  /dev/drbd/by-disk/<volume-group>/<logical-volume>
  /dev/drbd/by-res/<resource-name>/<volume-number>

Examples:

  /dev/drbd/by-res/CV_ac2cca49-8de1-45c4-8fc8-15c22f444b37/0
  /dev/drbd/by-disk/drbddata/CV_ac2cca49-8de1-45c4-8fc8-15c22f444b37_00

Simon Déziel (sdeziel) wrote :

On 2016-11-28 10:29 AM, Philipp Marek wrote:
>> DRBD devices are always followed by digit(s) so you could use this if
>> you haven't done the change already:
>>
>> /dev/drbd[0-9]* r,
> If they are referenced by name, instead of by minor, a DRBD path might look
> like this, too:
>
> /dev/drbd/by-disk/<volume-group>/<logical-volume>
> /dev/drbd/by-res/<resource-name>/<volume-number>

Interesting, are those files actual devices or symlinks pointing to
somewhere else? I'm asking because Apparmor rules only apply to the
final destination of symlinks.

Simon Déziel (sdeziel) wrote :

Christian, I was able to test your PPA build on Zesty. FYI, I test with a guest having the 3 storage devices:

    <disk type='file' device='disk'>
      <driver name='qemu' type='raw' cache='none'/>
      <source file='/dev/nvme0n1p6'/>
      <target dev='vda' bus='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
    </disk>
    <disk type='file' device='disk'>
      <driver name='qemu' type='raw' cache='none'/>
      <source file='/dev/zvol/internal/apt'/>
      <target dev='vdb' bus='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
    </disk>
    <disk type='file' device='disk'>
      <driver name='qemu' type='raw' cache='none'/>
      <source file='/dev/drbd/by-disk/nvme0n1p6'/>
      <target dev='vdc' bus='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
    </disk>

The only Apparmor denial I got was the read access on /dev/drbd0 which was caused by the symlink /dev/drbd/by-disk/nvme0n1p6 pointing there:

 # readlink -e /dev/drbd/by-disk/nvme0n1p6
 /dev/drbd0

So once you add the "/dev/drbd[0-9]* r," rule, this bug should be fully addressed. FYI, I added the drbd rule locally and it tested fine. Thanks again for all your help.

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package libvirt - 2.1.0-1ubuntu14

---------------
libvirt (2.1.0-1ubuntu14) zesty; urgency=medium

  * d/p/u/apparmor-fix-name-resolution.patch rework the fix to base
    on the apparmor nameservice abstraction to be future proof (LP: #1546674).
  * d/p/ubuntu/apparmor-fix-new-devicetypes.patch add new block device types to
    virt-aa-helpers profile (LP: #1641618)
  * d/p/u/apparmor-fix-other-seclabels.patch refresh to the now upstream
    accepted solution (LP: #1633207).

 -- Christian Ehrhardt <email address hidden> Thu, 24 Nov 2016 08:06:38 +0100

Changed in libvirt (Ubuntu):
status: Triaged → Fix Released
Simon Déziel (sdeziel) wrote :

Hi Christian, I was hoping for this to be SRU'ed to Xenial, when you have the time. I started filling the SRU justification but would appreciate your input for the regression potential section. Thanks.

description: updated

Thank you Simon,
since this is "only" about silencing a warning it wasn't worth an SRU in my personal opinion.
I can consider to wrap it up in another SRU thou and let the SRU Team decide.
The "impact" of warnigns might be too low, but OTOH the regression potential is as well and Xenial will be with us for quite a while still :-).

I keep this task unread and would try to bundle with another SRU eventually.
But sadly ATM I can't get to bisect the other case which is required to drive it forward (FYI bug 1620407).

Changed in libvirt (Ubuntu Xenial):
status: New → Triaged
importance: Undecided → Low

Thanks once again Simon for the SRU Template - I was adding the "impact" section as I started to work on verifying the SRU for this together with a bunch of other changes.

description: updated

On ppa identical to SRU upload:
- Automated Tests passed (to check for regressions)
- Manual Test passed (to check for regressions)
- Explicit tests on this case worked

Uploaded to SRU queue now for SRU Team to check.

Hello Simon, or anyone else affected,

Accepted libvirt into xenial-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/libvirt/1.3.1-1ubuntu10.8 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 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 to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. 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!

Changed in libvirt (Ubuntu Xenial):
status: Triaged → Fix Committed
tags: added: verification-needed
Simon Déziel (sdeziel) wrote :

Thanks Christian it works well.

tags: added: verification-done
removed: verification-needed
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package libvirt - 1.3.1-1ubuntu10.8

---------------
libvirt (1.3.1-1ubuntu10.8) xenial; urgency=medium

  * fix virsh nodecpumap output (LP: #1659769)
  * fix using type ethernet interfaces with user scripts (LP: #1620407)
  * add new block device types to virt-aa-helpers profile (LP: #1641618)

 -- Christian Ehrhardt <email address hidden> Mon, 06 Feb 2017 14:30:46 +0100

Changed in libvirt (Ubuntu Xenial):
status: Fix Committed → Fix Released

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.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.