ps2: check PS2Queue pointers in post_load routine

From: Prasad J Pandit <[email protected]>

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 <[email protected]>

Signed-off-by Prasad J Pandit <[email protected]>

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

hw/input/ps2.c +0 -0

index f388a23c8e..de171a28dd 100644

hw/input/ps2.c +7 -9

     /* 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;
 

hw/input/ps2.c static void ps2_common_reset(PS2State *s) · line 1229

Quoted 13 lines

hw/input/ps2.c +2 -3

index f388a23c8e..de171a28dd 100644
@@ -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.

hw/input/ps2.c static void ps2_common_reset(PS2State *s) · line 1244

Quoted 1 line

hw/input/ps2.c +0 -1

-        memcpy(q->data, tmp_data, size);

Quoted 16 lines

hw/input/ps2.c static void ps2_common_reset(PS2State *s) · line 1229

Quoted 13 lines

hw/input/ps2.c +2 -3

index f388a23c8e..de171a28dd 100644
@@ -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,

Quoted 4 lines

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

Quoted 1 line

hw/input/ps2.c static void ps2_common_reset(PS2State *s) · line 1244

Quoted 1 line

hw/input/ps2.c +0 -1

-        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

Quoted 4 lines

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.