qemu: Set label on vhostuser net device when hotplugging

Attaching a newly created vhostuser port to a VM fails due to an
apparmor denial

internal error: unable to execute QEMU command 'chardev-add': Failed
to bind socket to /run/openvswitch/vhu838c4d29-c9: Permission denied

In the case of a net device type VIR_DOMAIN_NET_TYPE_VHOSTUSER, the
underlying chardev is not labeled in qemuDomainAttachNetDevice prior
to calling qemuMonitorAttachCharDev.

A simple fix would be to call qemuSecuritySetChardevLabel using the
embedded virDomainChrSourceDef in the virDomainNetDef vhostuser data,
but this incurs the risk of incorrectly restoring the label. E.g.
consider the DAC driver behavior with a vhostuser net device, which
uses a socket for the chardev backend. The DAC driver uses XATTRS to
store original labelling information, but XATTRS are not compatible
with sockets. Without the original labelling information, the socket
labels will be restored with root ownership, preventing other
less-privileged processes from connecting to the socket.

This patch avoids overloading chardev labelling with vhostuser net
devices by introducing virSecurityManager{Set,Restore}NetdevLabel,
which is currently only implemented for the apparmor driver. The
new APIs are then used to set and restore labels for the vhostuser
net devices.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index fa11ee3df5811bbf90b49c732ec4ddfe59d0eb22..ab8a6c00c3bd435e365a91c799872fd7c296479a 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1688,6 +1688,7 @@ virSecurityManagerRestoreHostdevLabel;
 virSecurityManagerRestoreImageLabel;
 virSecurityManagerRestoreInputLabel;
 virSecurityManagerRestoreMemoryLabel;
+virSecurityManagerRestoreNetdevLabel;
 virSecurityManagerRestoreSavedStateLabel;
 virSecurityManagerRestoreTPMLabels;
 virSecurityManagerSetAllLabel;
@@ -1699,6 +1700,7 @@ virSecurityManagerSetImageFDLabel;
 virSecurityManagerSetImageLabel;
 virSecurityManagerSetInputLabel;
 virSecurityManagerSetMemoryLabel;
+virSecurityManagerSetNetdevLabel;
 virSecurityManagerSetProcessLabel;
 virSecurityManagerSetSavedStateLabel;
 virSecurityManagerSetSocketLabel;

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 64580b573b2c9209f54f2e662dda826083fce241..9c16ab456755df6d3c8344f4b0d505b6f664488b 100644 (file)
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1203,6 +1203,7 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver,
     g_autofree char *netdev_name = NULL;
     g_autoptr(virConnect) conn = NULL;
     virErrorPtr save_err = NULL;
