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