From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH 1/1] x86/hvm: prevent hvm_free_ioreq_gmfn() clobber of arbitrary memory Date: Tue, 14 Apr 2015 18:54:28 -0400 Message-ID: <552D9AA4.8070806@one.verizon.com> References: <1428940903-18302-1-git-send-email-dslutz@verizon.com> <552D1A820200007800071DF7@mail.emea.novell.com> <552D1375.8050204@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <552D1375.8050204@citrix.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: Andrew Cooper , Jan Beulich Cc: Keir Fraser , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org 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, 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 =3D gmfn - d->arch.hvm_domain.ioreq_gmfn.base; >>> + unsigned long i =3D gmfn - d->arch.hvm_domain.ioreq_gmfn.base; >>> = >>> + BUG_ON(i >=3D 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 conce= rn. > = > = > 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 =91hvm_create_ioreq_server=92: hvm.c:487:18: error: =91bufioreq_pfn=92 may be used uninitialised in this function Which is the line: unsigned int i =3D 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' =3D=3D=3D 0, 'handle_bufioreq' =3D=3D=3D 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 =3D hvm_alloc_ioreq_gmfn(d, &bufioreq_pfn); if ( rc ) goto fail2; } } rc =3D 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 =3D hvm_map_ioreq_page(s, 0, ioreq_pfn); if ( rc ) goto fail3; if ( handle_bufioreq ) { rc =3D 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 > =