Re: [libvirt] [PATCH] qemu: Label all TAP FDs

On Tue, Aug 19, 2014 at 03:18:02PM +0200, Michal Privoznik wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1095636 > > When starting up the domain the domain's NICs are allocated. As of > 1f24f682 (v1.0.6) we are able to use multiqueue feature on virtio > NICs. It breaks network processing into multiple queues which can be > processed in parallel by different host CPUs. The queues are, however, > created by opening /dev/net/tun several times. Unfortunately, only the > first FD in the row is labelled so when turning the multiqueue feature > on in the guest, qemu will get AVC denial. Make sure we label all the > FDs needed. > > Moreover, the default label of /dev/net/tun doesn't allow > attaching a queue: > > type=AVC msg=audit(1399622478.790:893): avc: denied { attach_queue } > for pid=7585 comm="qemu-kvm" > scontext=system_u:system_r:svirt_t:s0:c638,c877 > tcontext=system_u:system_r:virtd_t:s0-s0:c0.c1023 Wow, I didn't even know you could have range in the "level" field (I think it doesn't make sense, but I don't understand why it is allowed by SELinux). > tclass=tun_socket > > And as suggested by SELinux maintainers, the tun FD should be labeled > as svirt_t. Therefore, we don't need to adjust any range (as done > previously by Guannan in ae368ebf) rather set the seclabel of the > domain directly. > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > src/qemu/qemu_command.c | 18 +++--------------- > src/qemu/qemu_hotplug.c | 6 ++++++ > src/security/security_selinux.c | 34 ++-------------------------------- > 3 files changed, 11 insertions(+), 47 deletions(-) > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index a92315a..4bc0368 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -192,10 +192,6 @@ qemuPhysIfaceConnect(virDomainDefPtr def, > vmop, cfg->stateDir, > virDomainNetGetActualBandwidth(net)); > if (rc >= 0) { > - if (virSecurityManagerSetTapFDLabel(driver->securityManager, > - def, rc) < 0) > - goto error; > - > virDomainAuditNetDevice(def, net, res_ifname, true); > VIR_FREE(net->ifname); > net->ifname = res_ifname; > @@ -203,17 +199,6 @@ qemuPhysIfaceConnect(virDomainDefPtr def, > > virObjectUnref(cfg); > return rc; > - > - error: > - ignore_value(virNetDevMacVLanDeleteWithVPortProfile( > - res_ifname, &net->mac, > - virDomainNetGetActualDirectDev(net), > - virDomainNetGetActualDirectMode(net), > - virDomainNetGetActualVirtPortProfile(net), > - cfg->stateDir)); > - VIR_FREE(res_ifname); > - virObjectUnref(cfg); > - return -1; > } > > > @@ -7201,6 +7186,9 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, > } > > for (i = 0; i < tapfdSize; i++) { > + if (virSecurityManagerSetTapFDLabel(driver->securityManager, > + def, tapfd[i]) < 0) > + goto cleanup; > virCommandPassFD(cmd, tapfd[i], > VIR_COMMAND_PASS_FD_CLOSE_PARENT); > if (virAsprintf(&tapfdName[i], "%d", tapfd[i]) < 0) > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index f7e223a..b60bd22 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -922,6 +922,12 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, > goto cleanup; > } > > + for (i = 0; i < tapfdSize; i++) { > + if (virSecurityManagerSetTapFDLabel(driver->securityManager, > + vm->def, tapfd[i]) < 0) > + goto cleanup; > + } > + Shouldn't there be the same loop for vhostfd[i]? Although it won't probably be > 1. Just ckecking.

> if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NET_NAME) || > virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { > if (qemuAssignDeviceNetAlias(vm->def, net, -1) < 0) > diff --git a/src/security/security_selinux.c > b/src/security/security_selinux.c > index c078cab..5d18493 100644 > --- a/src/security/security_selinux.c > +++ b/src/security/security_selinux.c > @@ -2330,47 +2330,17 @@ > virSecuritySELinuxSetImageFDLabel(virSecurityManagerPtr mgr > ATTRIBUTE_UNUSED, > } > > static int > -virSecuritySELinuxSetTapFDLabel(virSecurityManagerPtr mgr, > +virSecuritySELinuxSetTapFDLabel(virSecurityManagerPtr mgr > ATTRIBUTE_UNUSED, > virDomainDefPtr def, > int fd) > { > - struct stat buf; > - security_context_t fcon = NULL; > virSecurityLabelDefPtr secdef; > - char *str = NULL; > - int rc = -1; > > secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); > if (!secdef || !secdef->label) > return 0; > > - if (fstat(fd, &buf) < 0) { > - virReportSystemError(errno, _("cannot stat tap fd %d"), fd); > - goto cleanup; > - } > - > - if ((buf.st_mode & S_IFMT) != S_IFCHR) { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("tap fd %d is not character device"), fd); > - goto cleanup; > - } > - > - if (getContext(mgr, "/dev/tap.*", buf.st_mode, &fcon) < 0) { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("cannot lookup default selinux label for tap > fd %d"), fd); > - goto cleanup; > - } > - > - if (!(str = virSecuritySELinuxContextAddRange(secdef->label, > fcon))) { > - goto cleanup; > - } else { > - rc = virSecuritySELinuxFSetFilecon(fd, str); > - } > - > - cleanup: > - freecon(fcon); > - VIR_FREE(str); > - return rc; > + return virSecuritySELinuxFSetFilecon(fd, secdef->label); > } > This looks much better now. Although it looks way too similar to virSecuritySELinuxSetImageFDLabel() :) I think it might be worth keeping just one of these two functions, let's say ...SetFDLabel() in order not to complicate things, and assign it to both needed fields in the domain structure.

ACK with that changed.