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

[Aurelien Jarno <aurelien@aurel32.net>]

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