[PATCH] qemu: Label vhostuser net device

[Jim Fehlig]

On 8/18/21 12:03 PM, Jim Fehlig wrote:
> 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 at suse.com>
>>>>>> ---
>>>>>>    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.
> 
> 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 :-).

Actual size seen here

https://listman.redhat.com/archives/libvir-list/2021-August/msg00547.html

Jim