Re: [libvirt] [PATCH 5/5] qemu: launch bridge helper from libvirtd

On 03/25/2013 10:25 AM, Paolo Bonzini wrote: > <source type='bridge'> uses a helper application to do the necessary > TUN/TAP setup to use an existing network bridge, thus letting > unprivileged users use TUN/TAP interfaces. > > However, libvirt should be preventing QEMU from running any setuid > programs at all, which would include this helper program. From > a security POV, any setuid helper needs to be run by libvirtd itself, > not QEMU. > > This is what this patch does. libvirt now invokes the setuid helper, > gets the TAP fd and then passes it to QEMU in the normal manner. > The path to the helper is specified in qemu.conf. > > As a small advantage, this adds a <target dev='tap0'/> element to the > XML of an active domain using <interface type='bridge'>. > > Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com&gt; > --- > src/qemu/qemu_command.c | 133 +++++++++++++++++++++++++++++++++++------------- > src/qemu/qemu_command.h | 1 - > src/qemu/qemu_hotplug.c | 25 +++------ > 3 files changed, 106 insertions(+), 53 deletions(-) > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index a0c278f..fa31102 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -28,12 +28,14 @@ > #include "qemu_capabilities.h" > #include "qemu_bridge_filter.h" > #include "cpu/cpu.h" > +#include "passfd.h" > #include "viralloc.h" > #include "virlog.h" > #include "virarch.h" > #include "virerror.h" > #include "virutil.h" > #include "virfile.h" > +#include "virnetdev.h" > #include "virstring.h" > #include "viruuid.h" > #include "c-ctype.h" > @@ -46,6 +48,9 @@ > #include "base64.h" > #include "device_conf.h" > #include "virstoragefile.h" > +#if defined(__linux__) > +# include <linux/capability.h> > +#endif > > #include <sys/stat.h> > #include <fcntl.h> > @@ -193,6 +198,77 @@ error: > } > > > +/** > + * qemuCreateInBridgePortWithHelper: > + * @cfg: the configuration object in which the helper name is looked up > + * @brname: the bridge name > + * @ifname: the returned interface name > + * @macaddr: the returned MAC address > + * @tapfd: file descriptor return value for the new tap device > + * @flags: OR of virNetDevTapCreateFlags: > + > + * VIR_NETDEV_TAP_CREATE_VNET_HDR > + * - Enable IFF_VNET_HDR on the tap device > + * > + * This function creates a new tap device on a bridge using an external > + * helper. The final name for the bridge will be stored in @ifname. > + * > + * Returns 0 in case of success or -1 on failure > + */ > +static int qemuCreateInBridgePortWithHelper(virQEMUDriverConfigPtr cfg, > + const char *brname, > + char **ifname, > + int *tapfd, > + unsigned int flags) > +{ > + virCommandPtr cmd; > + int status; > + int pair[2] = { -1, -1 }; > + > + if ((flags & ~VIR_NETDEV_TAP_CREATE_VNET_HDR) != VIR_NETDEV_TAP_CREATE_IFUP) > + return -1; > + > + if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair) < 0) { > + virReportSystemError(errno, "%s", _("failed to create socket")); > + return -1; > + } > + > + cmd = virCommandNew(cfg->bridgeHelperName); > + if (flags & VIR_NETDEV_TAP_CREATE_VNET_HDR) > + virCommandAddArgFormat(cmd, "--use-vnet"); > + virCommandAddArgFormat(cmd, "--br=%s", brname); > + virCommandAddArgFormat(cmd, "--fd=%d", pair[1]); > + virCommandTransferFD(cmd, pair[1]); > + virCommandClearCaps(cmd); > +#ifdef CAP_NET_ADMIN > + virCommandAllowCap(cmd, CAP_NET_ADMIN); I thought you had said that attempts to set caps would fail because CAP_SETPCAP was cleared for the non-privileged libvirtd.

> +#endif > + if (virCommandRunAsync(cmd, NULL) < 0) { > + *tapfd = -1; > + goto out; > + } > + > + do { > + *tapfd = recvfd(pair[0], 0); > + } while (*tapfd < 0 && errno == EINTR); > + if (*tapfd < 0) { > + virReportSystemError(errno, "%s", > + _("failed to retrieve file descriptor for interface")); > + goto out; > + } > + > + if (virNetDevTapGetName(*tapfd, ifname) < 0 || > + virCommandWait(cmd, &status) < 0) { > + VIR_FORCE_CLOSE(*tapfd); > + *tapfd = -1; > + } > + > +out: We prefer to name these labels "cleanup" rather that "out" (although there are some occurrences of "out" still in the code).

