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@xxxxxxxxxx>
> ---
> 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.)
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list