From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] x86/hvm: Fix use-after-free introduced by c/s 428607a Date: Tue, 2 Feb 2016 10:04:22 +0000 Message-ID: <56B07F26.3030204@citrix.com> References: <1454349419-18430-1-git-send-email-andrew.cooper3@citrix.com> <56B06214.2060306@bitdefender.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56B06214.2060306@bitdefender.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: Corneliu ZUZU , Xen-devel Cc: Jan Beulich List-Id: xen-devel@lists.xenproject.org On 02/02/16 08:00, Corneliu ZUZU wrote: > On 2/1/2016 7:56 PM, Andrew Cooper wrote: >> c/s 428607a "x86: shrink 'struct domain', was already PAGE_SIZE" >> introduced a >> use-after-free error during domain destruction, because of the order >> in which >> timers are torn down. >> >> (XEN) Xen call trace: >> (XEN) [] spinlock.c#check_lock+0x1e/0x40 >> (XEN) [] _spin_lock+0x11/0x52 >> (XEN) [] vpt.c#pt_lock+0x24/0x40 >> (XEN) [] destroy_periodic_time+0x18/0x81 >> (XEN) [] rtc_deinit+0x53/0x78 >> (XEN) [] hvm_domain_destroy+0x52/0x69 >> (XEN) [] arch_domain_destroy+0x1a/0x98 >> (XEN) [] >> domain.c#complete_domain_destroy+0x6f/0x182 >> (XEN) [] >> rcupdate.c#rcu_process_callbacks+0x144/0x1a6 >> (XEN) [] softirq.c#__do_softirq+0x82/0x8d >> (XEN) [] do_softirq+0x13/0x15 >> (XEN) [] entry.o#process_softirqs+0x21/0x30 >> (XEN) >> (XEN) >> (XEN) **************************************** >> (XEN) Panic on CPU 3: >> (XEN) GENERAL PROTECTION FAULT >> (XEN) [error_code=0000] >> (XEN) **************************************** >> >> Defer the freeing of d->arch.hvm_domain.pl_time until all timers have >> been >> destroyed. >> >> For safety, NULL out the pointers after freeing them, in an attempt >> to make >> mistakes more obvious in the future. >> >> Signed-off-by: Andrew Cooper >> --- >> CC: Jan Beulich >> CC: Corneliu ZUZU >> --- >> xen/arch/x86/hvm/hvm.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c >> index f24400d..38c65b3 100644 >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -1674,8 +1674,10 @@ void hvm_domain_relinquish_resources(struct >> domain *d) >> void hvm_domain_destroy(struct domain *d) >> { >> xfree(d->arch.hvm_domain.io_handler); >> + d->arch.hvm_domain.io_handler = NULL; >> + >> xfree(d->arch.hvm_domain.params); >> - xfree(d->arch.hvm_domain.pl_time); >> + d->arch.hvm_domain.params = NULL; >> hvm_destroy_cacheattr_region_list(d); >> @@ -1686,6 +1688,9 @@ void hvm_domain_destroy(struct domain *d) >> rtc_deinit(d); >> stdvga_deinit(d); >> vioapic_deinit(d); >> + >> + xfree(d->arch.hvm_domain.pl_time); >> + d->arch.hvm_domain.pl_time = NULL; >> } >> static int hvm_save_tsc_adjust(struct domain *d, >> hvm_domain_context_t *h) > > Ups, really sorry, ashamed to say I've done the mistake of not > actually testing this on a machine (working on ARM here). Won't happen > again, if I'm to send another patch I'll be sure to actually setup an > X86 Dom0 & at least do some basic tests. At least a boot test would certainly be nice. In this case however, the build passed all but a single one of the XenServer sanity tests, and even the failure here was just chance that the region got reused in a way which would be noticed. This is, after all, precisely the reason why development branches are not generally known to be stable. > Regarding the "set to NULL after free" practice, wouldn't it be wise > to define an xfreeandnull(void**) macro and use the "unsafe" xfree > only when it makes sense to (proly most of the time it won't)? This idea has been suggested in the past. I can't remember what became of it. ~Andrew