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.