[PATCH] xenstat: Fix buffer over-run with new_domains being negative.

[PATCH] xenstat: Fix buffer over-run with new_domains being negative.

[Konrad Rzeszutek Wilk]

Coverity identified this as:
CID 1055740 Out-of-bounds read - "In xenstat_get_node:
Out-of-bounds read from a buffer (CWE-125)"

And sure enough, if xc_domain_getinfolist returns us -1, we will
try to use it later on in the for (i = 0; i < new_domains; ..)
loop.

CC: ian.campbell@citrix.com
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/xenstat/libxenstat/src/xenstat.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/tools/xenstat/libxenstat/src/xenstat.c b/tools/xenstat/libxenstat/src/xenstat.c
index 104655d..e5facb8 100644
--- a/tools/xenstat/libxenstat/src/xenstat.c
+++ b/tools/xenstat/libxenstat/src/xenstat.c
@@ -208,15 +208,15 @@ xenstat_node *xenstat_get_node(xenstat_handle * handle, unsigned int flags)
 						    node->num_domains, 
 						    DOMAIN_CHUNK_SIZE, 
 						    domaininfo);
+		if (new_domains < 0)
+			goto err;
 
 		tmp = realloc(node->domains,
 			      (node->num_domains + new_domains)
 			      * sizeof(xenstat_domain));
-		if (tmp == NULL) {
-			free(node->domains);
-			free(node);
-			return NULL;
-		}
+		if (tmp == NULL)
+			goto err;
+
 		node->domains = tmp;
 
 		domain = node->domains + node->num_domains;
@@ -280,6 +280,10 @@ xenstat_node *xenstat_get_node(xenstat_handle * handle, unsigned int flags)
 	}
 
 	return node;
+err:
+	free(node->domains);
+	free(node);
+	return NULL;
 }
 
 void xenstat_free_node(xenstat_node * node)
-- 
1.7.7.6

Re: [PATCH] xenstat: Fix buffer over-run with new_domains being negative.

[Andrew Cooper]

On 10/09/13 16:08, Konrad Rzeszutek Wilk wrote:
> Coverity identified this as:
> CID 1055740 Out-of-bounds read - "In xenstat_get_node:
> Out-of-bounds read from a buffer (CWE-125)"
>
> And sure enough, if xc_domain_getinfolist returns us -1, we will
> try to use it later on in the for (i = 0; i < new_domains; ..)
> loop.
>
> CC: ian.campbell@citrix.com
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  tools/xenstat/libxenstat/src/xenstat.c |   14 +++++++++-----
>  1 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/tools/xenstat/libxenstat/src/xenstat.c b/tools/xenstat/libxenstat/src/xenstat.c
> index 104655d..e5facb8 100644
> --- a/tools/xenstat/libxenstat/src/xenstat.c
> +++ b/tools/xenstat/libxenstat/src/xenstat.c
> @@ -208,15 +208,15 @@ xenstat_node *xenstat_get_node(xenstat_handle * handle, unsigned int flags)
>  						    node->num_domains, 
>  						    DOMAIN_CHUNK_SIZE, 
>  						    domaininfo);
> +		if (new_domains < 0)
> +			goto err;
>  
>  		tmp = realloc(node->domains,
>  			      (node->num_domains + new_domains)
>  			      * sizeof(xenstat_domain));
> -		if (tmp == NULL) {
> -			free(node->domains);
> -			free(node);
> -			return NULL;
> -		}
> +		if (tmp == NULL)
> +			goto err;
> +
>  		node->domains = tmp;
>  
>  		domain = node->domains + node->num_domains;
> @@ -280,6 +280,10 @@ xenstat_node *xenstat_get_node(xenstat_handle * handle, unsigned int flags)
>  	}
>  
>  	return node;
> +err:
> +	free(node->domains);
> +	free(node);
> +	return NULL;
>  }
>  
>  void xenstat_free_node(xenstat_node * node)

Re: [PATCH] xenstat: Fix buffer over-run with new_domains being negative.

[Ian Campbell]

On Tue, 2013-09-10 at 17:10 +0100, Andrew Cooper wrote:
> On 10/09/13 16:08, Konrad Rzeszutek Wilk wrote:
> > Coverity identified this as:
> > CID 1055740 Out-of-bounds read - "In xenstat_get_node:
> > Out-of-bounds read from a buffer (CWE-125)"
> >
> > And sure enough, if xc_domain_getinfolist returns us -1, we will
> > try to use it later on in the for (i = 0; i < new_domains; ..)
> > loop.
> >
> > CC: ian.campbell@citrix.com
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Applied.