[libvirt] [PATCHv2 2/5] domifaddr: Implement the remote protocol
Osier Yang
jyang at redhat.com
Sun Aug 18 07:33:16 UTC 2013
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.
More information about the libvir-list
mailing list