From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH 1/1] x86/hvm: prevent hvm_free_ioreq_gmfn() clobber of arbitrary memory Date: Thu, 16 Apr 2015 12:32:44 +0100 Message-ID: <552FB9FC0200007800072BCD@mail.emea.novell.com> References: <1428940903-18302-1-git-send-email-dslutz@verizon.com> <552D1A820200007800071DF7@mail.emea.novell.com> <552D1375.8050204@citrix.com> <20150416105312.GE13443@deinos.phlegethon.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150416105312.GE13443@deinos.phlegethon.org> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tim Deegan Cc: Andrew Cooper , Keir Fraser , Don Slutz , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org >>> On 16.04.15 at 12:53, 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, 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