On Wed, Dec 20, 2017 at 12:58 PM, Christian Ehrhardt <christian.ehrhardt@canonical.com> wrote:
On Tue, Dec 19, 2017 at 5:26 PM, Jamie Strandboge <jamie@canonical.com> wrote:
> On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote:
>> vfio devices are generated on the fly, but the generic base is
>> missing.
>>
>> The base vfio has not much functionality but to provide a custom
>> container by opening this path.
>> See https://www.kernel.org/doc/Documentation/vfio.txt for more.
>>
>> Current access by qemu is "wr":
>> [ 2652.756712] audit: type=1400 audit(1491303691.719:25):
>> apparmor="DENIED" operation="open"
>> profile="libvirt-17a61b87-5132-497c-b928-421ac2ee0c8a"
>> name="/dev/vfio/vfio" pid=8486 comm="qemu-system-x86"
>> requested_mask="wr" denied_mask="wr" fsuid=64055 ouid=0
>>
>> Bug-Ubuntu: https://bugs.launchpad.net/bugs/1678322
>>
>> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
>> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
>> ---
>>  examples/apparmor/libvirt-qemu | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/examples/apparmor/libvirt-qemu
>> b/examples/apparmor/libvirt-qemu
>> index 5d811f9..eb4d58c 100644
>> --- a/examples/apparmor/libvirt-qemu
>> +++ b/examples/apparmor/libvirt-qemu
>> @@ -212,3 +212,6 @@
>>    # silence refusals to open lttng files (see LP: #1432644)
>>    deny /dev/shm/lttng-ust-wait-* r,
>>    deny /run/shm/lttng-ust-wait-* r,
>> +
>> +  # for vfio (LP: #1678322)
>> +  /dev/vfio/vfio rw,
>
> Why not just also add this rule iff there is a vfio-specific device
> rule? Ie, just add this along with the vfio device rule in virt-aa-
> helper instead of giving all VMs access when it isn't needed.

Good suggestion, but I realized that - other than the per-device rules
that I knew - we already do so for the base dev as well [1].
Thanks for making me aware.

One thing that puzzles me still is that I hit this in 2017 on a rather
new libvirt.
Maybe on that case the detection was broken and thereby the rule
missing - I'll go back and take a look again and might come up with a
fix for that logic then.

TL;DR - this is just old garbage, I'm dropping this on my end - yay cleanup !

I finally found why we still need the change, giving a TL;DR here and then submitting a refreshed patch.

If a guest description has no hostdev's defined, then in the current virt-aa-helper code it will not add /dev/vfio/vfio nor any per vfio device rules.
That works for guests that have
a) no hostdevs
b) hostdevs defined in the guest xml
It even works for
c) hostdevs defined in the guest xml, later more hostdevs hotplugged

But there is another case
d) guest has no hostdev in guest xml, later hostdevs get hotplugged

In case (d) virt-aa-helper did not add the rule since at the time there was no need.
And the way libvirt/qemu currently play together on vfio hotplug leads to the deny of /dev/vfio/vfio as any labeling hooks or similar I have seen are too late.

If we allow the rather safe /dev/vfio/vfio which is not giving you any device access anyway (see kernel doc) we are good.
In that case qemu will be allowed to open vfio to get its own vfio space.
Later libvirt will catch all that is needed and a labeling call lets virt-aa-helper add the right vfio device.

So we end up with the proposed safe allowance here in the global profile.
And the actual device only being in the per guest profile.
  /dev/vfio/93

re-submitting the patch in a few minutes ...


 
[1]: https://libvirt.org/git/?p=libvirt.git;a=commit;h=74e86b6b2521881808bb93290bcebcb469ab7820

> --
> Jamie Strandboge             | http://www.canonical.com



--
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd