Re: [PATCH] qemu: Label vhostuser net device

On Wed, Aug 18, 2021 at 09:00:52AM -0400, Laine Stump wrote: > On 8/18/21 4:21 AM, Michal Prívozník wrote: >> 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(a)suse.com&gt; >>>>>> --- >>>>>>   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_da... >>>> >>>> https://gitlab.com/libvirt/libvirt/-/blob/master/src/security/security_se... >>>> >> >> 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. > > Sorry, I really know nothing about this topic. Who would be the proper > person at the next level down to ask? Michael, do you know? hmm... Maxime? > >> >> 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 >>