From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52154) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eH8RZ-0008Rx-Cp for qemu-devel@nongnu.org; Tue, 21 Nov 2017 08:18:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eH8RU-0003bz-Ep for qemu-devel@nongnu.org; Tue, 21 Nov 2017 08:18:29 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55460) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eH8RU-0003bg-8v for qemu-devel@nongnu.org; Tue, 21 Nov 2017 08:18:24 -0500 Date: Tue, 21 Nov 2017 13:18:13 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20171121131812.GC2434@work-vm> References: <20171116075155.22378-1-ppandit@redhat.com> <658ecab6-1f09-0c94-c89e-55f71c37309a@redhat.com> <20171117093556.ntseydf7eiboeyww@sirius.home.kraxel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171117093556.ntseydf7eiboeyww@sirius.home.kraxel.org> Subject: Re: [Qemu-devel] [PATCH v1] ps2: check PS2Queue pointers in post_load routine List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann Cc: Paolo Bonzini , P J P , Qemu Developers , "Daniel P . Berrange" , Cyrille Chatras , Prasad J Pandit * Gerd Hoffmann (kraxel@redhat.com) wrote: > > > diff --git a/hw/input/ps2.c b/hw/input/ps2.c > > > index f388a23c8e..de171a28dd 100644 > > > --- a/hw/input/ps2.c > > > +++ b/hw/input/ps2.c > > > @@ -1225,24 +1225,21 @@ static void ps2_common_reset(PS2State *s) > > > static void ps2_common_post_load(PS2State *s) > > > { > > > PS2Queue *q = &s->queue; > > > - int size; > > > - int i; > > > - int tmp_data[PS2_QUEUE_SIZE]; > > > + uint8_t i, size; > > > + uint8_t tmp_data[PS2_QUEUE_SIZE]; > > > > Hi Prasad, > > > > you don't need to change the invalid values to sane ones. Instead, make > > ps2_common_post_load return an int (just like the .post_load member of > > VMStateDescription). You can then detect out of range count/rptr/wptr > > and return -1 for bad indices. > > Well, it's not that simple because older qemu versions had larger > queues. So post_load accepts migrations with queues which are too big. > It just clears the queue in that case, so the input events are dropped > in that (highly unlikely) case. > > Also note that the current post_load is broken. tmp_data is int whereas > q->data is uint8_t. So ... > > > > - memcpy(q->data, tmp_data, size); > > ... this memcpy doesn't work as intended. Nobody noticed in years. Oops! > v1 fixes this bug, v2 doesn't. > > So, I'm tempted to just rip out the whole backward compatibility buffer > shuffling logic. Or take v1 of this patch. I think v1 is OK. Dave > cheers, > Gerd > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK