Re: [Qemu-devel] [PATCH v1] ps2: check PS2Queue pointers in post_load routine

[Gerd Hoffmann <kraxel@redhat.com>]

> > 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.

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.

cheers,
  Gerd