On Wed, 20 Mar 2019, Christian Ehrhardt wrote: > On Wed, Mar 20, 2019 at 8:45 AM Christian Ehrhardt > <christian.ehrhardt@xxxxxxxxxxxxx> wrote: > > > > On Mon, Mar 4, 2019 at 11:42 AM Christian Ehrhardt > > <christian.ehrhardt@xxxxxxxxxxxxx> wrote: > > > > > > On Fri, Mar 1, 2019 at 5:56 PM Jamie Strandboge <jamie@xxxxxxxxxxxxx> wrote: > > > > > > > > On Mon, 18 Feb 2019, Christian Ehrhardt wrote: > > > > > > > > > So far we were detecting at guest start if any devices needed vhost net > > > > > and only if that was true added a rule for /dev/vhost-net. > > > > > > > > > > It turns out that it is an absolutely valid case to start a guest > > > > > without any vhost-net networking but later on wanting to hotplug such a > > > > > device which then would be denied by apparmor. > > > > > > > > > > Unfortunately there also is no security labeling callback involved other > > > > > than the one to /dev/net/tun. But on the other hand vhost-net is no more > > > > > new and considered rather safe. Therefore drop the old detection and > > > > > just add it as a static rule. > > > > > > > > > > Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1815910 > > > > > > > > > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@xxxxxxxxxxxxx> > > > > > --- > > > > > src/security/apparmor/libvirt-qemu | 1 + > > > > > src/security/virt-aa-helper.c | 17 +---------------- > > > > > 2 files changed, 2 insertions(+), 16 deletions(-) > > > > > > > > > > diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu > > > > > index eaa5167525..a71f34c175 100644 > > > > > --- a/src/security/apparmor/libvirt-qemu > > > > > +++ b/src/security/apparmor/libvirt-qemu > > > > > @@ -21,6 +21,7 @@ > > > > > signal (receive) peer=/usr/sbin/libvirtd, > > > > > > > > > > /dev/net/tun rw, > > > > > + /dev/vhost-net rw, > > > > > /dev/kvm rw, > > > > > /dev/ptmx rw, > > > > > /dev/kqemu rw, > > > > > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c > > > > > index 8e22e9978a..ebc4feac77 100644 > > > > > --- a/src/security/virt-aa-helper.c > > > > > +++ b/src/security/virt-aa-helper.c > > > > > @@ -937,7 +937,7 @@ get_files(vahControl * ctl) > > > > > size_t i; > > > > > char *uuid; > > > > > char uuidstr[VIR_UUID_STRING_BUFLEN]; > > > > > - bool needsVfio = false, needsvhost = false; > > > > > + bool needsVfio = false; > > > > > > > > > > /* verify uuid is same as what we were given on the command line */ > > > > > virUUIDFormat(ctl->def->uuid, uuidstr); > > > > > @@ -1248,21 +1248,6 @@ get_files(vahControl * ctl) > > > > > } > > > > > } > > > > > > > > > > - if (ctl->def->virtType == VIR_DOMAIN_VIRT_KVM) { > > > > > - for (i = 0; i < ctl->def->nnets; i++) { > > > > > - virDomainNetDefPtr net = ctl->def->nets[i]; > > > > > - if (net && net->model) { > > > > > - if (net->driver.virtio.name == VIR_DOMAIN_NET_BACKEND_TYPE_QEMU) > > > > > - continue; > > > > > - if (!virDomainNetIsVirtioModel(net)) > > > > > - continue; > > > > > - } > > > > > - needsvhost = true; > > > > > - } > > > > > - } > > > > > - if (needsvhost) > > > > > - virBufferAddLit(&buf, " \"/dev/vhost-net\" rw,\n"); > > > > > - > > > > > if (needsVfio) { > > > > > virBufferAddLit(&buf, " \"/dev/vfio/vfio\" rw,\n"); > > > > > virBufferAddLit(&buf, " \"/dev/vfio/[0-9]*\" rw,\n"); > > > > > > > > A few things I noticed: > > > > > > > > - if I launch a vm with a virtio net device, then I can detach/attach as much > > > > as I want > > > > - if I launch a vm with one virtio net device, then detach it, /dev/vhost-net > > > > is not removed from the profile. > > > > - if launch a vm with one virtio net device, then I detach it and I shutdown > > > > the vm with no attached network devices, on the next vm start, the device is > > > > present and the profile still has the access. IIRC, this is by design > > > > - if I remove the net device from the vm definition then libvirtd updates > > > > profile to not allow the access. When I start the vm I cannot attach a virtio > > > > net device because the access is denied (this bug) > > > > - /dev/vhost-net is root:root 600 and vms typically run as non-root > > > > (libvirt-qemu:kvm here) and therefore a qemu process would not be able to > > > > open the device on its own. The only reason why qemu can is because libvirt > > > > passes an fd to it, and it is when qemu writes to it that the access is > > > > denied > > > > > > > > To me, it seems the real bug is that hotplug attach/detach is not updating the > > > > profile accordingly (attach adds it if it isn't there and detach removes it if > > > > no more net devices are present). Ideally, this would be the fix for this bug > > > > (we certainly support other hotplug/hotunplug events, like disks). > > > > > > Yes ideally it would, but I that would be a major change how hotplug > > > in general is handled by virt-aa-helper and I haven't had something > > > like that around easily. > > > I think we even talked about that quite a while a go when we also > > > talked about the inability to get more insight into e.g. pools from > > > the POV of virt-aa-helper. > > > It is on my "long term find no time for it list" :-/ > > > > > > > Considering > > > > that the device expands the attack surface to this part of the kernel and > > > > because the root:root 600 permissions show that non-root is not meant to access > > > > the device, this is most correct. > > > > > > > > That said, libvirt itself is providing protection here because it is mediating > > > > the access to /dev/vhost-net for the VMs since the VMs can't open the device > > > > themselves and they must rely on libvirt to pass in the fd. Because of this, > > > > adding the access to the profile is not providing additional protections beyond > > > > DAC *for this common use case and configuration*. Conditionally adding the > > > > access would provide benefit when 'user = "root"' is set in qemu.conf or the > > > > device itself has different permissions that allow the access (eg, 660 > > > > root:kvm). > > > > > > > > I maintain a preference for updating the profile on hotplug events. I'm willing > > > > to concede that adding the access for now until the correct solution is > > > > implemented would be acceptable, but I'd better like to understand how common > > > > it is to start a VM with no network devices and then hotplug one. > > > > > > I agree that if the use case is unreasonable we might declare this as > > > "work-as-intended" and the few people affected would then be supposed > > > to add a local override opting-in to open /dev/vhost up to all guests. > > > I'll ping on the bug for affected people to explain why/how they hit > > > the case, that might shed some light on it to allow us making the > > > decision if it is reasonable enough to concede the static rule as an > > > interim solution until one day it would be detected correctly on every > > > hotplug (and unplug) event. > > > > Hi, > > there was no further comment here so far. > > On the bug the following is what I got: > > (Note that it was reported to be triggered by Openstack usage) > > > > James Page: > > This is not the default otherwise no ones cloud would actually work right now. > > Instances are created with a network interface in paused state - at > > which point the interface plugging in neutron is executed; once that's > > completed the instance transitions to running and boots. > > That's the default behaviour with neutron/ml2/ovs driver. > > Are you using a different neutron driver? > > > > Daniel Pawlik (bug reporter): > > I also use neutron ml2 ovs driver. I understand that in default nova > > policy, only administrator can spawn instance without any interface, > > but if someone else can "tune" the policy, he/she will have a problem. > > > > No other feedback was added so far. > > Today this was also added - quote here as FYI > "The same behavior is reproduced in case when VM was created with network > interface, interface detached, VM stopped (virsh destroy) and started > again without network. This usecase does not need OpenStack admin > privileges." > > > I personally tend to err on the side of caution and in this case would > > say an admin that wants to allow this can set up /dev/vhost-net as a > > local apparmor override. > > > > @Libvirt maintainers and @Jamie for apparmor guidance. > > Is in your opinion that enough of a common case that we'd add the the > > path as static rule in general? > > > > @JamesPage - you are likely the most experienced Openstack expert of > > us, what would be your opinion on the above? I maintain my preference as stated above for upstream libvirt since libvirt supports other configurations besides the default of running as non-root. Admins and distros are free to add the access as dictated by their requirements. I guess it would be conceivable to instead of addressing the real bug, checking if libvirt is configured to run VMs as non-root and if so, adding the access. This is really a band-aid on the problem and potentially confusing for users. I'd much rather see that effort go towards fixing the hotplug/unplug issue. -- Jamie Strandboge | http://www.canonical.com
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
