[libvirt] [PATCH] network: restrict usage of port management APIs

Daniel P. Berrangé posted 1 patch 5 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180808154609.24032-1-berrange@redhat.com
Test syntax-check passed
src/conf/domain_conf.c      | 12 +++++-------
src/libxl/libxl_domain.c    |  6 ++++--
src/libxl/libxl_driver.c    |  9 ++++++---
src/lxc/lxc_driver.c        |  6 ++++--
src/lxc/lxc_process.c       | 10 ++++++++--
src/network/bridge_driver.c | 22 +++++++++++++++-------
src/qemu/qemu_hotplug.c     | 17 +++++++++++------
src/qemu/qemu_process.c     |  9 ++++++---
8 files changed, 59 insertions(+), 32 deletions(-)
[libvirt] [PATCH] network: restrict usage of port management APIs
Posted by Daniel P. Berrangé 5 years, 7 months ago
The port allocation APIs are currently called unconditionally for all
types of NIC, but (mostly) only do anything for NICs with type=network.

The exception is the port allocate API which does some validation even
for NICs with type!=network. Relying on this validation is flawed,
however, since the network driver may not even be installed, so virt
drivers must not delegation validation to it for NICs with
type!=network.

This change allows us to report errors when the virtual network driver
is not registered.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/conf/domain_conf.c      | 12 +++++-------
 src/libxl/libxl_domain.c    |  6 ++++--
 src/libxl/libxl_driver.c    |  9 ++++++---
 src/lxc/lxc_driver.c        |  6 ++++--
 src/lxc/lxc_process.c       | 10 ++++++++--
 src/network/bridge_driver.c | 22 +++++++++++++++-------
 src/qemu/qemu_hotplug.c     | 17 +++++++++++------
 src/qemu/qemu_process.c     |  9 ++++++---
 8 files changed, 59 insertions(+), 32 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index adcd8f41b9..e7d2acdcc9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -29947,13 +29947,11 @@ int
 virDomainNetAllocateActualDevice(virDomainDefPtr dom,
                                  virDomainNetDefPtr iface)
 {
-    /* Just silently ignore if network driver isn't present. If something
-     * has tried to use a NIC with type=network, other code will already
-     * cause an error. This ensures type=bridge doesn't break when
-     * network driver is compiled out.
-     */
-    if (!netAllocate)
-        return 0;
+    if (!netAllocate) {
+        virReportError(VIR_ERR_NO_SUPPORT, "%s",
+                       _("Virtual networking driver is not available"));
+        return -1;
+    }
 
     return netAllocate(dom, iface);
 }
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 2ab78ac9a5..c78d5ee96c 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -791,7 +791,8 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
 
             /* cleanup actual device */
             virDomainNetRemoveHostdev(vm->def, net);
-            virDomainNetReleaseActualDevice(vm->def, net);
+            if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
+                virDomainNetReleaseActualDevice(vm->def, net);
         }
     }
 
@@ -948,7 +949,8 @@ libxlNetworkPrepareDevices(virDomainDefPtr def)
          * network's pool of devices, or resolve bridge device name
          * to the one defined in the network definition.
          */
-        if (virDomainNetAllocateActualDevice(def, net) < 0)
+        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
+            virDomainNetAllocateActualDevice(def, net) < 0)
             return -1;
 
         actualType = virDomainNetGetActualType(net);
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 5a5e792957..fb5f046ade 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -3264,7 +3264,8 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver,
      * network's pool of devices, or resolve bridge device name
      * to the one defined in the network definition.
      */
-    if (virDomainNetAllocateActualDevice(vm->def, net) < 0)
+    if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
+        virDomainNetAllocateActualDevice(vm->def, net) < 0)
         goto cleanup;
 
     actualType = virDomainNetGetActualType(net);
@@ -3314,7 +3315,8 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver,
         vm->def->nets[vm->def->nnets++] = net;
     } else {
         virDomainNetRemoveHostdev(vm->def, net);
-        virDomainNetReleaseActualDevice(vm->def, net);
+        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
+            virDomainNetReleaseActualDevice(vm->def, net);
     }
     virObjectUnref(cfg);
     return ret;
