Re: [Xen-devel] [PATCH] libxc: fix uninitialised usage of rc in meminit_hvm

[Roger Pau Monnà <roger.pau@xxxxxxxxxx>]

El 3/2/16 a les 11:30, Ian Campbell ha escrit:
> On Tue, 2016-02-02 at 12:37 +0000, Wei Liu wrote:
>> On Tue, Feb 02, 2016 at 12:33:20PM +0100, Roger Pau Monne wrote:
>>> From: Roger Pau Monne <royger@xxxxxxxxxxx>
>>>
>>> Due to the HVMlite changes there's a chance that the value in rc is
>>> checked
>>> without being initialised. Fix this by initialising it to 0.
>>>
>>> Signed-off-by: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
>>> Reported-by: Olaf Hering <olaf@xxxxxxxxx>
>>
>> Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> 
> This is CID 1351229, I think?

Looks like, according the the description below.

> 
> ** CID 1351229:  Uninitialized variables  (UNINIT)
>> /tools/libxc/xc_dom_x86.c: 1443 in meminit_hvm()
>>  
>>  
>> ________________________________________________________________________________________________________
>> *** CID 1351229:  Uninitialized variables  (UNINIT)
>> /tools/libxc/xc_dom_x86.c: 1443 in meminit_hvm()
>> 1437                 cur_pages = 0xc0;
>> 1438                 stat_normal_pages += 0xc0;
>> 1439             }
>> 1440             else
>> 1441                 cur_pages = vmemranges[vmemid].start >> PAGE_SHIFT;
>> 1442     
>>>>>      CID 1351229:  Uninitialized variables  (UNINIT)
>>>>>      Using uninitialized value "rc".
>> 1443             while ( (rc == 0) && (end_pages > cur_pages) )
>> 1444             {
>> 1445                 /* Clip count to maximum 1GB extent. */
>> 1446                 unsigned long count = end_pages - cur_pages;
>> 1447                 unsigned long max_pages = SUPERPAGE_1GB_NR_PFNS;
>> 1448    
> 
> Note that this while loop ends with:
>         if ( rc != 0 )
>             break;
> and there are no continue statements.
> 
> Therefore I wonder if we would be better off removing the rc == 0 part of
> the loop condition?

We could, but I think we would still have the same issue with the "if (
rc != 0 )" at the end of the loop, AFAICT rc is never unconditionally
set inside of the loop itself, so gcc and coverity would still complain
about uninitialized usage.

> The issue with this patch is the usual one that it will hide other
> unintentional uses of rc before it is set to a good value.
> 
> This issue was exposed by a prior "rc = xc_domain_populate_physmap_exact"
> becoming conditional on device_model. What is also concerning is the lack
> of error checking on that call -- is it really ok to just barrel on under
> these circumstance?

Hm, I guess we then rely on the rc == 0 at the start of the while loop
in order to bail out. IMHO the logic in this function is overly complicated.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel