From: Prasad J Pandit <pjp@fedoraproject.org>

During Qemu guest migration, a destination process invokes ps2
post_load function. In that, if 'rptr' and 'count' values were
invalid, it could lead to OOB access or infinite loop issue.
Add check to avoid it.

Reported-by: Cyrille Chatras <cyrille.chatras@orange.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/input/ps2.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

Update: avoid changing field type, add range check on values
  -> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg02815.html

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];
 
     /* set the useful data buffer queue size, < PS2_QUEUE_SIZE */
-    size = q->count > PS2_QUEUE_SIZE ? 0 : q->count;
+    size = (q->count < 0 || 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;
-            }
+    for (i = 0; i < size; i++) {
+        if (q->rptr < 0 || q->rptr >= sizeof(q->data)) {
+            q->rptr = 0;
         }
-        memcpy(q->data, tmp_data, size);
+        tmp_data[i] = q->data[q->rptr++];
     }
+    memcpy(q->data, tmp_data, size);
+
     /* reset rptr/wptr/count */
     q->rptr = 0;
     q->wptr = size;
-- 
2.13.6

On 16/11/2017 08:51, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> During Qemu guest migration, a destination process invokes ps2
> post_load function. In that, if 'rptr' and 'count' values were
> invalid, it could lead to OOB access or infinite loop issue.
> Add check to avoid it.
> 
> Reported-by: Cyrille Chatras <cyrille.chatras@orange.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/input/ps2.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> Update: avoid changing field type, add range check on values
>   -> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg02815.html
> 
> 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.

In the callers,

	ps2_common_post_load(ps2);

	return 0;

can then become simply

	return ps2_common_post_load(ps2);

Thanks,

Paolo

>      /* set the useful data buffer queue size, < PS2_QUEUE_SIZE */
> -    size = q->count > PS2_QUEUE_SIZE ? 0 : q->count;
> +    size = (q->count < 0 || 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;
> -            }
> +    for (i = 0; i < size; i++) {
> +        if (q->rptr < 0 || q->rptr >= sizeof(q->data)) {
> +            q->rptr = 0;
>          }
> -        memcpy(q->data, tmp_data, size);
> +        tmp_data[i] = q->data[q->rptr++];
>      }
> +    memcpy(q->data, tmp_data, size);
> +
>      /* reset rptr/wptr/count */
>      q->rptr = 0;
>      q->wptr = size;
> 

+-- On Thu, 16 Nov 2017, Paolo Bonzini wrote --+
| 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.
| 
| In the callers,
| 
| 	ps2_common_post_load(ps2);
| 
| 	return 0;
| 
| can then become simply
| 
| 	return ps2_common_post_load(ps2);

  Okay; Sent a revised patch v2.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

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

* 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

  Hi,

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

Ok, finally queueed up v1 for merge.

thanks,
  Gerd

+-- On Thu, 25 Jan 2018, Gerd Hoffmann wrote --+
| Ok, finally queueed up v1 for merge.

Okay, cool. Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F