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

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

[Michael S. Tsirkin]

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>
---

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");
-- 
1.7.2.rc0.14.g41c1c

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

[Alex Williamson]

On Mon, 2010-07-12 at 20:48 +0300, 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>
> ---
> 
> Anthony, Alex, please review.

Looks fine to me.

Acked-by: Alex Williamson <alex.williamson@redhat.com>

>  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");

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

[Anthony Liguori]

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

> ---
>
> 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");
>    

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

[Michael S. Tsirkin]

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.

It's the buffer guest allocated, and we leave it untouched.
How does this leak data?

> 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");

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

[Anthony Liguori]

On 07/12/2010 04:30 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.
>>      
> It's the buffer guest allocated, and we leave it untouched.
> How does this leak data?
>    

Sorry, you're right.

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

>    
>> 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");
>>>        

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

[Michael S. Tsirkin]

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?

> >---
> >
> >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");

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

[Anthony Liguori]

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?

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");
>>>        

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

[Gleb Natapov]

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.

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

[Michael S. Tsirkin]

On Tue, Jul 13, 2010 at 09:35:49AM +0300, Gleb Natapov wrote:
> 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.

I guess we could work around this. Is there a bz?

> > 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.

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

[Gleb Natapov]

On Tue, Jul 13, 2010 at 02:11:10PM +0300, Michael S. Tsirkin wrote:
> On Tue, Jul 13, 2010 at 09:35:49AM +0300, Gleb Natapov wrote:
> > 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.
> 
> I guess we could work around this. Is there a bz?
> 
BZ where? We do not support e1000 with Windows guests.

--
			Gleb.