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