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

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

[P J P]

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' fields were
tampered, it could lead to OOB access or infinite loop issues.
Change their type to 'uint8_t' so they remain within bounds.

Reported-by: Cyrille Chatras <cyrille.chatras@orange.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 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),
         VMSTATE_BUFFER(queue.data, PS2State),
         VMSTATE_END_OF_LIST()
     }
-- 
2.13.6

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 <pjp@fedoraproject.org>
> 
> 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 <cyrille.chatras@orange.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  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 :|

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

[Paolo Bonzini]

On 15/11/2017 13:51, Daniel P. Berrange wrote:
> 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.
> 

This is not entirely true.  A lot of such cases were fixed in the past,
especially when they could cause out-of-bounds access.  Someone could
provide a bad migration stream (e.g. as a fake bug report!), so
migration data should not be considered trusted.

However, PJP's patch breaks migration by changing a 4-byte field to
1-byte.  The correct fix is to range-check the fields in
ps2_common_post_load.

Thanks,

Paolo

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

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 15/11/2017 13:51, Daniel P. Berrange wrote:
> > 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.
> > 
> 
> This is not entirely true.  A lot of such cases were fixed in the past,
> especially when they could cause out-of-bounds access.  Someone could
> provide a bad migration stream (e.g. as a fake bug report!), so
> migration data should not be considered trusted.

There's probably others to be honest; it's not something we've
traditionally been careful of.

> However, PJP's patch breaks migration by changing a 4-byte field to
> 1-byte.  The correct fix is to range-check the fields in
> ps2_common_post_load.

Agreed.

Dave

> Thanks,
> 
> Paolo
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Paolo Bonzini]

On 15/11/2017 14:30, Dr. David Alan Gilbert wrote:
> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>> On 15/11/2017 13:51, Daniel P. Berrange wrote:
>>> 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.
>>
>> This is not entirely true.  A lot of such cases were fixed in the past,
>> especially when they could cause out-of-bounds access.  Someone could
>> provide a bad migration stream (e.g. as a fake bug report!), so
>> migration data should not be considered trusted.
> 
> There's probably others to be honest; it's not something we've
> traditionally been careful of.

There was a flurry of fixes a while ago:

- CVE-2013-4149 to CVE-2013-4151
- CVE-2013-4526 to CVE-2013-4527
- CVE-2013-4529 to CVE-2013-4542
- CVE-2013-6399
- CVE-2014-0182

This one was introduced in 2.1, around the same time these others were
fixed, by commit 2858ab09e6 ("ps2: set ps/2 output buffer size as the
same as kernel", 2014-05-16).

Thanks,

Paolo

> 
>> However, PJP's patch breaks migration by changing a 4-byte field to
>> 1-byte.  The correct fix is to range-check the fields in
>> ps2_common_post_load.
> 
> Agreed.
> 
> Dave
> 
>> Thanks,
>>
>> Paolo
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

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

[P J P]

+-- On Wed, 15 Nov 2017, Dr. David Alan Gilbert wrote --+
| * Paolo Bonzini (pbonzini@redhat.com) wrote:
| > However, PJP's patch breaks migration by changing a 4-byte field to
| > 1-byte.  The correct fix is to range-check the fields in
| > ps2_common_post_load.
| 
| Agreed.

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