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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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?
>
> 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