Re: [Qemu-devel] [PATCH] ps2: fix PS2Queue counter field type

[Daniel P. Berrange]

On Wed, Nov 15, 2017 at 06:16:02PM +0530, P J P wrote:
> From: Prasad J Pandit <address@hidden>
> 
> During Qemu guest migration, a destination process invokes ps2
> post_load function. In that, if 'rptr' and 'count' fields were
> tampered, it could lead to OOB access or infinite loop issues.
> Change their type to 'uint8_t' so they remain within bounds.

If you're concerned that someone is tampering with QEMU state
in transit during migration, then you're going to end up playing
whack-a-mole across the entire QEMU codebase IMHO. The answer
to the problem of tampering is to have encryption of the
migration data stream between both QEMU's. Thus QEMU on the
target merely has to trust QEMU on the source. If QEMU on the
source is itself compromised you've already lost and migration
won't make life any worse.

> 
> Reported-by: Cyrille Chatras <address@hidden>
> Signed-off-by: Prasad J Pandit <address@hidden>
> ---
>  hw/input/ps2.c | 26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/input/ps2.c b/hw/input/ps2.c
> index f388a23c8e..ce4b167084 100644
> --- a/hw/input/ps2.c
> +++ b/hw/input/ps2.c
> @@ -90,7 +90,7 @@ typedef struct {
>      /* Keep the data array 256 bytes long, which compatibility
>       with older qemu versions. */
>      uint8_t data[256];
> -    int rptr, wptr, count;
> +    uint8_t rptr, wptr, count;
>  } PS2Queue;
>  
>  struct PS2State {
> @@ -1225,24 +1225,18 @@ 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];
>  
>      /* set the useful data buffer queue size, < PS2_QUEUE_SIZE */
>      size = q->count > PS2_QUEUE_SIZE ? 0 : q->count;
>  
>      /* move the queue elements to the start of data array */
> -    if (size > 0) {
> -        for (i = 0; i < size; i++) {
> -            /* move the queue elements to the temporary buffer */
> -            tmp_data[i] = q->data[q->rptr];
> -            if (++q->rptr == 256) {
> -                q->rptr = 0;
> -            }
> -        }
> -        memcpy(q->data, tmp_data, size);
> +    for (i = 0; i < size; i++) {
> +        tmp_data[i] = q->data[q->rptr++];
>      }
> +    memcpy(q->data, tmp_data, size);
> +
>      /* reset rptr/wptr/count */
>      q->rptr = 0;
>      q->wptr = size;
> @@ -1286,9 +1280,9 @@ static const VMStateDescription vmstate_ps2_common = {
>      .minimum_version_id = 2,
>      .fields = (VMStateField[]) {
>          VMSTATE_INT32(write_cmd, PS2State),
> -        VMSTATE_INT32(queue.rptr, PS2State),
> -        VMSTATE_INT32(queue.wptr, PS2State),
> -        VMSTATE_INT32(queue.count, PS2State),
> +        VMSTATE_UINT8(queue.rptr, PS2State),
> +        VMSTATE_UINT8(queue.wptr, PS2State),
> +        VMSTATE_UINT8(queue.count, PS2State),

This would surely break migration backwards compatibility. Any new QEMU
release accepting incoming migration from an old QEMU release will read
the wrong amount of data off the migration stream, and thus will ultimately
cause migraiton to fail.

>          VMSTATE_BUFFER(queue.data, PS2State),
>          VMSTATE_END_OF_LIST()
>      }

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|