On 8/18/21 12:31 AM, Jim Fehlig wrote: > On 8/17/21 12:11 PM, Jim Fehlig wrote: >> 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 >> Yeah, you're right. But I view chardevs as runtime, fire and go things. I mean, you start a VM with a chardev, say an UNIX socket and its lifetime is identical to the VM lifetime. vhostuser on the other hand is handled by a third party daemon (typically OVS) and thus UNIX socket lifetime is different to VM lifetime. IOW, I worry that if we changed the UNIX socket label we might be preventing other VMs from connecting to it. And I don't even know if a single socket can be shared between two VMs. For instance, if OVS created an UNIX socket whether I can attach vhostuser interface to one domain and then to the other. Because if I could, then we should not change labels. Perhaps Laine can shed more light here. I do understand that apparmor needs to add an entry to the VM's profile, but I worry that DAC and SELinux might screw things up. >> >>> 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. I think it does sound more promising given my assumption above is correct. This way we can have only AppArmor driver implement the callback leaving us with consistent behavior. Michal