@@ -3737,7 +3739,8 @@ libxlDomainDetachNetDevice(libxlDriverPrivatePtr driver,
  cleanup:
     libxl_device_nic_dispose(&nic);
     if (!ret) {
-        virDomainNetReleaseActualDevice(vm->def, detach);
+        if (detach->type == VIR_DOMAIN_NET_TYPE_NETWORK)
+            virDomainNetReleaseActualDevice(vm->def, detach);
         virDomainNetRemove(vm->def, detachidx);
     }
     virObjectUnref(cfg);
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 8867645cdc..8729fc0174 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3871,7 +3871,8 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn,
      * network's pool of devices, or resolve bridge device name
      * to the one defined in the network definition.
      */
-    if (virDomainNetAllocateActualDevice(vm->def, net) < 0)
+    if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
+        virDomainNetAllocateActualDevice(vm->def, net) < 0)
         return -1;
 
     actualType = virDomainNetGetActualType(net);
@@ -4425,7 +4426,8 @@ lxcDomainDetachDeviceNetLive(virDomainObjPtr vm,
     ret = 0;
  cleanup:
     if (!ret) {
-        virDomainNetReleaseActualDevice(vm->def, detach);
+        if (detach->type == VIR_DOMAIN_NET_TYPE_NETWORK)
+            virDomainNetReleaseActualDevice(vm->def, detach);
         virDomainNetRemove(vm->def, detachidx);
         virDomainNetDefFree(detach);
     }
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 33c806630b..7a6b40d9b8 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -213,7 +213,8 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver,
                                 iface->ifname));
             ignore_value(virNetDevVethDelete(iface->ifname));
         }
-        virDomainNetReleaseActualDevice(vm->def, iface);
+        if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK)
+            virDomainNetReleaseActualDevice(vm->def, iface);
     }
 
     virDomainConfVMNWFilterTeardown(vm);
@@ -547,6 +548,10 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn,
         if (virLXCProcessValidateInterface(net) < 0)
             goto cleanup;
 
+        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
+            virDomainNetAllocateActualDevice(def, net) < 0)
+            goto cleanup;
+
         if (virDomainNetAllocateActualDevice(def, net) < 0)
             goto cleanup;
 
@@ -626,7 +631,8 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn,
                 ignore_value(virNetDevOpenvswitchRemovePort(
                                 virDomainNetGetActualBridgeName(iface),
                                 iface->ifname));
-            virDomainNetReleaseActualDevice(def, iface);
+            if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK)
+                virDomainNetReleaseActualDevice(def, iface);
         }
     }
     return ret;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index f92cc61e47..c44cb73c5b 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -4458,8 +4458,11 @@ networkAllocateActualDevice(virDomainDefPtr dom,
     size_t i;
     int ret = -1;
 
-    if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
-        goto validate;
+    if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("Expected a interface for a virtual network"));
+        goto error;
+    }
 
     virDomainActualNetDefFree(iface->data.network.actual);
     iface->data.network.actual = NULL;
@@ -4778,7 +4781,6 @@ networkAllocateActualDevice(virDomainDefPtr dom,
     if (virNetDevVPortProfileCheckComplete(virtport, true) < 0)
         goto error;
 
- validate:
     /* make sure that everything now specified for the device is
      * actually supported on this type of network. NB: network,
      * netdev, and iface->data.network.actual may all be NULL.
@@ -4881,8 +4883,11 @@ networkNotifyActualDevice(virDomainDefPtr dom,
     size_t i;
     char *master = NULL;
 
-    if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
-        return;
+    if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("Expected a interface for a virtual network"));
+        goto error;
+    }
 
     obj = virNetworkObjFindByName(driver->networks, iface->data.network.name);
     if (!obj) {
@@ -5114,8 +5119,11 @@ networkReleaseActualDevice(virDomainDefPtr dom,
     size_t i;
     int ret = -1;
 
-    if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
-        return 0;
+    if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("Expected a interface for a virtual network"));
+        goto error;
+    }
 
     obj = virNetworkObjFindByName(driver->networks, iface->data.network.name);
     if (!obj) {
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 1488f0a7c2..512fead050 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1062,7 +1062,8 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
      * network's pool of devices, or resolve bridge device name
      * to the one defined in the network definition.
      */
-    if (virDomainNetAllocateActualDevice(vm->def, net) < 0)
+    if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
+        virDomainNetAllocateActualDevice(vm->def, net) < 0)
         goto cleanup;
 
     actualType = virDomainNetGetActualType(net);
@@ -1352,7 +1353,8 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
 
         virDomainNetRemoveHostdev(vm->def, net);
 
