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

Re: [libvirt] [PATCHv2 2/5] domifaddr: Implement the remote protocol



On 15/08/13 17:36, Daniel P. Berrange wrote:
On Thu, Aug 15, 2013 at 02:24:27AM +0530, nehaljwani wrote:
Implement RPC calls for virDomainInterfacesAddresses

daemon/remote.c
    * Define remoteSerializeDomainInterfacePtr, remoteDispatchDomainInterfacesAddresses

src/remote/remote_driver.c
    * Define remoteDomainInterfacesAddresses

src/remote/remote_protocol.x
    * New RPC procedure: REMOTE_PROC_DOMAIN_INTERFACES_ADDRESSES
    * Define structs remote_domain_ip_addr, remote_domain_interface,
      remote_domain_interfaces_addresses_args, remote_domain_interfaces_addresses_ret

src/remote_protocol-structs
    * New structs added

---
  daemon/remote.c              | 122 +++++++++++++++++++++++++++++++++++++++++++
  src/remote/remote_driver.c   |  88 +++++++++++++++++++++++++++++++
  src/remote/remote_protocol.x |  30 ++++++++++-
  src/remote_protocol-structs  |  24 +++++++++
  4 files changed, 263 insertions(+), 1 deletion(-)

diff --git a/daemon/remote.c b/daemon/remote.c
index 03d5557..9d8651c 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -5025,7 +5025,129 @@ cleanup:
      return rv;
  }
