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

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

[Jason Wessel]

[-- Attachment #1: Type: text/plain, Size: 425 bytes --]

After using the simulated usb serial device with significant amounts of
gdb debugger traffic, I found that the data packets over 255 bytes were
getting arbitrary data from prior data packets.   It turns out there are
two different memory overruns which lead to this problem.

The attached patch addresses both issues as well as removing an unused
buffer.

Please consider applying it to the development tree.

Thanks,
Jason.

[-- Attachment #2: ftdi_overrun_fix.patch --]
[-- Type: text/x-diff, Size: 1350 bytes --]

From: Jason Wessel <jason.wessel@windriver.com>
Subject: [PATCH] usb-serial: Fix memory overruns with usb serial emulation

* Remove the unused send_buf variable and its constant.

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

* 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 <jason.wessel@windriver.com>

---
 hw/usb-serial.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 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
@@ -93,10 +92,9 @@ typedef struct {
     USBDevice dev;
     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];
+    uint8_t recv_buf[RECV_BUF + 1];
+    uint16_t recv_ptr;
+    uint16_t recv_used;
     uint8_t event_chr;
     uint8_t error_chr;
     uint8_t event_trigger;

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

[Paul Brook]

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.

Paul

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

[Paul Brook]

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.

Paul

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

[Jason Wessel]

[-- Attachment #1: Type: text/plain, Size: 1215 bytes --]

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.



[-- Attachment #2: ftdi_overrun_fix_v2.patch --]
[-- Type: text/x-diff, Size: 1088 bytes --]

From: Jason Wessel <jason.wessel@windriver.com>
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 <jason.wessel@windriver.com>

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

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

[Samuel Thibault]

Jason Wessel, le Wed 17 Sep 2008 06:54:47 -0500, a écrit :
> * 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 <jason.wessel@windriver.com>

Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

Samuel

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

[Aurelien Jarno]

On Wed, Sep 17, 2008 at 06:54:47AM -0500, Jason Wessel wrote:
> 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.

Applied, thanks

> Jason.
> 
> 

> From: Jason Wessel <jason.wessel@windriver.com>
> 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 <jason.wessel@windriver.com>
> 
> ---
>  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;


-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net