-        virDomainNetReleaseActualDevice(vm->def, net);
+        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
+            virDomainNetReleaseActualDevice(vm->def, net);
     }
 
     VIR_FREE(nicstr);
@@ -3722,7 +3724,8 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
 
         /* this function doesn't work with HOSTDEV networks yet, thus
          * no need to change the pointer in the hostdev structure */
-        virDomainNetReleaseActualDevice(vm->def, olddev);
+        if (olddev->type == VIR_DOMAIN_NET_TYPE_NETWORK)
+            virDomainNetReleaseActualDevice(vm->def, olddev);
         virDomainNetDefFree(olddev);
         /* move newdev into the nets list, and NULL it out from the
          * virDomainDeviceDef that we were given so that the caller
@@ -3753,7 +3756,7 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
      * that the changes were minor enough that we didn't need to
      * replace the entire device object.
      */
-    if (newdev)
+    if (newdev && newdev->type == VIR_DOMAIN_NET_TYPE_NETWORK)
         virDomainNetReleaseActualDevice(vm->def, newdev);
 
     return ret;
@@ -4310,7 +4313,8 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
     virDomainHostdevDefFree(hostdev);
 
     if (net) {
-        virDomainNetReleaseActualDevice(vm->def, net);
+        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
+            virDomainNetReleaseActualDevice(vm->def, net);
         virDomainNetDefFree(net);
     }
 
@@ -4406,7 +4410,8 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
 
     qemuDomainNetDeviceVportRemove(net);
 
-    virDomainNetReleaseActualDevice(vm->def, net);
+    if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
+        virDomainNetReleaseActualDevice(vm->def, net);
     virDomainNetDefFree(net);
     ret = 0;
 
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c4e33723d1..440e2b326d 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3127,7 +3127,8 @@ qemuProcessNotifyNets(virDomainDefPtr def)
         if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT)
            ignore_value(virNetDevMacVLanReserveName(net->ifname, false));
 
-        virDomainNetNotifyActualDevice(def, net);
+        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
+            virDomainNetNotifyActualDevice(def, net);
     }
 }
 
@@ -5326,7 +5327,8 @@ qemuProcessNetworkPrepareDevices(virDomainDefPtr def)
          * network's pool of devices, or resolve bridge device name
          * to the one defined in the network definition.
          */
-        if (virDomainNetAllocateActualDevice(def, net) < 0)
+        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
+            virDomainNetAllocateActualDevice(def, net) < 0)
             goto cleanup;
 
         actualType = virDomainNetGetActualType(net);
@@ -7075,7 +7077,8 @@ void qemuProcessStop(virQEMUDriverPtr driver,
 
         /* kick the device out of the hostdev list too */
         virDomainNetRemoveHostdev(def, net);
-        virDomainNetReleaseActualDevice(vm->def, net);
+        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
+            virDomainNetReleaseActualDevice(vm->def, net);
     }
 
  retry:
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] network: restrict usage of port management APIs
Posted by Laine Stump 5 years, 7 months ago
On 08/08/2018 11:46 AM, Daniel P. Berrangé wrote:
> The port allocation APIs are currently called unconditionally for all
> types of NIC, but (mostly) only do anything for NICs with type=network.
>
> The exception is the port allocate API which does some validation even
> for NICs with type!=network. Relying on this validation is flawed,
> however, since the network driver may not even be installed, so virt
> drivers must not delegation validation to it for NICs with
> type!=network.

Although I never thought through all the minute details to the end (and
didn't need to because ,,,AllocateActualDevice() wasn't in a public
API), I had always figured that these calls into the network driver
would be where, eventually, we would do all of the plumbing for a
network device, like creating the tap/macvtap device during startup, and
disconnecting/deleting devices during domain shutdown. (it also kind of
makes sense for nwfilters to be added/removed by the network driver
during these APIs, since the filterref is in the <interface> definition).

That's the reason for the unconditional calls.

(one of the "minute details" that I hadn't thought about was the fact
that we probably can't do *all* of the plumbing at one time - at the
very least we need to have one API call for creating the devices and
hooking them up, and another call that happens right before the guest
CPUs are started (to bring everything online). Still, if we limit the
netAllocate API to only being called for type='network' then we
definitely will need an additional API that will likely be called
unconditionally just after netAllocation is called just for type='network'.)

