[Qemu-devel] [PATCH 0/2] usb: check RNDIS offsets & length

[Qemu-devel] [PATCH 0/2] usb: check RNDIS offsets & length

[P J P]

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

Hello,

When processing remote NDIS control message packets, the USB Net
device emulator uses a fixed length(4096) data buffer. The incoming
packet length could exceed that OR informationBufferOffset & Length
combination could overflow and cross that range. These two patches
add checks to avoid such overflows.

Thank you.
---
Prasad J Pandit (2):
  usb: check RNDIS message length
  usb: check RNDIS buffer offsets & length

 hw/usb/core.c        | 18 +++++++++---------
 hw/usb/dev-network.c |  9 ++++++---
 2 files changed, 15 insertions(+), 12 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH 1/2] usb: check RNDIS message length

[P J P]

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

When processing remote NDIS control message packets, the USB Net
device emulator uses a fixed length(4096) data buffer. The incoming
packet length could exceed this limit. Add a check to avoid it.

Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/usb/core.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Update as per review
  -> https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg03475.html

diff --git a/hw/usb/core.c b/hw/usb/core.c
index d0025db..7f46370 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -128,9 +128,16 @@ static void do_token_setup(USBDevice *s, USBPacket *p)
     }
 
     usb_packet_copy(p, s->setup_buf, p->iov.size);
+    s->setup_index = 0;
     p->actual_length = 0;
     s->setup_len   = (s->setup_buf[7] << 8) | s->setup_buf[6];
-    s->setup_index = 0;
+    if (s->setup_len > sizeof(s->data_buf)) {
+        fprintf(stderr,
+                "usb_generic_handle_packet: ctrl buffer too small (%d > %zu)\n",
+                s->setup_len, sizeof(s->data_buf));
+        p->status = USB_RET_STALL;
+        return;
+    }
 
     request = (s->setup_buf[0] << 8) | s->setup_buf[1];
     value   = (s->setup_buf[3] << 8) | s->setup_buf[2];
@@ -151,13 +158,6 @@ static void do_token_setup(USBDevice *s, USBPacket *p)
         }
         s->setup_state = SETUP_STATE_DATA;
     } else {
-        if (s->setup_len > sizeof(s->data_buf)) {
-            fprintf(stderr,
-                "usb_generic_handle_packet: ctrl buffer too small (%d > %zu)\n",
-                s->setup_len, sizeof(s->data_buf));
-            p->status = USB_RET_STALL;
-            return;
-        }
         if (s->setup_len == 0)
             s->setup_state = SETUP_STATE_ACK;
         else
@@ -176,7 +176,7 @@ static void do_token_in(USBDevice *s, USBPacket *p)
     request = (s->setup_buf[0] << 8) | s->setup_buf[1];
     value   = (s->setup_buf[3] << 8) | s->setup_buf[2];
     index   = (s->setup_buf[5] << 8) | s->setup_buf[4];
- 
+
     switch(s->setup_state) {
     case SETUP_STATE_ACK:
         if (!(s->setup_buf[0] & USB_DIR_IN)) {
-- 
2.5.0

[Qemu-devel] [PATCH 2/2] usb: check RNDIS buffer offsets & length

[P J P]

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

When processing remote NDIS control message packets,
the USB Net device emulator uses a fixed length(4096) data buffer.
The incoming informationBufferOffset & Length combination could
overflow and cross that range. Check control message buffer
offsets and length to avoid it.

Reported-by: Qinghao Tang <luodalongde@gmail.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/usb/dev-network.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Update as per review
  -> https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg03475.html

diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index 8a4ff49..180adce 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -915,8 +915,9 @@ static int rndis_query_response(USBNetState *s,
 
     bufoffs = le32_to_cpu(buf->InformationBufferOffset) + 8;
     buflen = le32_to_cpu(buf->InformationBufferLength);
-    if (bufoffs + buflen > length)
+    if (buflen > length || bufoffs >= length || bufoffs + buflen > length) {
         return USB_RET_STALL;
+    }
 
     infobuflen = ndis_query(s, le32_to_cpu(buf->OID),
                             bufoffs + (uint8_t *) buf, buflen, infobuf,
@@ -961,8 +962,9 @@ static int rndis_set_response(USBNetState *s,
 
     bufoffs = le32_to_cpu(buf->InformationBufferOffset) + 8;
     buflen = le32_to_cpu(buf->InformationBufferLength);
-    if (bufoffs + buflen > length)
+    if (buflen > length || bufoffs >= length || bufoffs + buflen > length) {
         return USB_RET_STALL;
+    }
 
     ret = ndis_set(s, le32_to_cpu(buf->OID),
                     bufoffs + (uint8_t *) buf, buflen);
@@ -1212,8 +1214,9 @@ static void usb_net_handle_dataout(USBNetState *s, USBPacket *p)
     if (le32_to_cpu(msg->MessageType) == RNDIS_PACKET_MSG) {
         uint32_t offs = 8 + le32_to_cpu(msg->DataOffset);
         uint32_t size = le32_to_cpu(msg->DataLength);
-        if (offs + size <= len)
+        if (offs < len && size < len && offs + size <= len) {
             qemu_send_packet(qemu_get_queue(s->nic), s->out_buf + offs, size);
+        }
     }
     s->out_ptr -= len;
     memmove(s->out_buf, &s->out_buf[len], s->out_ptr);
-- 
2.5.0

Re: [Qemu-devel] [PATCH 0/2] usb: check RNDIS offsets & length

[Gerd Hoffmann]

On Mi, 2016-02-17 at 00:23 +0530, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> Hello,
> 
> When processing remote NDIS control message packets, the USB Net
> device emulator uses a fixed length(4096) data buffer. The incoming
> packet length could exceed that OR informationBufferOffset & Length
> combination could overflow and cross that range. These two patches
> add checks to avoid such overflows.

Added both to usb patch queue.

thanks,
  Gerd