From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 5/5] xenstat: Fix buffer over-run with new_domains being negative (again). Date: Mon, 25 Nov 2013 17:36:29 +0000 Message-ID: <52938A9D.8060706@citrix.com> References: <1385398842-8247-1-git-send-email-konrad.wilk@oracle.com> <1385398842-8247-6-git-send-email-konrad.wilk@oracle.com> <1385399207.22002.100.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Vl0FH-0005bG-Qn for xen-devel@lists.xenproject.org; Mon, 25 Nov 2013 17:46:52 +0000 In-Reply-To: <1385399207.22002.100.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On 25/11/13 17:06, Ian Campbell wrote: > On Mon, 2013-11-25 at 12:00 -0500, Konrad Rzeszutek Wilk wrote: >> The git commit 1438d36f96e90d1116bebc6b3013634ca21c49c8 >> "xenstat: Fix buffer over-run with new_domains being negative." >> fixed part of the problem, but it failed to do one more check. >> >> That is the if we don't exit out of the loop if the >> xc_domain_getinfolist returns -1. This makes the check >> by casting the unsigned int value to int as otherwise >> the >> if (new_domains < 0) >> >> would never get executed (as unsigned int won't be negative). >> >> Fixes CID 1055740. >> >> CC: andrew.cooper3@citrix.com >> Signed-off-by: Konrad Rzeszutek Wilk >> --- >> tools/xenstat/libxenstat/src/xenstat.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tools/xenstat/libxenstat/src/xenstat.c b/tools/xenstat/libxenstat/src/xenstat.c >> index e5facb8..27d8e22 100644 >> --- a/tools/xenstat/libxenstat/src/xenstat.c >> +++ b/tools/xenstat/libxenstat/src/xenstat.c >> @@ -208,7 +208,7 @@ xenstat_node *xenstat_get_node(xenstat_handle * handle, unsigned int flags) >> node->num_domains, >> DOMAIN_CHUNK_SIZE, >> domaininfo); >> - if (new_domains < 0) >> + if ((int)new_domains < 0) > I expect that the right fix is to make new_domains the correct (signed) > type, no? > > Ian. > xc_domain_getinfolist() is just as crazy as its partner in crime, xc_domain_getinfo(), and takes an unsigned int "max_doms" and returns a signed number of domains gotten, or -1 for certain failed hypercalls. I might be inclined to introduce a signed rc to hold the return value xc_domain_getinfolist(), check for a negative return before assigning to new_domains and also check for an overflow of node->num_domains + new_domains for the realloc. ~Andrew