BTW, once it is the responsibility of the network driver to create tap
devices and connect them to bridges, the network driver will no longer
be optional (unless we want to duplicate the code in the hypervisor
drivers), but that is the price we have to pay in order to give
unprivileged libvirt the ability to use any type of network device.

>
> This change allows us to report errors when the virtual network driver
> is not registered.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/conf/domain_conf.c      | 12 +++++-------
>  src/libxl/libxl_domain.c    |  6 ++++--
>  src/libxl/libxl_driver.c    |  9 ++++++---
>  src/lxc/lxc_driver.c        |  6 ++++--
>  src/lxc/lxc_process.c       | 10 ++++++++--
>  src/network/bridge_driver.c | 22 +++++++++++++++-------
>  src/qemu/qemu_hotplug.c     | 17 +++++++++++------
>  src/qemu/qemu_process.c     |  9 ++++++---
>  8 files changed, 59 insertions(+), 32 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index adcd8f41b9..e7d2acdcc9 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -29947,13 +29947,11 @@ int
>  virDomainNetAllocateActualDevice(virDomainDefPtr dom,
>                                   virDomainNetDefPtr iface)
>  {
> -    /* Just silently ignore if network driver isn't present. If something
> -     * has tried to use a NIC with type=network, other code will already
> -     * cause an error. This ensures type=bridge doesn't break when
> -     * network driver is compiled out.
> -     */
> -    if (!netAllocate)
> -        return 0;
> +    if (!netAllocate) {
> +        virReportError(VIR_ERR_NO_SUPPORT, "%s",
> +                       _("Virtual networking driver is not available"));
> +        return -1;
> +    }
>  
>      return netAllocate(dom, iface);
>  }
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 2ab78ac9a5..c78d5ee96c 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -791,7 +791,8 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
>  
>              /* cleanup actual device */
>              virDomainNetRemoveHostdev(vm->def, net);
> -            virDomainNetReleaseActualDevice(vm->def, net);
> +            if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> +                virDomainNetReleaseActualDevice(vm->def, net);
>          }
>      }
>  
> @@ -948,7 +949,8 @@ libxlNetworkPrepareDevices(virDomainDefPtr def)
>           * network's pool of devices, or resolve bridge device name
>           * to the one defined in the network definition.
>           */
> -        if (virDomainNetAllocateActualDevice(def, net) < 0)
> +        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> +            virDomainNetAllocateActualDevice(def, net) < 0)
>              return -1;
>  
>          actualType = virDomainNetGetActualType(net);
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 5a5e792957..fb5f046ade 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -3264,7 +3264,8 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver,
>       * network's pool of devices, or resolve bridge device name
>       * to the one defined in the network definition.
>       */
> -    if (virDomainNetAllocateActualDevice(vm->def, net) < 0)
> +    if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> +        virDomainNetAllocateActualDevice(vm->def, net) < 0)
>          goto cleanup;
>  
>      actualType = virDomainNetGetActualType(net);
> @@ -3314,7 +3315,8 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver,
>          vm->def->nets[vm->def->nnets++] = net;
>      } else {
>          virDomainNetRemoveHostdev(vm->def, net);
> -        virDomainNetReleaseActualDevice(vm->def, net);
> +        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> +            virDomainNetReleaseActualDevice(vm->def, net);
>      }
>      virObjectUnref(cfg);
>      return ret;
> @@ -3737,7 +3739,8 @@ libxlDomainDetachNetDevice(libxlDriverPrivatePtr driver,
>   cleanup:
>      libxl_device_nic_dispose(&nic);
>      if (!ret) {
> -        virDomainNetReleaseActualDevice(vm->def, detach);
> +        if (detach->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> +            virDomainNetReleaseActualDevice(vm->def, detach);
>          virDomainNetRemove(vm->def, detachidx);
>      }
>      virObjectUnref(cfg);
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index 8867645cdc..8729fc0174 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -3871,7 +3871,8 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn,
>       * network's pool of devices, or resolve bridge device name
>       * to the one defined in the network definition.
>       */
> -    if (virDomainNetAllocateActualDevice(vm->def, net) < 0)
> +    if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> +        virDomainNetAllocateActualDevice(vm->def, net) < 0)
>          return -1;
>  
>      actualType = virDomainNetGetActualType(net);
> @@ -4425,7 +4426,8 @@ lxcDomainDetachDeviceNetLive(virDomainObjPtr vm,
>      ret = 0;
>   cleanup:
>      if (!ret) {
> -        virDomainNetReleaseActualDevice(vm->def, detach);
> +        if (detach->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> +            virDomainNetReleaseActualDevice(vm->def, detach);
>          virDomainNetRemove(vm->def, detachidx);
>          virDomainNetDefFree(detach);
>      }
> diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
> index 33c806630b..7a6b40d9b8 100644
> --- a/src/lxc/lxc_process.c
> +++ b/src/lxc/lxc_process.c
> @@ -213,7 +213,8 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver,
>                                  iface->ifname));
>              ignore_value(virNetDevVethDelete(iface->ifname));
>          }
> -        virDomainNetReleaseActualDevice(vm->def, iface);
> +        if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> +            virDomainNetReleaseActualDevice(vm->def, iface);
>      }
>  
>      virDomainConfVMNWFilterTeardown(vm);
> @@ -547,6 +548,10 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn,
>          if (virLXCProcessValidateInterface(net) < 0)
>              goto cleanup;
>  
> +        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> +            virDomainNetAllocateActualDevice(def, net) < 0)
> +            goto cleanup;
> +
>          if (virDomainNetAllocateActualDevice(def, net) < 0)
>              goto cleanup;
>  
> @@ -626,7 +631,8 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn,
>                  ignore_value(virNetDevOpenvswitchRemovePort(
>                                  virDomainNetGetActualBridgeName(iface),
>                                  iface->ifname));
> -            virDomainNetReleaseActualDevice(def, iface);
> +            if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> +                virDomainNetReleaseActualDevice(def, iface);
>          }
>      }
>      return ret;
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index f92cc61e47..c44cb73c5b 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -4458,8 +4458,11 @@ networkAllocateActualDevice(virDomainDefPtr dom,
>      size_t i;
>      int ret = -1;
>  
> -    if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
> -        goto validate;
> +    if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Expected a interface for a virtual network"));
> +        goto error;
> +    }
>  
>      virDomainActualNetDefFree(iface->data.network.actual);
>      iface->data.network.actual = NULL;
> @@ -4778,7 +4781,6 @@ networkAllocateActualDevice(virDomainDefPtr dom,
>      if (virNetDevVPortProfileCheckComplete(virtport, true) < 0)
>          goto error;
>  
> - validate:
>      /* make sure that everything now specified for the device is
>       * actually supported on this type of network. NB: network,
>       * netdev, and iface->data.network.actual may all be NULL.
> @@ -4881,8 +4883,11 @@ networkNotifyActualDevice(virDomainDefPtr dom,
>      size_t i;
>      char *master = NULL;
>  
> -    if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
> -        return;
> +    if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Expected a interface for a virtual network"));
> +        goto error;
> +    }
>  
>      obj = virNetworkObjFindByName(driver->networks, iface->data.network.name);
>      if (!obj) {
> @@ -5114,8 +5119,11 @@ networkReleaseActualDevice(virDomainDefPtr dom,
>      size_t i;
>      int ret = -1;
>  
> -    if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
> -        return 0;
> +    if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Expected a interface for a virtual network"));
> +        goto error;
> +    }
>  
>      obj = virNetworkObjFindByName(driver->networks, iface->data.network.name);
>      if (!obj) {
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 1488f0a7c2..512fead050 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1062,7 +1062,8 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>       * network's pool of devices, or resolve bridge device name
>       * to the one defined in the network definition.
>       */
> -    if (virDomainNetAllocateActualDevice(vm->def, net) < 0)
> +    if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> +        virDomainNetAllocateActualDevice(vm->def, net) < 0)
>          goto cleanup;
>  
>      actualType = virDomainNetGetActualType(net);
> @@ -1352,7 +1353,8 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>  
>          virDomainNetRemoveHostdev(vm->def, net);
>  
> -        virDomainNetReleaseActualDevice(vm->def, net);
> +        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> +            virDomainNetReleaseActualDevice(vm->def, net);
>      }
>  
>      VIR_FREE(nicstr);
> @@ -3722,7 +3724,8 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
>  
>          /* this function doesn't work with HOSTDEV networks yet, thus
>           * no need to change the pointer in the hostdev structure */
> -        virDomainNetReleaseActualDevice(vm->def, olddev);
> +        if (olddev->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> +            virDomainNetReleaseActualDevice(vm->def, olddev);
>          virDomainNetDefFree(olddev);
>          /* move newdev into the nets list, and NULL it out from the
>           * virDomainDeviceDef that we were given so that the caller
> @@ -3753,7 +3756,7 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
>       * that the changes were minor enough that we didn't need to
>       * replace the entire device object.
>       */
> -    if (newdev)
> +    if (newdev && newdev->type == VIR_DOMAIN_NET_TYPE_NETWORK)
>          virDomainNetReleaseActualDevice(vm->def, newdev);
>  
>      return ret;
> @@ -4310,7 +4313,8 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
>      virDomainHostdevDefFree(hostdev);
>  
>      if (net) {
> -        virDomainNetReleaseActualDevice(vm->def, net);
> +        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> +            virDomainNetReleaseActualDevice(vm->def, net);
>          virDomainNetDefFree(net);
>      }
>  
> @@ -4406,7 +4410,8 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
>  
>      qemuDomainNetDeviceVportRemove(net);
>  
> -    virDomainNetReleaseActualDevice(vm->def, net);
> +    if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> +        virDomainNetReleaseActualDevice(vm->def, net);
>      virDomainNetDefFree(net);
>      ret = 0;
>  
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index c4e33723d1..440e2b326d 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3127,7 +3127,8 @@ qemuProcessNotifyNets(virDomainDefPtr def)
>          if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT)
>             ignore_value(virNetDevMacVLanReserveName(net->ifname, false));
>  
> -        virDomainNetNotifyActualDevice(def, net);
> +        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> +            virDomainNetNotifyActualDevice(def, net);
>      }
>  }
>  
> @@ -5326,7 +5327,8 @@ qemuProcessNetworkPrepareDevices(virDomainDefPtr def)
>           * network's pool of devices, or resolve bridge device name
>           * to the one defined in the network definition.
>           */
> -        if (virDomainNetAllocateActualDevice(def, net) < 0)
> +        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> +            virDomainNetAllocateActualDevice(def, net) < 0)
>              goto cleanup;
>  
>          actualType = virDomainNetGetActualType(net);
> @@ -7075,7 +7077,8 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>  
>          /* kick the device out of the hostdev list too */
>          virDomainNetRemoveHostdev(def, net);
> -        virDomainNetReleaseActualDevice(vm->def, net);
> +        if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> +            virDomainNetReleaseActualDevice(vm->def, net);
>      }
>  
>   retry:


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] network: restrict usage of port management APIs
Posted by Daniel P. Berrangé 5 years, 7 months ago
On Wed, Aug 08, 2018 at 11:41:23PM -0400, Laine Stump wrote:
> On 08/08/2018 11:46 AM, Daniel P. Berrangé wrote:
> > The port allocation APIs are currently called unconditionally for all
> > types of NIC, but (mostly) only do anything for NICs with type=network.
> >
> > The exception is the port allocate API which does some validation even
> > for NICs with type!=network. Relying on this validation is flawed,
> > however, since the network driver may not even be installed, so virt
> > drivers must not delegation validation to it for NICs with
> > type!=network.
> 
> Although I never thought through all the minute details to the end (and
> didn't need to because ,,,AllocateActualDevice() wasn't in a public
> API), I had always figured that these calls into the network driver
> would be where, eventually, we would do all of the plumbing for a
> network device, like creating the tap/macvtap device during startup, and
> disconnecting/deleting devices during domain shutdown. (it also kind of
> makes sense for nwfilters to be added/removed by the network driver
> during these APIs, since the filterref is in the <interface> definition).
> 
> That's the reason for the unconditional calls.
> 
> (one of the "minute details" that I hadn't thought about was the fact
> that we probably can't do *all* of the plumbing at one time - at the
> very least we need to have one API call for creating the devices and
> hooking them up, and another call that happens right before the guest
> CPUs are started (to bring everything online). Still, if we limit the
> netAllocate API to only being called for type='network' then we
> definitely will need an additional API that will likely be called
> unconditionally just after netAllocation is called just for type='network'.)

