[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 redhat com>
> ---
>  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.)


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]