Skip to contentSkip to conversation

usb: fix up post load checks

PATCH Superseded Michael S. Tsirkin 3 participants 9 messages May 12, 2014 – May 14, 2014

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Michael S. Tsirkin

author
#0 lore raw

PATCH 1/1 usb: fix up post load checks

Correct post load checks:
1. dev->setup_len == sizeof(dev->data_buf)
seems fine, no need to fail migration
2. When state is DATA, passing index > len
will cause memcpy with negative length,
resulting in heap overflow

First of the issues was reported by dgilbert.

Reported-by "Dr. David Alan Gilbert" <[email protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/usb/bus.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
hw/usb/bus.c +3 -2
index e48b19f..2721719 100644
@@ -51,8 +51,9 @@ static int usb_device_post_load(void *opaque, int version_id)
     }
     if (dev->setup_index < 0 ||
         dev->setup_len < 0 ||
-        dev->setup_index >= sizeof(dev->data_buf) ||
-        dev->setup_len >= sizeof(dev->data_buf)) {
+        (dev->setup_state == SETUP_STATE_DATA &&
+         dev->setup_index > dev->setup_len) ||
+        dev->setup_len > sizeof(dev->data_buf)) {
         return -EINVAL;
     }
     return 0;
Signature · 1 line
MST
#1 lore raw

Hi,

Quoted 29 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Michael S. Tsirkin
Sent: Monday, May 12, 2014 8:16 PM
To: [email protected]
Cc: Gerd Hoffmann; [email protected]
Subject: [Qemu-devel] [PATCH] usb: fix up post load checks

Correct post load checks:
1. dev->setup_len == sizeof(dev->data_buf)
seems fine, no need to fail migration
2. When state is DATA, passing index > len
will cause memcpy with negative length,
resulting in heap overflow

First of the issues was reported by dgilbert.

