From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KfvcN-0003Nc-QW for qemu-devel@nongnu.org; Wed, 17 Sep 2008 07:54:47 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KfvcL-0003NG-C2 for qemu-devel@nongnu.org; Wed, 17 Sep 2008 07:54:46 -0400 Received: from [199.232.76.173] (port=55862 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KfvcL-0003NB-2L for qemu-devel@nongnu.org; Wed, 17 Sep 2008 07:54:45 -0400 Received: from mail.windriver.com ([147.11.1.11]:44975 helo=mail.wrs.com) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KfvcK-0002qk-Jq for qemu-devel@nongnu.org; Wed, 17 Sep 2008 07:54:44 -0400 Received: from ALA-MAIL03.corp.ad.wrs.com (ala-mail03 [147.11.57.144]) by mail.wrs.com (8.13.6/8.13.6) with ESMTP id m8HBscjv008099 for ; Wed, 17 Sep 2008 04:54:39 -0700 (PDT) Message-ID: <48D0F007.1070903@windriver.com> Date: Wed, 17 Sep 2008 06:54:47 -0500 From: Jason Wessel MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] usb-serial: Fix memory overruns with usb serial emulation References: <48D08F06.2070905@windriver.com> <200809171118.42802.paul@codesourcery.com> <200809171138.01465.paul@codesourcery.com> In-Reply-To: <200809171138.01465.paul@codesourcery.com> Content-Type: multipart/mixed; boundary="------------020305060708030107000400" 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 This is a multi-part message in MIME format. --------------020305060708030107000400 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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. --------------020305060708030107000400 Content-Type: text/x-diff; name="ftdi_overrun_fix_v2.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="ftdi_overrun_fix_v2.patch" 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; --------------020305060708030107000400--