+    bool teardownlabel = false;
 
     /* If appropriate, grab a physical device from the configured
      * network's pool of devices, or resolve bridge device name
@@ -1343,6 +1344,9 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver,
                                                    &net->ifname) < 0)
             goto cleanup;
 
+        if (qemuSecuritySetNetdevLabel(driver, vm, net) < 0)
+            goto cleanup;
+        teardownlabel = true;
         break;
 
     case VIR_DOMAIN_NET_TYPE_USER:
@@ -1559,6 +1563,10 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver,
             qemuDomainNetDeviceVportRemove(net);
         }
 
+        if (teardownlabel &&
+            qemuSecurityRestoreNetdevLabel(driver, vm, net) < 0)
+            VIR_WARN("Unable to restore network device labelling on hotplug fail");
+
         /* we had potentially pre-added the device to the domain
          * device lists, if so we need to remove it (from def->nets
          * and/or def->hostdevs) on failure
@@ -4803,6 +4811,11 @@ qemuDomainRemoveNetDevice(virQEMUDriver *driver,
                          cfg->stateDir));
     }
 
+    if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
+        if (qemuSecurityRestoreNetdevLabel(driver, vm, net) < 0)
+            VIR_WARN("Unable to restore security label on vhostuser char device");
+    }
+
     qemuDomainNetDeviceVportRemove(net);
 
     if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {

diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index e582a66071a1f71be33a9cb8ca02c05132cdcf94..19d957dd4b9665d564af0055d30a3a88f5cca6d5 100644 (file)
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -439,6 +439,65 @@ qemuSecurityRestoreChardevLabel(virQEMUDriver *driver,
     return ret;
 }
 
+int
+qemuSecuritySetNetdevLabel(virQEMUDriver *driver,
+                           virDomainObj *vm,
+                           virDomainNetDef *net)
+{
+    int ret = -1;
+    qemuDomainObjPrivate *priv = vm->privateData;
+    pid_t pid = -1;
+
+    if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
+        pid = vm->pid;
+
+    if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
+        goto cleanup;
+
+    if (virSecurityManagerSetNetdevLabel(driver->securityManager,
+                                         vm->def, net) < 0)
+        goto cleanup;
+
+    if (virSecurityManagerTransactionCommit(driver->securityManager,
+                                            pid, priv->rememberOwner) < 0)
+        goto cleanup;
+
+    ret = 0;
+ cleanup:
+    virSecurityManagerTransactionAbort(driver->securityManager);
+    return ret;
+}
+
+
+int
+qemuSecurityRestoreNetdevLabel(virQEMUDriver *driver,
+                               virDomainObj *vm,
+                               virDomainNetDef *net)
+{
+    int ret = -1;
+    qemuDomainObjPrivate *priv = vm->privateData;
+    pid_t pid = -1;
+
+    if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
+        pid = vm->pid;
+
+    if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
+        goto cleanup;
+
+    if (virSecurityManagerRestoreNetdevLabel(driver->securityManager,
+                                             vm->def, net) < 0)
+        goto cleanup;
+
+    if (virSecurityManagerTransactionCommit(driver->securityManager,
+                                            pid, priv->rememberOwner) < 0)
+        goto cleanup;
+
+    ret = 0;
+ cleanup:
+    virSecurityManagerTransactionAbort(driver->securityManager);
+    return ret;
+}
+
 
 /*
  * qemuSecurityStartVhostUserGPU:

diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h
index 4c3d81e4b5e2a3464c596072525dce08bf2b71c2..8b26ea3f9922a57b485e2dfc065e7bdad4ad9098 100644 (file)
--- a/src/qemu/qemu_security.h
+++ b/src/qemu/qemu_security.h
@@ -79,6 +79,14 @@ int qemuSecurityRestoreChardevLabel(virQEMUDriver *driver,
                                     virDomainObj *vm,
                                     virDomainChrDef *chr);
 
+int qemuSecuritySetNetdevLabel(virQEMUDriver *driver,
+                               virDomainObj *vm,
+                               virDomainNetDef *net);
+
+int qemuSecurityRestoreNetdevLabel(virQEMUDriver *driver,
+                                   virDomainObj *vm,
+                                   virDomainNetDef *net);
+
 int qemuSecurityStartVhostUserGPU(virQEMUDriver *driver,
                                   virDomainObj *vm,
                                   virCommand *cmd,

diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 84363015dc9c8919ee66a5002d18da1d419ba134..d942ea5005a26258cf7671aa99379897d0b4f105 100644 (file)
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -1053,6 +1053,64 @@ AppArmorRestoreChardevLabel(virSecurityManager *mgr,
     return reload_profile(mgr, def, NULL, false);
 }
 
+static int
+AppArmorSetNetdevLabel(virSecurityManager *mgr,
+                       virDomainDef *def,
+                       virDomainNetDef *net)
+{
+    int ret = -1;
+    virSecurityLabelDef *secdef;
+    virDomainChrSourceDef *dev_source;
+    virDomainNetType actualType;
+
+    secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
+    if (!secdef)
+        return 0;
+
+    actualType = virDomainNetGetActualType(net);
+    if (actualType != VIR_DOMAIN_NET_TYPE_VHOSTUSER)
+        return 0;
+
+    dev_source = net->data.vhostuser;
+    switch ((virDomainChrType)dev_source->type) {
+    case VIR_DOMAIN_CHR_TYPE_UNIX:
+        ret = reload_profile(mgr, def, dev_source->data.file.path, true);
+        break;
+
+    case VIR_DOMAIN_CHR_TYPE_DEV:
+    case VIR_DOMAIN_CHR_TYPE_FILE:
+    case VIR_DOMAIN_CHR_TYPE_PTY:
+    case VIR_DOMAIN_CHR_TYPE_PIPE:
+    case VIR_DOMAIN_CHR_TYPE_SPICEPORT:
+    case VIR_DOMAIN_CHR_TYPE_NULL:
+    case VIR_DOMAIN_CHR_TYPE_VC:
+    case VIR_DOMAIN_CHR_TYPE_STDIO:
+    case VIR_DOMAIN_CHR_TYPE_UDP:
+    case VIR_DOMAIN_CHR_TYPE_TCP:
+    case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
+    case VIR_DOMAIN_CHR_TYPE_NMDM:
+    case VIR_DOMAIN_CHR_TYPE_LAST:
+        ret = 0;
+        break;
+    }
+
+    return ret;
+}
+
+static int
+AppArmorRestoreNetdevLabel(virSecurityManager *mgr,
+                           virDomainDef *def,
+                           virDomainNetDef *net G_GNUC_UNUSED)
+{
+    virSecurityLabelDef *secdef;
+
+    secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
+    if (!secdef)
+        return 0;
+
+    return reload_profile(mgr, def, NULL, false);
+}
+
 static int
 AppArmorSetPathLabel(virSecurityManager *mgr,
                            virDomainDef *def,
@@ -1168,6 +1226,9 @@ virSecurityDriver virAppArmorSecurityDriver = {
     .domainSetSecurityChardevLabel      = AppArmorSetChardevLabel,
     .domainRestoreSecurityChardevLabel  = AppArmorRestoreChardevLabel,
 
+    .domainSetSecurityNetdevLabel       = AppArmorSetNetdevLabel,
+    .domainRestoreSecurityNetdevLabel   = AppArmorRestoreNetdevLabel,
+
     .domainSetSecurityImageFDLabel      = AppArmorSetFDLabel,
     .domainSetSecurityTapFDLabel        = AppArmorSetFDLabel,
 

diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index 07f8def3d3c644dda9407e2df488818c3ec0489f..a1fc23be383f533fd79af44af435611a5c5346d2 100644 (file)
--- a/src/security/security_driver.h
+++ b/src/security/security_driver.h
@@ -157,6 +157,12 @@ typedef int (*virSecurityDomainSetTPMLabels) (virSecurityManager *mgr,
                                               virDomainDef *def);
 typedef int (*virSecurityDomainRestoreTPMLabels) (virSecurityManager *mgr,
                                                   virDomainDef *def);
+typedef int (*virSecurityDomainSetNetdevLabel) (virSecurityManager *mgr,
+                                                virDomainDef *def,
+                                                virDomainNetDef *net);
+typedef int (*virSecurityDomainRestoreNetdevLabel) (virSecurityManager *mgr,
+                                                    virDomainDef *def,
+                                                    virDomainNetDef *net);
 
 
 struct _virSecurityDriver {
@@ -224,6 +230,9 @@ struct _virSecurityDriver {
 
     virSecurityDomainSetTPMLabels domainSetSecurityTPMLabels;
     virSecurityDomainRestoreTPMLabels domainRestoreSecurityTPMLabels;
+
+    virSecurityDomainSetNetdevLabel domainSetSecurityNetdevLabel;
+    virSecurityDomainRestoreNetdevLabel domainRestoreSecurityNetdevLabel;
 };
 
 virSecurityDriver *virSecurityDriverLookup(const char *name,

diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 9906c1691d0f6ffa505ed5cdb6f6a1ef6e72139b..d8a03a19cb8b0054d7b2759b58c7b63af1a86c23 100644 (file)
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -1297,6 +1297,44 @@ virSecurityManagerRestoreTPMLabels(virSecurityManager *mgr,
 }
 
 
+int
+virSecurityManagerSetNetdevLabel(virSecurityManager *mgr,
+                                 virDomainDef *vm,
+                                 virDomainNetDef *net)
+{
+    int ret;
+
+    if (mgr->drv->domainSetSecurityNetdevLabel) {
+        virObjectLock(mgr);
+        ret = mgr->drv->domainSetSecurityNetdevLabel(mgr, vm, net);
+        virObjectUnlock(mgr);
+
+        return ret;
+    }
+
+    return 0;
+}
+
+
+int
+virSecurityManagerRestoreNetdevLabel(virSecurityManager *mgr,
+                                     virDomainDef *vm,
+                                     virDomainNetDef *net)
+{
+    int ret;
+
+    if (mgr->drv->domainRestoreSecurityNetdevLabel) {
+        virObjectLock(mgr);
+        ret = mgr->drv->domainRestoreSecurityNetdevLabel(mgr, vm, net);
+        virObjectUnlock(mgr);
+
+        return ret;
+    }
+
+    return 0;
+}
+
+
 static int
 cmpstringp(const void *p1, const void *p2)
 {

diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index 57047ccb137d7e0f098bf887b0171e420687d78a..59020b147527583a21c01dac593a5b68abaa5921 100644 (file)
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -220,6 +220,14 @@ int virSecurityManagerSetTPMLabels(virSecurityManager *mgr,
 int virSecurityManagerRestoreTPMLabels(virSecurityManager *mgr,
                                        virDomainDef *vm);
 
+int virSecurityManagerSetNetdevLabel(virSecurityManager *mgr,
+                                     virDomainDef *vm,
+                                     virDomainNetDef *net);
+
+int virSecurityManagerRestoreNetdevLabel(virSecurityManager *mgr,
+                                         virDomainDef *vm,
+                                         virDomainNetDef *net);
+
 typedef struct _virSecurityManagerMetadataLockState virSecurityManagerMetadataLockState;
 struct _virSecurityManagerMetadataLockState {
     size_t nfds; /* Captures size of both @fds and @paths */

