[libvirt] [PATCH] qemu: Label all TAP FDs
Michal Privoznik
mprivozn at redhat.com
Wed Aug 20 07:58:53 UTC 2014
On 20.08.2014 05:49, Martin Kletzander wrote:
> 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 at redhat.com>
>> ---
>> 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.
No, vhost FDs are not connected queue to. Frankly, I don't fully
understand all the details, but hey - it works :-)
>
>> 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.
Yes and no. While SetTapFDLabel needs to set domain process label
(seclabel->label) SetImageFDLabel needs to use image label
(seclabel->imagelabel). And I don't think it's worth wrap these two
functions into one wrapper function (I doubt it'll be readable more too).
>
> ACK with that changed.
So I'm taking this as ACK without any change required and pushing. Thanks!
Michal
More information about the libvir-list
mailing list