[PATCH] qemu: Label vhostuser net device

Jim Fehlig posted 1 patch 2 years, 7 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20210813213651.17073-1-jfehlig@suse.com
src/qemu/qemu_hotplug.c | 9 +++++++++
1 file changed, 9 insertions(+)
[PATCH] qemu: Label vhostuser net device
Posted by Jim Fehlig 2 years, 7 months ago
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@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;
-- 
2.32.0


Re: [PATCH] qemu: Label vhostuser net device
Posted by Michal Prívozník 2 years, 7 months ago
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@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.

So I think we should address this inconsistency in behavior. But I don't
know how :-(

Also, looking at the code itself - please move qemuSecurity* calls
outside of the monitor code. I mean, qemuDomainAttachNetDevice() has a
section that prepares interfaces before calling
qemuDomainObjEnterMonitor(). That looks like a better place to call
qemuSecuritySetChardevLabel(). Similarly, the restore in detach path
should be done outside of monitor handling code.
Oh, a side note - don't forget that hotplug can fail and thus the
seclabel should be restored if it was set. If you want to look around
for inspiration, we usually use teardownlabel variable for that.

Michal

Re: [PATCH] qemu: Label vhostuser net device
Posted by Jim Fehlig 2 years, 7 months ago
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@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

> 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.

> Also, looking at the code itself - please move qemuSecurity* calls
> outside of the monitor code. I mean, qemuDomainAttachNetDevice() has a
> section that prepares interfaces before calling
> qemuDomainObjEnterMonitor(). That looks like a better place to call
> qemuSecuritySetChardevLabel(). Similarly, the restore in detach path
> should be done outside of monitor handling code.

Good point. I'll change it in V2.

> Oh, a side note - don't forget that hotplug can fail and thus the
> seclabel should be restored if it was set. If you want to look around
> for inspiration, we usually use teardownlabel variable for that.

Thanks for catching that! I'll fix it in V2.

Before working on V2 I'd like to resolve your first item. Is it safe to use 
qemuSecuritySetChardevLabel for a vhostuser net device? Is the underlying socket 
always in listen mode for these devices? At least when adding an openvswitch 
vhostuser port to a running VM, the underlying socket has 'server=true'

2021-04-22 15:29:40.261+0000: 643: debug : qemuMonitorJSONCheckError:401 : 
unable to execute QEMU command 
{"execute":"chardev-add","arguments":{"id":"charnet1","backend":{"type":"socket","data":{"addr":{"type":"unix","data":{"path":"/run/openvswitch/vhu2fca98d5-52"}},"wait":false,"server":true}}},"id":"libvirt-689"}: 
{"id":"libvirt-689","error":{"class":"GenericError","desc":"Failed to bind 
socket to /run/openvswitch/vhu2fca98d5-52: Permission denied"}}

Regards,
Jim


Re: [PATCH] qemu: Label vhostuser net device
Posted by Jim Fehlig 2 years, 7 months ago
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@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 
> 
> 
>> 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.
> 
>> Also, looking at the code itself - please move qemuSecurity* calls
>> outside of the monitor code. I mean, qemuDomainAttachNetDevice() has a
>> section that prepares interfaces before calling
>> qemuDomainObjEnterMonitor(). That looks like a better place to call
>> qemuSecuritySetChardevLabel(). Similarly, the restore in detach path
>> should be done outside of monitor handling code.
> 
> Good point. I'll change it in V2.
> 
>> Oh, a side note - don't forget that hotplug can fail and thus the
>> seclabel should be restored if it was set. If you want to look around
>> for inspiration, we usually use teardownlabel variable for that.
> 
> Thanks for catching that! I'll fix it in V2.
> 
> Before working on V2 I'd like to resolve your first item. Is it safe to use 
> qemuSecuritySetChardevLabel for a vhostuser net device? Is the underlying socket 
> always in listen mode for these devices?

Answering my own question: No. A vhostuser interface can have a 
mode='client|server' setting on the <source> element describing the socket.

Perhaps the big hammer of adding domain{Set,Restore}SecurityNetdevLabel APIs is 
what's needed for this small nail?

Regards,
Jim

> At least when adding an openvswitch 
> vhostuser port to a running VM, the underlying socket has 'server=true'
> 
> 2021-04-22 15:29:40.261+0000: 643: debug : qemuMonitorJSONCheckError:401 : 
> unable to execute QEMU command 
> {"execute":"chardev-add","arguments":{"id":"charnet1","backend":{"type":"socket","data":{"addr":{"type":"unix","data":{"path":"/run/openvswitch/vhu2fca98d5-52"}},"wait":false,"server":true}}},"id":"libvirt-689"}: 
> {"id":"libvirt-689","error":{"class":"GenericError","desc":"Failed to bind 
> socket to /run/openvswitch/vhu2fca98d5-52: Permission denied"}}
> 
> Regards,
> Jim


Re: [PATCH] qemu: Label vhostuser net device
Posted by Michal Prívozník 2 years, 7 months ago
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@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.

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

Re: [PATCH] qemu: Label vhostuser net device
Posted by Laine Stump 2 years, 7 months ago
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@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.
> 
> 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?


> 
> 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
> 

Re: [PATCH] qemu: Label vhostuser net device
Posted by Michael S. Tsirkin 2 years, 7 months ago
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@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.
> > 
> > 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
> > 

Re: [PATCH] qemu: Label vhostuser net device
Posted by Maxime Coquelin 2 years, 7 months ago

On 8/19/21 11:10 AM, Michael S. Tsirkin wrote:
> 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@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.
>>>
>>> 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.

In theory, I think there is nothing that prevents a Vhost-user socket
created by OVS to be used by one VM and later by another one. But in
practice, the management layer likely request OVS a new socket on VM
creation.

I'd like to add that the mode where OVS is the Vhost-user socket server
is deprecated in OVS (but not yet removed) [0]:

"
Open vSwitch provides two types of vHost User ports:

    vhost-user (dpdkvhostuser)
    vhost-user-client (dpdkvhostuserclient)

vHost User uses a client-server model. The server
creates/manages/destroys the vHost User sockets, and the client connects
to the server. Depending on which port type you use, dpdkvhostuser or
dpdkvhostuserclient, a different configuration of the client-server
model is used.

For vhost-user ports, Open vSwitch acts as the server and QEMU the
client. This means if OVS dies, all VMs must be restarted. On the other
hand, for vhost-user-client ports, OVS acts as the client and QEMU the
server. This means OVS can die and be restarted without issue, and it is
also possible to restart an instance itself. For this reason,
vhost-user-client ports are the preferred type for all known use cases;
the only limitation is that vhost-user client mode ports require QEMU
version 2.7. Ports of type vhost-user are currently deprecated and will
be removed in a future release.
"

>>>
>>> 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
>>>
> 

Re: [PATCH] qemu: Label vhostuser net device
Posted by Jim Fehlig 2 years, 7 months ago
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@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 :-).

Jim


Re: [PATCH] qemu: Label vhostuser net device
Posted by Jim Fehlig 2 years, 7 months ago
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@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