ps2: fix PS2Queue counter field type

From: Prasad J Pandit <[email protected]>

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

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

---
 hw/input/ps2.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

hw/input/ps2.c +7 -13

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

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.

Quoted 9 lines · Daniel P. Berrange

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

Quoted 10 lines · * Paolo Bonzini ([email protected]) wrote:

Quoted 8 lines · Daniel P. Berrange

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.

Quoted 4 lines

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.

Quoted 4 lines

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

Quoted 4 lines

Thanks,

Paolo

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK