Re: [Qemu-devel] Re: [PATCH RFC] e1000: fix access 4 bytes beyond buffer end

[Gleb Natapov <gleb@redhat.com>]

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<mst@redhat.com>
> >>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.