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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 16.04.15 at 12:53, <tim@xen.org> wrote:
> At 14:17 +0100 on 14 Apr (1429021061), 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.
> 
> AFAICT GCC is wrong, though the code is confusing enough to me that I
> don't blame GCC for being confused too. :)
> 
> I would be inclined to use a bigger hammer here.  IMO refactoring like
> this makes it easier to reason about (compile tested only):

This looks like a pretty nice cleanup; I particularly like the 4 labels
going away.

Jan