From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH]: Fix memory corruption in-kernel IOAPIC emulation Date: Thu, 31 Jan 2008 09:24:46 +0200 Message-ID: <47A177BE.6020300@qumranet.com> References: <479FB5C6.6060204@redhat.com> <47A0E613.7080408@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org To: Chris Lalancette Return-path: In-Reply-To: <47A0E613.7080408-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.org Chris Lalancette wrote: > Another version of the patch, done by changing the on-the-wire protocol > as Avi suggested. I've tested this with: > > old -> old - Migration works, but runs into the bug I'm trying to fix > old -> new - Migration works, but runs into the bug I'm trying to fix > new -> old - Migration fails gracefully with ioapic version mismatch > new -> new - Migration works, and doesn't run into this particular bug > > > -static void kvm_kernel_ioapic_load_from_user(IOAPICState *s) > +static int kvm_kernel_ioapic_load_from_user(IOAPICState *s, int version_id) > { > #if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386) > struct kvm_irqchip chip; > struct kvm_ioapic_state *kioapic; > int i; > > + if (version_id > 2) { > + return -EINVAL; > + } > + > chip.chip_id = KVM_IRQCHIP_IOAPIC; > kioapic = &chip.chip.ioapic; > > + if (version_id == 2) { > + kioapic->base_address = s->base_address; > + kioapic->irr = s->irr; > + } > + > Preferably this function would always set all variables; the caller should make sure all the values are sane (by initializing them to sane values on ioapic creation, and not modifying them when a version 1 migration happens). It's best to limit protocol change handling to the actual qemu save/restore routines. > static void ioapic_save(QEMUFile *f, void *opaque) > @@ -1173,8 +1188,9 @@ static int ioapic_load(QEMUFile *f, void *opaque, int version_id) > { > IOAPICState *s = opaque; > int i; > + int ret; > > - if (version_id != 1) > + if (version_id > 2) > return -EINVAL; > This allows version_id == 0 to sneak in, where it couldn't before. It's not a problem in practice, but let's get it right. -- error compiling committee.c: too many arguments to function ------------------------------------------------------------------------- This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/