Re: [PATCH] qemu: Label vhostuser net device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 8/17/21 4:13 AM, Michal Prívozník wrote:
On 8/13/21 11:36 PM, Jim Fehlig wrote:
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. Label the chardev before calling
qemuMonitorAttachCharDev, and restore the label when removing the
net device.

Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
---
  src/qemu/qemu_hotplug.c | 9 +++++++++
  1 file changed, 9 insertions(+)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index c00e8a7852..42e7997112 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1467,6 +1467,11 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver,
      }
if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
+        virDomainChrDef chr = { .source = net->data.vhostuser };
+
+        if (qemuSecuritySetChardevLabel(driver, vm, &chr) < 0)
+            goto cleanup;
+
          if (qemuMonitorAttachCharDev(priv->mon, charDevAlias, net->data.vhostuser) < 0) {
              ignore_value(qemuDomainObjExitMonitor(driver, vm));
              virDomainAuditNet(vm, NULL, net, "attach", false);
@@ -4692,6 +4697,8 @@ qemuDomainRemoveNetDevice(virQEMUDriver *driver,
      }
if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
+        virDomainChrDef chr = { .source = net->data.vhostuser };
+
          /* vhostuser has a chardev too */
          if (qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0) {
              /* well, this is a messy situation. Guest visible PCI device has
@@ -4699,6 +4706,8 @@ qemuDomainRemoveNetDevice(virQEMUDriver *driver,
               * to just ignore the error and carry on.
               */
          }
+        if (qemuSecurityRestoreChardevLabel(driver, vm, &chr) < 0)
+            VIR_WARN("Unable to restore security label on vhostuser char device");
      } else if (actualType == VIR_DOMAIN_NET_TYPE_VDPA) {
          int vdpafdset = -1;
          g_autoptr(qemuMonitorFdsets) fdsets = NULL;


What an interesting bug. Previously we assumed that the UNIX socket is
created with broad enough permissions so that QEMU can connect to it. I
mean, when a VM is starting up nor DAC nor SELinux drivers care about
VHOSTUSER. It's AppArmor driver that cares. And it makes sense.
But, what's problematic here is that upon attach the socket perms will
be changed (because both DAC and SELinux drivers implement
domainSetSecurityChardevLabel callback). And since sockets can't have
XATTRs libvirt won't remember its original labels and thus subsequent
restore changes owner to root:root.

How are existing chardevs with socket backends handled? It seems they would suffer the same problem when restoring the labels. The DAC and selinux callbacks seem to avoid labeling for "server" sockets

https://gitlab.com/libvirt/libvirt/-/blob/master/src/security/security_dac.c#L1534
https://gitlab.com/libvirt/libvirt/-/blob/master/src/security/security_selinux.c#L2544

So I think we should address this inconsistency in behavior. But I don't
know how :-(

My first attempt at fixing this introduced domain{Set,Restore}SecurityNetdevLabel to the security driver, but it seemed like overkill after I discovered virDomainChrSourceDef embedded in the virDomainNetDef structure. I can revisit that approach if it sounds more promising.

Also, looking at the code itself - please move qemuSecurity* calls
outside of the monitor code. I mean, qemuDomainAttachNetDevice() has a
section that prepares interfaces before calling
qemuDomainObjEnterMonitor(). That looks like a better place to call
qemuSecuritySetChardevLabel(). Similarly, the restore in detach path
should be done outside of monitor handling code.

Good point. I'll change it in V2.

Oh, a side note - don't forget that hotplug can fail and thus the
seclabel should be restored if it was set. If you want to look around
for inspiration, we usually use teardownlabel variable for that.

Thanks for catching that! I'll fix it in V2.

Before working on V2 I'd like to resolve your first item. Is it safe to use qemuSecuritySetChardevLabel for a vhostuser net device? Is the underlying socket always in listen mode for these devices? At least when adding an openvswitch vhostuser port to a running VM, the underlying socket has 'server=true'

2021-04-22 15:29:40.261+0000: 643: debug : qemuMonitorJSONCheckError:401 : unable to execute QEMU command {"execute":"chardev-add","arguments":{"id":"charnet1","backend":{"type":"socket","data":{"addr":{"type":"unix","data":{"path":"/run/openvswitch/vhu2fca98d5-52"}},"wait":false,"server":true}}},"id":"libvirt-689"}: {"id":"libvirt-689","error":{"class":"GenericError","desc":"Failed to bind socket to /run/openvswitch/vhu2fca98d5-52: Permission denied"}}

Regards,
Jim





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux