Re: [PATCH] qemu: Label vhostuser net device

On 8/18/21 2: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. Agreed, particularly when an openvSwitch port is 'vhost-user' type https://docs.openvswitch.org/en/latest/topics/dpdk/vhost-user/ where openvSwitch acts as the server and qemu is the client. > 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. According to the above doc each vhostuser device would have its own socket "Note that a separate and different chardev path needs to be specified for each vhost-user device." > 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. Unfortunately I agree. The unfortunate part is the huge diff compared to this small patch :-).