Yep, this is a bigger problem than I first considered. We have four
hypervisors using tap devices (qemu, lxc, libxl & bhyve) and all of
them have different requirements for the way the tap devices are
created. This is going to make it hard to delegate everything to the
network driver, as the work is hypervisor specific.

I'm trying to tackle this is distinct achieveable phases. Even the
current AllocateActualDevice() method is doing too much at the same
time. For example, it deals with allocating the resources inside the
network driver (ie reserving a VF or hostdev), as well as populating
the virDomainNetActualDef struct, and invoking hook scripts. This
makes it too tangled up with the hyervisor drivers - for example
needing a full domain XML is very undesirable.

> BTW, once it is the responsibility of the network driver to create tap
> devices and connect them to bridges, the network driver will no longer
> be optional (unless we want to duplicate the code in the hypervisor
> drivers), but that is the price we have to pay in order to give
> unprivileged libvirt the ability to use any type of network device.

Even if the network driver can create tap devices, I'd still want to
deal with type=network vs type=bridge separately for access control
purposes. It is desirable to be able to have ACLs on the opening operation
and the natural place to attach the ACL is against the virNetworkPtr
object. We have no object to represent standalone bridge devices outside
of a virNetworkPtr, so nothing to attach an ACL to for creating tap
devices.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] network: restrict usage of port management APIs
Posted by Laine Stump 5 years, 7 months ago
On 08/09/2018 03:58 AM, Daniel P. Berrangé wrote:
> On Wed, Aug 08, 2018 at 11:41:23PM -0400, Laine Stump wrote:
>> On 08/08/2018 11:46 AM, Daniel P. Berrangé wrote:
>>> The port allocation APIs are currently called unconditionally for all
>>> types of NIC, but (mostly) only do anything for NICs with type=network.
>>>
>>> The exception is the port allocate API which does some validation even
>>> for NICs with type!=network. Relying on this validation is flawed,
>>> however, since the network driver may not even be installed, so virt
>>> drivers must not delegation validation to it for NICs with
>>> type!=network.
>> Although I never thought through all the minute details to the end (and
>> didn't need to because ,,,AllocateActualDevice() wasn't in a public
>> API), I had always figured that these calls into the network driver
>> would be where, eventually, we would do all of the plumbing for a
>> network device, like creating the tap/macvtap device during startup, and
>> disconnecting/deleting devices during domain shutdown. (it also kind of
>> makes sense for nwfilters to be added/removed by the network driver
>> during these APIs, since the filterref is in the <interface> definition).
>>
>> That's the reason for the unconditional calls.
>>
>> (one of the "minute details" that I hadn't thought about was the fact
>> that we probably can't do *all* of the plumbing at one time - at the
>> very least we need to have one API call for creating the devices and
>> hooking them up, and another call that happens right before the guest
>> CPUs are started (to bring everything online). Still, if we limit the
>> netAllocate API to only being called for type='network' then we
>> definitely will need an additional API that will likely be called
>> unconditionally just after netAllocation is called just for type='network'.)
> Yep, this is a bigger problem than I first considered. We have four
> hypervisors using tap devices (qemu, lxc, libxl & bhyve) and all of
> them have different requirements for the way the tap devices are
> created. This is going to make it hard to delegate everything to the
> network driver, as the work is hypervisor specific.

