4:10 a.m.
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(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.
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
>