Re: [Qemu-devel] [PATCH] usb-serial: Fix memory overruns with usb serial emulation

[Jason Wessel]

Paul Brook wrote:
> On Wednesday 17 September 2008, Paul Brook wrote:
>   
>> On Wednesday 17 September 2008, Jason Wessel wrote:
>>     
>>> * Fix a memory overrun
>>>     recv_buf[RECV_BUF + 1];
>>>   This has to be + 1 because RECV_BUF is used for memcpy computations
>>>   in usb_serial_read() such that an extra byte is 0..RECV_BUF bytes
>>>   are used.
>>>       
>> I think this is wrong. I can't see any way this code could overflow.
>>     
>
> On further inspection I can see a bug, but the above change is not the 
> correct 
> fix, and it will cause lost data not overflows.  The calculation of 
> first_size is incorrect when the buffer has wrapped.
>   


The overflow was a result of the printf()'s introduced to track and
print all the data.  So it is correct in that you do not need the
RECV_BUF+1 for the buffer for the base patch.  I did not see any kind of
miscalculation with the first_size with or without the wrap condition. 
Regression testing with all the checksummed packets shows zero failures
with the revised attached patch.

Obviously the math error as a result of not using variables that are
large enough is a very real problem.  With results that can easily be
demonstrated.

Jason.


From: Jason Wessel <address@hidden>
Subject: [PATCH] usb-serial: Fix data corruption with usb serial emulation

* Remove the unused send_buf variable and its constant.

* Fix a math error
  The variables recv_ptr and recv_used are not large enough to hold
  the constant 384, which causes data corruption when the pointer is
  reset with: s->recv_ptr = (s->recv_ptr + len) % RECV_BUF;

Signed-off-by: Jason Wessel <address@hidden>

---
 hw/usb-serial.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

--- a/hw/usb-serial.c
+++ b/hw/usb-serial.c
@@ -22,7 +22,6 @@ do { printf("usb-serial: " fmt , ##args)
 #endif
 
 #define RECV_BUF 384
-#define SEND_BUF 128        // Not used for now
 
 /* Commands */
 #define FTDI_RESET             0
@@ -94,9 +93,8 @@ typedef struct {
     uint16_t vendorid;
     uint16_t productid;
     uint8_t recv_buf[RECV_BUF];
-    uint8_t recv_ptr;
-    uint8_t recv_used;
-    uint8_t send_buf[SEND_BUF];
+    uint16_t recv_ptr;
+    uint16_t recv_used;
     uint8_t event_chr;
     uint8_t error_chr;
     uint8_t event_trigger;