[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



Il 18/04/2013 19:32, Laine Stump ha scritto:
> 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.

I still prefer to note down the capabilities.  This way running libvirt
with CAP_SETPCAP as a permitted file capability (and only that one + the
effective bit set) should do the right thing.

I tested this by forcing this path for the system libvirt.

> 
>> +#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).

Ok.

Paolo

>> +    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]