diff --git a/src/security/security_stack.c b/src/security/security_stack.c
index f7a9ed1e33a3fa0bacea80a797191bd940afb1cb..3c2239910aa588b0fad4b45956b8b5332a9f0a17 100644 (file)
--- a/src/security/security_stack.c
+++ b/src/security/security_stack.c
@@ -963,6 +963,55 @@ virSecurityStackRestoreTPMLabels(virSecurityManager *mgr,
 }
 
 
+static int
+virSecurityStackDomainSetNetdevLabel(virSecurityManager *mgr,
+                                     virDomainDef *def,
+                                     virDomainNetDef *net)
+{
+    virSecurityStackData *priv = virSecurityManagerGetPrivateData(mgr);
+    virSecurityStackItem *item = priv->itemsHead;
+
+    for (; item; item = item->next) {
+        if (virSecurityManagerSetNetdevLabel(item->securityManager, def, net) < 0)
+            goto rollback;
+    }
+
+    return 0;
+
+ rollback:
+    for (item = item->prev; item; item = item->prev) {
+        if (virSecurityManagerRestoreNetdevLabel(item->securityManager,
+                                                 def, net) < 0) {
+            VIR_WARN("Unable to restore netdev label after failed set label "
+                     "call virDriver=%s driver=%s domain=%s",
+                     virSecurityManagerGetVirtDriver(mgr),
+                     virSecurityManagerGetDriver(item->securityManager),
+                     def->name);
+        }
+    }
+    return -1;
+}
+
+
+static int
+virSecurityStackDomainRestoreNetdevLabel(virSecurityManager *mgr,
+                                         virDomainDef *def,
+                                         virDomainNetDef *net)
+{
+    virSecurityStackData *priv = virSecurityManagerGetPrivateData(mgr);
+    virSecurityStackItem *item = priv->itemsHead;
+    int rc = 0;
+
+    for (; item; item = item->next) {
+        if (virSecurityManagerRestoreNetdevLabel(item->securityManager,
+                                                 def, net) < 0)
+            rc = -1;
+    }
+
+    return rc;
+}
+
+
 virSecurityDriver virSecurityDriverStack = {
     .privateDataLen                     = sizeof(virSecurityStackData),
     .name                               = "stack",
@@ -1028,4 +1077,7 @@ virSecurityDriver virSecurityDriverStack = {
 
     .domainSetSecurityTPMLabels         = virSecurityStackSetTPMLabels,
     .domainRestoreSecurityTPMLabels     = virSecurityStackRestoreTPMLabels,
+
+    .domainSetSecurityNetdevLabel      = virSecurityStackDomainSetNetdevLabel,
+    .domainRestoreSecurityNetdevLabel  = virSecurityStackDomainRestoreNetdevLabel,
 };