The differences between qemu and lxc were on my mind but, without
looking, I had naively assumed that "all the others" (including libxl
and bhve) were just doing something similar to qemu :-/

>
> I'm trying to tackle this is distinct achieveable phases. Even the
> current AllocateActualDevice() method is doing too much at the same
> time. For example, it deals with allocating the resources inside the
> network driver (ie reserving a VF or hostdev), as well as populating
> the virDomainNetActualDef struct, and invoking hook scripts. This
> makes it too tangled up with the hyervisor drivers - for example
> needing a full domain XML is very undesirable.

I've always been hopeful that we could figure out some way to
consolidate everything needed for setup/teardown into just a few well
defined actions (to avoid an explosion of APIs that, as a side effect,
are difficult to explain). But as always, "Easy to do" is easy to say
(and again I find myself using ":-/")


Anyway, as long as I know you're considering this, I'm sure you've got a
better picture in your brain than I do, so

Reviewed-by: Laine Stump <laine@laine.org>

for the patch.

>
>> BTW, once it is the responsibility of the network driver to create tap
>> devices and connect them to bridges, the network driver will no longer
>> be optional (unless we want to duplicate the code in the hypervisor
>> drivers), but that is the price we have to pay in order to give
>> unprivileged libvirt the ability to use any type of network device.
> Even if the network driver can create tap devices, I'd still want to
> deal with type=network vs type=bridge separately for access control
> purposes. It is desirable to be able to have ACLs on the opening operation
> and the natural place to attach the ACL is against the virNetworkPtr
> object. We have no object to represent standalone bridge devices outside
> of a virNetworkPtr, so nothing to attach an ACL to for creating tap
> devices.