+static int
+remoteSerializeDomainInterfacePtr(virDomainInterfacePtr ifaces,
+                                  unsigned int ifaces_count,
+                                  remote_domain_interfaces_addresses_ret *ret)
+{
+    size_t i, j;
+
+    if (VIR_ALLOC_N(ret->ifaces.ifaces_val, ifaces_count) < 0)
+        return -1;
You *must* check upper bound of ifaces_count before allocating memory - this
value is untrusted data.

+
+    ret->ifaces.ifaces_len = ifaces_count;
+
+    for (i = 0; i < ifaces_count; i++) {
+        virDomainInterfacePtr iface = &(ifaces[i]);
+        remote_domain_interface *iface_ret = &(ret->ifaces.ifaces_val[i]);
+
+        if ((VIR_STRDUP(iface_ret->name, iface->name)) < 0)
+            goto no_memory;
+
+        if (iface->hwaddr) {
+            char **hwaddr_p = NULL;
+            if (VIR_ALLOC(hwaddr_p) < 0)
+                goto no_memory;
+            if (VIR_STRDUP(*hwaddr_p, iface->hwaddr) < 0) {
+                VIR_FREE(hwaddr_p);
+                goto no_memory;
+            }
+
+            iface_ret->hwaddr = hwaddr_p;
+        }
+
+        if (VIR_ALLOC_N(iface_ret->ip_addrs.ip_addrs_val,
+                        iface->ip_addrs_count) < 0)
+            goto no_memory;
Again  ip_addrs_count is untrusted data so must have an upper
bound checked.

+
+        iface_ret->ip_addrs.ip_addrs_len = iface->ip_addrs_count;
+
+        for (j = 0; j < iface->ip_addrs_count; j++) {
+            virDomainIPAddressPtr ip_addr = &(iface->ip_addrs[j]);
+            remote_domain_ip_addr *ip_addr_ret =
+                &(iface_ret->ip_addrs.ip_addrs_val[j]);
+
+            if (VIR_STRDUP(ip_addr_ret->addr, ip_addr->addr) < 0)
+                goto no_memory;
+
+            ip_addr_ret->prefix = ip_addr->prefix;
+            ip_addr_ret->type = ip_addr->type;
+        }
+    }
+
+    return 0;
+
+no_memory:
+    if (ret->ifaces.ifaces_val) {
+        for (i = 0; i < ifaces_count; i++) {
+            remote_domain_interface *iface_ret = &(ret->ifaces.ifaces_val[i]);
+            VIR_FREE(iface_ret->name);
+            VIR_FREE(iface_ret->hwaddr);
+            for (j = 0; j < iface_ret->ip_addrs.ip_addrs_len; j++) {
+                remote_domain_ip_addr *ip_addr =
+                    &(iface_ret->ip_addrs.ip_addrs_val[j]);
+                VIR_FREE(ip_addr->addr);
+            }
+            VIR_FREE(iface_ret);
+        }
+        VIR_FREE(ret->ifaces.ifaces_val);
+    }
+ return -1;
+}
+
+static int
+remoteDispatchDomainInterfacesAddresses(
+    virNetServerPtr server ATTRIBUTE_UNUSED,
+    virNetServerClientPtr client,
+    virNetMessagePtr msg ATTRIBUTE_UNUSED,
+    virNetMessageErrorPtr rerr,
+    remote_domain_interfaces_addresses_args *args,
+    remote_domain_interfaces_addresses_ret *ret)
+{
+    int rv = -1;
+    virDomainPtr dom = NULL;
+    virDomainInterfacePtr ifaces = NULL;
+    unsigned int ifaces_count = 0;
+    struct daemonClientPrivate *priv =
+        virNetServerClientGetPrivateData(client);
+
+    if (!priv->conn) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
+        goto cleanup;
+    }
+
+    if (!(dom = get_nonnull_domain(priv->conn, args->dom)))
+        goto cleanup;
+
+    if (virDomainInterfacesAddresses(dom, &ifaces,
+                                     &ifaces_count, args->flags) < 0)
+        goto cleanup;
+
+    if (remoteSerializeDomainInterfacePtr(ifaces, ifaces_count, ret) < 0)
+        goto cleanup;
+
+    rv = 0;
+
+cleanup:
+    if (rv < 0)
+        virNetMessageSaveError(rerr);
+    if (dom)
+        virDomainFree(dom);
+    if (ifaces) {
+        size_t i, j;
+
+        for (i = 0; i < ifaces_count; i++) {
+            VIR_FREE(ifaces[i].name);
+            VIR_FREE(ifaces[i].hwaddr);
+            for (j = 0; j < ifaces[i].ip_addrs_count; j++)
+                VIR_FREE(ifaces[i].ip_addrs[j].addr);
+            VIR_FREE(ifaces[i].ip_addrs);
+        }
+        VIR_FREE(ifaces);
+    }
+    return rv;
+}
/*----- Helpers. -----*/ +cleanup:
+    if (rv < 0) {
+        for (i = 0; i < *ifaces_count; i++) {
+            VIR_FREE((*ifaces)[i].name);
+            VIR_FREE((*ifaces)[i].hwaddr);
+            for (j = 0; j < (*ifaces)[i].ip_addrs_count; j++)
+                VIR_FREE((*ifaces)[i].ip_addrs[j].addr);
+            VIR_FREE((*ifaces)[i].ip_addrs);
+        }
+        VIR_FREE(*ifaces);
This horrible code is repeated soo many times - just reinforces my
point on the previous patch about providing a function to hide this.

+    }
+    xdr_free((xdrproc_t)xdr_remote_domain_interfaces_addresses_ret,
+             (char *) &ret);
+done:
+    remoteDriverUnlock(priv);
+    return rv;
+}
+
  static void
  remoteDomainEventQueue(struct private_data *priv, virDomainEventPtr event)
  {
@@ -6811,6 +6898,7 @@ static virDriver remote_driver = {
      .domainMigratePerform3Params = remoteDomainMigratePerform3Params, /* 1.1.0 */
      .domainMigrateFinish3Params = remoteDomainMigrateFinish3Params, /* 1.1.0 */
      .domainMigrateConfirm3Params = remoteDomainMigrateConfirm3Params, /* 1.1.0 */
+    .domainInterfacesAddresses = remoteDomainInterfacesAddresses, /* 1.1.2 */
  };
static virNetworkDriver network_driver = {
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index 7cfebdf..06929e7 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -2837,6 +2837,27 @@ struct remote_domain_event_device_removed_msg {
      remote_nonnull_string devAlias;
  };
+struct remote_domain_ip_addr {
+    int type;
+    remote_nonnull_string addr;
+    int prefix;
+};
+
+struct remote_domain_interface {
+    remote_nonnull_string name;
+    remote_string hwaddr;
+    remote_domain_ip_addr ip_addrs<>;
Use of <> *NOT* allowed - this is a security flaw allowing the client
to trigger DOS on libvirtd allocating memory. Follow the examples of
other APis which set an explicit limit.	

In that case, we have bug on APIs like listAllDomains too, as they use
variable-length array too.


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