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