So are you saying that the network driver simply wouldn't be able to
create the tap devices for type=bridge (or type=direct)? I'm not exactly
following you (although I don't doubt what you say).

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] network: restrict usage of port management APIs
Posted by Daniel P. Berrangé 5 years, 7 months ago
On Thu, Aug 09, 2018 at 11:54:42AM -0400, Laine Stump wrote:
> On 08/09/2018 03:58 AM, Daniel P. Berrangé wrote:
> > On Wed, Aug 08, 2018 at 11:41:23PM -0400, Laine Stump wrote:
> >> On 08/08/2018 11:46 AM, Daniel P. Berrangé wrote:
> >>> The port allocation APIs are currently called unconditionally for all
> >>> types of NIC, but (mostly) only do anything for NICs with type=network.
> >>>
> >>> The exception is the port allocate API which does some validation even
> >>> for NICs with type!=network. Relying on this validation is flawed,
> >>> however, since the network driver may not even be installed, so virt
> >>> drivers must not delegation validation to it for NICs with
> >>> type!=network.
> >> Although I never thought through all the minute details to the end (and
> >> didn't need to because ,,,AllocateActualDevice() wasn't in a public
> >> API), I had always figured that these calls into the network driver
> >> would be where, eventually, we would do all of the plumbing for a
> >> network device, like creating the tap/macvtap device during startup, and
> >> disconnecting/deleting devices during domain shutdown. (it also kind of
> >> makes sense for nwfilters to be added/removed by the network driver
> >> during these APIs, since the filterref is in the <interface> definition).
> >>
> >> That's the reason for the unconditional calls.
> >>
> >> (one of the "minute details" that I hadn't thought about was the fact
> >> that we probably can't do *all* of the plumbing at one time - at the
> >> very least we need to have one API call for creating the devices and
> >> hooking them up, and another call that happens right before the guest
> >> CPUs are started (to bring everything online). Still, if we limit the
> >> netAllocate API to only being called for type='network' then we
> >> definitely will need an additional API that will likely be called
> >> unconditionally just after netAllocation is called just for type='network'.)
> > Yep, this is a bigger problem than I first considered. We have four
> > hypervisors using tap devices (qemu, lxc, libxl & bhyve) and all of
> > them have different requirements for the way the tap devices are
> > created. This is going to make it hard to delegate everything to the
> > network driver, as the work is hypervisor specific.
> 
> The differences between qemu and lxc were on my mind but, without
> looking, I had naively assumed that "all the others" (including libxl
> and bhve) were just doing something similar to qemu :-/

