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

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

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.

~Andrew