Re: [PATCH 1/1] x86/hvm: prevent hvm_free_ioreq_gmfn() clobber of arbitrary memory

[Don Slutz <dslutz@verizon.com>]

On 04/14/15 09:17, Andrew Cooper wrote:
> On 14/04/15 12:47, Jan Beulich wrote:
>>>>> On 13.04.15 at 18:01, <dslutz@verizon.com> wrote:
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -536,8 +536,9 @@ static int hvm_alloc_ioreq_gmfn(struct domain *d, 
>>> unsigned long *gmfn)
>>>  
>>>  static void hvm_free_ioreq_gmfn(struct domain *d, unsigned long gmfn)
>>>  {
>>> -    unsigned int i = gmfn - d->arch.hvm_domain.ioreq_gmfn.base;
>>> +    unsigned long i = gmfn - d->arch.hvm_domain.ioreq_gmfn.base;
>>>  
>>> +    BUG_ON(i >= sizeof(d->arch.hvm_domain.ioreq_gmfn.mask) * 8);
>>>      clear_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
>>>  }
>> I'd be happier with an ASSERT() - Andrew?
> 
> If I recall, this is a follow on from the compiler error, where gmfn now
> gets initialised to ~0 to avoid a build failure.
> 
> If gcc is correct and there is a way for gmfn to be used, then the
> clear_bit() here clobber memory.  The BUG_ON() serves as a protection
> against the clobbering.
> 
> If however gcc was actually wrong, then the code here is actually fine,
> and a BUG_ON() or ASSERT() will never actually trigger.
> 
> In addition, not a hotpath in the slightest, so performance isn't a concern.
> 
> 
> I have still not managed to conclusively work out whether gcc is correct
> or wrong.  As a result, I would lean in the direction of BUG_ON() rather
> than ASSERT(), out of paranoia.  However, I would prefer even more a
> solution where we were certain that gmfn isn't bogus.
> 

The best I can do is to write out the code paths that I can see.  This
is all taken from RELEASE-4.5.0


There are 4 calls to hvm_free_ioreq_gmfn:

1 x86/hvm/hvm.c hvm_ioreq_server_map_pages   764 hvm_free_ioreq_gmfn(d,
bufioreq_pfn);
2 x86/hvm/hvm.c hvm_ioreq_server_map_pages   768 hvm_free_ioreq_gmfn(d,
ioreq_pfn);
3 x86/hvm/hvm.c hvm_ioreq_server_unmap_pages 788 hvm_free_ioreq_gmfn(d,
s->bufioreq.gmfn);
4 x86/hvm/hvm.c hvm_ioreq_server_unmap_pages 790 hvm_free_ioreq_gmfn(d,
s->ioreq.gmfn);



GCC only reported about bufioreq_pfn:

hvm.c: In function ‘hvm_create_ioreq_server’:
hvm.c:487:18: error: ‘bufioreq_pfn’ may be used uninitialised in this
function


Which is the line:

    unsigned int i = gmfn - d->arch.hvm_domain.ioreq_gmfn.base;

And gcc has changed the arg 'gmfn' into 'bufioreq_pfn' so this should
only apply to #1.

Now for #1, the args to hvm_ioreq_server_map_pages() 'is_default' and
'handle_bufioreq' are important.  The call that GCC is reporting on
(with context):

--------------------------------------------------------------------------
    return 0;

fail4:
    hvm_unmap_ioreq_page(s, 0);

fail3:
    if ( !is_default && handle_bufioreq )
        hvm_free_ioreq_gmfn(d, bufioreq_pfn);
--------------------------------------------------------------------------


So only the case:

'is_default' === 0, 'handle_bufioreq' === 1 needs to be checked for
all goto's of fail4 or fail3.

Now the call to hvm_alloc_ioreq_gmfn(d, &bufioreq_pfn) does have a
path where bufioreq_pfn is not set, however it will return -ENOMEM
for this case.  And so should always goto fail2:

---------------------------------------------------------------------
        if ( handle_bufioreq )
        {
            rc = hvm_alloc_ioreq_gmfn(d, &bufioreq_pfn);
            if ( rc )
                goto fail2;
        }
    }

    rc = hvm_map_ioreq_page(s, 0, ioreq_pfn);
    if ( rc )
        goto fail3;
---------------------------------------------------------------------

Since both share 'handle_bufioreq', the only time I can see
'handle_bufioreq' not uninitialised is when we goto fail2.

The ways to fail3 & fail4:
---------------------------------------------------------------------
    rc = hvm_map_ioreq_page(s, 0, ioreq_pfn);
    if ( rc )
        goto fail3;

    if ( handle_bufioreq )
    {
        rc = hvm_map_ioreq_page(s, 1, bufioreq_pfn);
        if ( rc )
            goto fail4;
    }

    return 0;
--------------------------------------------------------------------

are all after the goto for fail2.  So I see hvm_alloc_ioreq_gmfn()
setting 'bufioreq_pfn' in all cases where it is possible to get to
fail3.


Hope this helps.

    -Don Slutz


> ~Andrew
>