[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] usb-serial: Fix memory overruns with usb serial
From: |
Aurelien Jarno |
Subject: |
Re: [Qemu-devel] [PATCH] usb-serial: Fix memory overruns with usb serial emulation |
Date: |
Thu, 18 Sep 2008 00:04:07 +0200 |
User-agent: |
Mutt/1.5.18 (2008-05-17) |
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 <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;
--
.''`. Aurelien Jarno | GPG: 1024D/F1BCDB73
: :' : Debian developer | Electrical Engineer
`. `' address@hidden | address@hidden
`- people.debian.org/~aurel32 | www.aurel32.net