Reported-by "Dr. David Alan Gilbert" <[email protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/usb/bus.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
hw/usb/bus.c +0 -0
index e48b19f..2721719 100644

hw/usb/bus.c static int usb_device_post_load(void *opaque, int · line 55

Quoted 5 lines
hw/usb/bus.c +0 -1
@@ -51,8 +51,9 @@ static int usb_device_post_load(void *opaque, int
 version_id)
     }
     if (dev->setup_index < 0 ||
         dev->setup_len < 0 ||
-        dev->setup_index >= sizeof(dev->data_buf) ||

Does this check should be deleted ?

Quoted 9 lines
hw/usb/bus.c +3 -1
-        dev->setup_len >= sizeof(dev->data_buf)) {
+        (dev->setup_state == SETUP_STATE_DATA &&
+         dev->setup_index > dev->setup_len) ||
+        dev->setup_len > sizeof(dev->data_buf)) {
         return -EINVAL;
     }

     return 0;

--
MST

Best regards,
-Gonglei

#2 lore raw
Quoted 26 lines · Gonglei (Arei)

Hi,

Quoted 24 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Michael S. Tsirkin
Sent: Monday, May 12, 2014 8:16 PM
To: [email protected]
Cc: Gerd Hoffmann; [email protected]
Subject: [Qemu-devel] [PATCH] usb: fix up post load checks

Correct post load checks:
1. dev->setup_len == sizeof(dev->data_buf)
seems fine, no need to fail migration
2. When state is DATA, passing index > len
will cause memcpy with negative length,
resulting in heap overflow

First of the issues was reported by dgilbert.

Reported-by "Dr. David Alan Gilbert" <[email protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/usb/bus.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Quoted 12 lines

hw/usb/bus.c static int usb_device_post_load(void *opaque, int · line 55

Quoted 10 lines
hw/usb/bus.c +0 -1
index e48b19f..2721719 100644
@@ -51,8 +51,9 @@ static int usb_device_post_load(void *opaque, int
 version_id)
     }
     if (dev->setup_index < 0 ||
         dev->setup_len < 0 ||
-        dev->setup_index >= sizeof(dev->data_buf) ||

Does this check should be deleted ?

It's ok, index <= len && len <= sizeof(buf) implies index <= sizeof(buf)

Quoted 13 lines
Quoted 10 lines
hw/usb/bus.c +3 -1
-        dev->setup_len >= sizeof(dev->data_buf)) {
+        (dev->setup_state == SETUP_STATE_DATA &&
+         dev->setup_index > dev->setup_len) ||
+        dev->setup_len > sizeof(dev->data_buf)) {
         return -EINVAL;
     }

     return 0;

--
MST

Best regards,
-Gonglei

#3 lore raw
In reply to Michael S. Tsirkin

Hi,

Quoted 1 line

+ (dev->setup_state == SETUP_STATE_DATA &&

Fails to build, SETUP_STATE_DATA is not defined here.

I think we can simply drop that check, index should never ever be larger than len, no matter what the state is.

cheers,
Gerd

author
#4 lore raw
Quoted 11 lines · Gerd Hoffmann

Hi,

Quoted 1 line

+ (dev->setup_state == SETUP_STATE_DATA &&

Fails to build, SETUP_STATE_DATA is not defined here.

I think we can simply drop that check, index should never ever be larger than len, no matter what the state is.

cheers,
Gerd

I'm confused by usb_generic_async_ctrl_complete which can modify len without touching index.
If index is stale it could get > len?
Could you hop on irc so we can discuss?

Signature · 1 line
MST
#5 lore raw
Quoted 7 lines · Michael S. Tsirkin
Quoted 6 lines · Gerd Hoffmann

Hi,

Quoted 1 line

+ (dev->setup_state == SETUP_STATE_DATA &&

Fails to build, SETUP_STATE_DATA is not defined here.

Quoted 8 lines
Quoted 5 lines

I think we can simply drop that check, index should never ever be larger than len, no matter what the state is.

cheers,
Gerd

I'm confused by usb_generic_async_ctrl_complete which can modify len without touching index.

only in setup state, before any data from/to the buffer is transfered, so index is still zero at that point.

flow is this:

state_setup: len = $buflen, index = 0
state_data:  xfer %buf data, increase index up to len while doing so.
state_ack:   index == len
state_idle:  likewise.

cheers,
Gerd

author
#6 lore raw
Quoted 11 lines · Gerd Hoffmann
Quoted 10 lines · Michael S. Tsirkin
Quoted 9 lines · Gerd Hoffmann

Hi,

Quoted 1 line

+ (dev->setup_state == SETUP_STATE_DATA &&

Fails to build, SETUP_STATE_DATA is not defined here.

I think we can simply drop that check, index should never ever be larger than len, no matter what the state is.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

cheers,
Gerd

I'm confused by usb_generic_async_ctrl_complete which can modify len without touching index.

only in setup state, before any data from/to the buffer is transfered, so index is still zero at that point.

And SETUP_STATE_PARAM?

Quoted 10 lines

flow is this:

state_setup: len = $buflen, index = 0
state_data:  xfer %buf data, increase index up to len while doing so.
state_ack:   index == len
state_idle:  likewise.

cheers,
Gerd

#7 lore raw

Hi,

Quoted 1 line

And SETUP_STATE_PARAM?

Shortcut for small control transfers on xhci. Doesn't go through the idle -> setup -> data -> ack state engine.

security-wise: you can't go from 'param' to 'data' without 'setup' inbetween. beside that index should still be zero at the point where len is modified (simliar to the other place in setup state).

side note: changing len should not happen in normal operation, only with a malicious / buggy guest. It happens in case the guest claims the data transfer is larger than the buffer supplied by the guest.

cheers,
Gerd

author
#8 lore raw
In reply to Michael S. Tsirkin
Quoted 11 lines · Michael S. Tsirkin

Correct post load checks:
1. dev->setup_len == sizeof(dev->data_buf)
seems fine, no need to fail migration
2. When state is DATA, passing index > len
will cause memcpy with negative length,
resulting in heap overflow

First of the issues was reported by dgilbert.

Reported-by "Dr. David Alan Gilbert" <[email protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>

This is CVE-2014-3461

Quoted 22 lines
---
 hw/usb/bus.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
hw/usb/bus.c +3 -2
index e48b19f..2721719 100644
@@ -51,8 +51,9 @@ static int usb_device_post_load(void *opaque, int version_id)
     }
     if (dev->setup_index < 0 ||
         dev->setup_len < 0 ||
-        dev->setup_index >= sizeof(dev->data_buf) ||
-        dev->setup_len >= sizeof(dev->data_buf)) {
+        (dev->setup_state == SETUP_STATE_DATA &&
+         dev->setup_index > dev->setup_len) ||
+        dev->setup_len > sizeof(dev->data_buf)) {
         return -EINVAL;
     }
     return 0;
Signature · 1 line
MST