> + virCommandFree(cmd); > + VIR_FORCE_CLOSE(pair[0]); > + return *tapfd < 0 ? -1 : 0; > +} > + > int > qemuNetworkIfaceConnect(virDomainDefPtr def, > virConnectPtr conn, > @@ -270,11 +346,17 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, > tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR; > } > > - err = virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac, > - def->uuid, &tapfd, > - virDomainNetGetActualVirtPortProfile(net), > - virDomainNetGetActualVlan(net), > - tap_create_flags); > + if (cfg->privileged) > + err = virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac, > + def->uuid, &tapfd, > + virDomainNetGetActualVirtPortProfile(net), > + virDomainNetGetActualVlan(net), > + tap_create_flags); > + else > + err = qemuCreateInBridgePortWithHelper(cfg, brname, > + &net->ifname, > + &tapfd, tap_create_flags); > + > virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd >= 0); > if (err < 0) { > if (template_ifname) > @@ -3746,7 +3828,6 @@ error: > char * > qemuBuildHostNetStr(virDomainNetDefPtr net, > virQEMUDriverPtr driver, > - virQEMUCapsPtr qemuCaps, qemuCaps might again become useful for this function in the future, so you may want to leave it here (marked as ATTRIBUTE_UNUSED) to reduce code churn. > char type_sep, > int vlan, > const char *tapfd, > @@ -3755,7 +3836,6 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, > bool is_tap = false; > virBuffer buf = VIR_BUFFER_INITIALIZER; > enum virDomainNetType netType = virDomainNetGetActualType(net); > - const char *brname = NULL; > virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); > > if (net->script && netType != VIR_DOMAIN_NET_TYPE_ETHERNET) { > @@ -3773,14 +3853,6 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, > * through, -net tap,fd > */ > case VIR_DOMAIN_NET_TYPE_BRIDGE: > - if (!cfg->privileged && > - virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV_BRIDGE)) { > - brname = virDomainNetGetActualBridgeName(net); > - virBufferAsprintf(&buf, "bridge%cbr=%s", type_sep, brname); > - type_sep = ','; > - is_tap = true; > - break; > - } > case VIR_DOMAIN_NET_TYPE_NETWORK: > case VIR_DOMAIN_NET_TYPE_DIRECT: > virBufferAsprintf(&buf, "tap%cfd=%s", type_sep, tapfd); > @@ -6681,26 +6753,17 @@ qemuBuildCommandLine(virConnectPtr conn, > > if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || > actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { > - /* > - * If type='bridge' then we attempt to allocate the tap fd here only if > - * running under a privilged user or -netdev bridge option is not > - * supported. > - */ > - if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || > - cfg->privileged || > - (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV_BRIDGE))) { > - int tapfd = qemuNetworkIfaceConnect(def, conn, driver, net, > - qemuCaps); > - if (tapfd < 0) > - goto error; > + int tapfd = qemuNetworkIfaceConnect(def, conn, driver, net, > + qemuCaps); > + if (tapfd < 0) > + goto error; > > - last_good_net = i; > - virCommandTransferFD(cmd, tapfd); > + last_good_net = i; > + virCommandTransferFD(cmd, tapfd); > > - if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", > - tapfd) >= sizeof(tapfd_name)) > - goto no_memory; > - } > + if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", > + tapfd) >= sizeof(tapfd_name)) > + goto no_memory; > } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { > int tapfd = qemuPhysIfaceConnect(def, driver, net, > qemuCaps, vmop); > @@ -6744,7 +6807,7 @@ qemuBuildCommandLine(virConnectPtr conn, > if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && > virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { > virCommandAddArg(cmd, "-netdev"); > - if (!(host = qemuBuildHostNetStr(net, driver, qemuCaps, > + if (!(host = qemuBuildHostNetStr(net, driver, > ',', vlan, tapfd_name, > vhostfd_name))) > goto error; > @@ -6768,7 +6831,7 @@ qemuBuildCommandLine(virConnectPtr conn, > if (!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && > virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) { > virCommandAddArg(cmd, "-net"); > - if (!(host = qemuBuildHostNetStr(net, driver, qemuCaps, > + if (!(host = qemuBuildHostNetStr(net, driver, > ',', vlan, tapfd_name, > vhostfd_name))) > goto error; > diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h > index 17687f4..267a96e 100644 > --- a/src/qemu/qemu_command.h > +++ b/src/qemu/qemu_command.h > @@ -71,7 +71,6 @@ qemuBuildChrDeviceStr (virDomainChrDefPtr serial, > /* With vlan == -1, use netdev syntax, else old hostnet */ > char * qemuBuildHostNetStr(virDomainNetDefPtr net, > virQEMUDriverPtr driver, > - virQEMUCapsPtr qemuCaps, > char type_sep, > int vlan, > const char *tapfd, > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index de9edd4..6891efd 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -729,21 +729,12 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, > > if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || > actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { > - /* > - * If type=bridge then we attempt to allocate the tap fd here only if > - * running under a privilged user or -netdev bridge option is not > - * supported. > - */ > - if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || > - cfg->privileged || > - (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV_BRIDGE))) { > - if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, net, > - priv->qemuCaps)) < 0) > - goto cleanup; > - iface_connected = true; > - if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0) > - goto cleanup; > - } > + if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, net, > + priv->qemuCaps)) < 0) > + goto cleanup; > + iface_connected = true; > + if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0) > + goto cleanup; > } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { > if ((tapfd = qemuPhysIfaceConnect(vm->def, driver, net, > priv->qemuCaps, > @@ -803,12 +794,12 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, > > if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV) && > virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { > - if (!(netstr = qemuBuildHostNetStr(net, driver, priv->qemuCaps, > + if (!(netstr = qemuBuildHostNetStr(net, driver, > ',', -1, tapfd_name, > vhostfd_name))) > goto cleanup; > } else { > - if (!(netstr = qemuBuildHostNetStr(net, driver, priv->qemuCaps, > + if (!(netstr = qemuBuildHostNetStr(net, driver, > ' ', vlan, tapfd_name, > vhostfd_name))) > goto cleanup; I still don't like using qemu-bridge-helper, but this is better than the alternative of having qemu call it (although, due to the way that process capabilities works, we are unable to prevent a rogue qemu started by unprivileged libvirtd from calling it :-( ACK to this patch (I think I would prefer you left the qemuCaps arg in, but others may disagree with me.)