3:21 a.m.
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>
>>> ---
>>> 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.
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