From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH 5/5] xenstat: Fix buffer over-run with new_domains being negative (again). Date: Mon, 25 Nov 2013 14:51:23 -0500 Message-ID: <20131125195123.GC3339@phenom.dumpdata.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> <52938A9D.8060706@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Vl2Bw-00081l-ER for xen-devel@lists.xenproject.org; Mon, 25 Nov 2013 19:51:32 +0000 Content-Disposition: inline In-Reply-To: <52938A9D.8060706@citrix.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: Andrew Cooper Cc: xen-devel@lists.xenproject.org, Ian Campbell List-Id: xen-devel@lists.xenproject.org On Mon, Nov 25, 2013 at 05:36:29PM +0000, Andrew Cooper wrote: > 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? That can be done too. > > > > 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. Oh, hadn't thought about the overflow.