Re: [PATCH 5/5] xenstat: Fix buffer over-run with new_domains being negative (again).

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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 <konrad.wilk@oracle.com>
> >> ---
> >>  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.