From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=48310 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OYZ5u-0000RP-U4 for qemu-devel@nongnu.org; Tue, 13 Jul 2010 02:35:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OYZ5t-0001qn-So for qemu-devel@nongnu.org; Tue, 13 Jul 2010 02:35:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:4791) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OYZ5t-0001qc-Ml for qemu-devel@nongnu.org; Tue, 13 Jul 2010 02:35:53 -0400 Date: Tue, 13 Jul 2010 09:35:49 +0300 From: Gleb Natapov Subject: Re: [Qemu-devel] Re: [PATCH RFC] e1000: fix access 4 bytes beyond buffer end Message-ID: <20100713063549.GA5463@redhat.com> References: <20100712174823.GA11411@redhat.com> <4C3B8409.9030202@codemonkey.ws> <20100712224254.GD13707@redhat.com> <4C3B9E84.3050809@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4C3B9E84.3050809@codemonkey.ws> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: alex.williamson@redhat.com, qemu-devel@nongnu.org, "Michael S. Tsirkin" On Mon, Jul 12, 2010 at 06:00:20PM -0500, Anthony Liguori wrote: > On 07/12/2010 05:42 PM, Michael S. Tsirkin wrote: > >On Mon, Jul 12, 2010 at 04:07:21PM -0500, Anthony Liguori wrote: > >>On 07/12/2010 12:48 PM, Michael S. Tsirkin wrote: > >>>We do range check for size, and get size as buffer, > >>>but copy size + 4 bytes (4 is for FCS). > >>>Let's copy size bytes but put size + 4 in length. > >>> > >>>Signed-off-by: Michael S. Tsirkin > >>I think I'd feel slightly better if we zero'd out the FCS before > >>writing it to the guest. It is potentially a data leak. > >> > >>Regards, > >> > >>Anthony Liguori > >I am guessing there's no chance guest actually looks > >at this data, otherwise it won't match and we'd get errors, right? > > That's my assumption too. Although I believe there are some known > issues with e1000 and certain versions of Windows and the Microsoft > built-in driver. Maybe this is why those drivers don't work and the > Intel drivers do? > At least one known issue with Windows drivers to me is that they sometimes (on resume from S4 at least) enable interrupts before setup irq routing, so if interrupt is generated in the wrong time it hangs the guest. I guess it works on real HW for them because line speed negotiation takes non-zero time. > Regards, > > Anthony Liguori > > >>>--- > >>> > >>>Anthony, Alex, please review. > >>> > >>> hw/e1000.c | 3 +-- > >>> 1 files changed, 1 insertions(+), 2 deletions(-) > >>> > >>>diff --git a/hw/e1000.c b/hw/e1000.c > >>>index 0da65f9..70aba11 100644 > >>>--- a/hw/e1000.c > >>>+++ b/hw/e1000.c > >>>@@ -649,7 +649,6 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size) > >>> } > >>> > >>> rdh_start = s->mac_reg[RDH]; > >>>- size += 4; // for the header > >>> do { > >>> if (s->mac_reg[RDH] == s->mac_reg[RDT]&& s->check_rxov) { > >>> set_ics(s, 0, E1000_ICS_RXO); > >>>@@ -663,7 +662,7 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size) > >>> if (desc.buffer_addr) { > >>> cpu_physical_memory_write(le64_to_cpu(desc.buffer_addr), > >>> (void *)(buf + vlan_offset), size); > >>>- desc.length = cpu_to_le16(size); > >>>+ desc.length = cpu_to_le16(size + 4 /* for FCS */); > >>> desc.status |= E1000_RXD_STAT_EOP|E1000_RXD_STAT_IXSM; > >>> } else // as per intel docs; skip descriptors with null buf addr > >>> DBGOUT(RX, "Null RX descriptor!!\n"); > -- Gleb.