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

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.