If anything libxl & bhyve are much closer to LXC in approach, in that
they want a persistent TAP device passed by name, not a transient TAP
device passed by FD.

> >> BTW, once it is the responsibility of the network driver to create tap
> >> devices and connect them to bridges, the network driver will no longer
> >> be optional (unless we want to duplicate the code in the hypervisor
> >> drivers), but that is the price we have to pay in order to give
> >> unprivileged libvirt the ability to use any type of network device.
> > Even if the network driver can create tap devices, I'd still want to
> > deal with type=network vs type=bridge separately for access control
> > purposes. It is desirable to be able to have ACLs on the opening operation
> > and the natural place to attach the ACL is against the virNetworkPtr
> > object. We have no object to represent standalone bridge devices outside
> > of a virNetworkPtr, so nothing to attach an ACL to for creating tap
> > devices.
> 
> So are you saying that the network driver simply wouldn't be able to
> create the tap devices for type=bridge (or type=direct)? I'm not exactly
> following you (although I don't doubt what you say).

That's the direction i'm leaning towards right now at least. Adding the
API is easy, I just don't know what useful security model I could give it.

Consider the original goal was to allow the unprivileged libvirtd to have
useful network connectivity for guests. This means we want unprivileged
libvirtd to be able to request a TAP device without requiring root password
auth.

I don't think it is viable to allow this for arbitrary bridge devices out
of the box.  It is, however, just about reasonable to allow unprivileged
libvirtd to request a connection to the virbr0 "default" virtual network
without root auth, as the connectivity provided by the dfault network
is quite tightly defined.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list