From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Kg58B-0003et-V8 for qemu-devel@nongnu.org; Wed, 17 Sep 2008 18:04:16 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Kg58A-0003ec-GT for qemu-devel@nongnu.org; Wed, 17 Sep 2008 18:04:15 -0400 Received: from [199.232.76.173] (port=35138 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Kg58A-0003eZ-EX for qemu-devel@nongnu.org; Wed, 17 Sep 2008 18:04:14 -0400 Received: from hall.aurel32.net ([91.121.138.14]:48220) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1Kg589-0000Xg-RG for qemu-devel@nongnu.org; Wed, 17 Sep 2008 18:04:14 -0400 Received: from volta.aurel32.net ([2002:52e8:2fb:1:21e:8cff:feb0:693b]) by hall.aurel32.net with esmtpsa (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.63) (envelope-from ) id 1Kg585-0007Lh-GA for qemu-devel@nongnu.org; Thu, 18 Sep 2008 00:04:09 +0200 Received: from aurel32 by volta.aurel32.net with local (Exim 4.69) (envelope-from ) id 1Kg583-0001Uq-PT for qemu-devel@nongnu.org; Thu, 18 Sep 2008 00:04:07 +0200 Date: Thu, 18 Sep 2008 00:04:07 +0200 From: Aurelien Jarno Subject: Re: [Qemu-devel] [PATCH] usb-serial: Fix memory overruns with usb serial emulation Message-ID: <20080917220407.GA18704@volta.aurel32.net> References: <48D08F06.2070905@windriver.com> <200809171118.42802.paul@codesourcery.com> <200809171138.01465.paul@codesourcery.com> <48D0F007.1070903@windriver.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: <48D0F007.1070903@windriver.com> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org 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 > 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 > > --- > 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