Re: [PATCH] qemu: Label vhostuser net device

[Jim Fehlig <jfehlig@xxxxxxxx>]

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