[Qemu-devel] [PATCH 0/2] rtl8139: Fix buffer overflow in standard mode

[Qemu-devel] [PATCH 0/2] rtl8139: Fix buffer overflow in standard mode

[Vladislav Yasevich]

When rtl8139 card is running in standard mode, it is very easy
to overlflow and the receive buffer and get into a siutation
where all packets are dropped.  Simply reproduction case is
to ping the guest from the host with 6500 byte packets.  

There are actually 2 problems here.
 1) When the rtl8129 buffer is overflow, the card emulation
    returns the size of the packet back to queue transmission.
    This signals successful reception even though the packet
    has been dropped.  The proper solution is to return 0, so
    that the packet is re-queued and will be resubmitted later.

 2) When packets are sized such that the fragments end up completely
    filling the receive buffer without overflow, the device thinks
    that the buffer is actually empty (instead of full).  This causes
    next packet to over-write the existing packets.  With the above
    ping reproducer, ever ICMP packet fills the buffer and thus keeps
    overwriting the previous packet and never waking up the guest.
    The solution here is to trap the buffer full condition and let
    the guest read the data before writing more data to the rxBuffer.


Vladislav Yasevich (2):
  rtl8139: Do not consume the packet during overflow in standard mode.
  rtl8139: correctly track full receive buffer in standard mode

 hw/net/rtl8139.c | 39 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/2] rtl8139: Do not consume the packet during overflow in standard mode.

[Vladislav Yasevich]

When operation in standard mode, we currently return the size
of packet during buffer overflow.  This consumes the overflow
packet.  Return 0 instead so we can re-process the overflow packet
when we have room.

This fixes issues with lost/dropped fragments of large messages.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 hw/net/rtl8139.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index edbb61c..359e001 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -1157,7 +1157,7 @@ static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t
             s->IntrStatus |= RxOverflow;
             ++s->RxMissed;
             rtl8139_update_irq(s);
-            return size_;
+            return 0;
         }
 
         packet_header |= RxStatusOK;
-- 
1.9.3

Re: [Qemu-devel] [PATCH 1/2] rtl8139: Do not consume the packet during overflow in standard mode.

[Stefan Hajnoczi]

On Fri, Aug 21, 2015 at 02:59:24PM -0700, Vladislav Yasevich wrote:
> When operation in standard mode, we currently return the size
> of packet during buffer overflow.  This consumes the overflow
> packet.  Return 0 instead so we can re-process the overflow packet
> when we have room.
> 
> This fixes issues with lost/dropped fragments of large messages.
> 
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
>  hw/net/rtl8139.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> index edbb61c..359e001 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -1157,7 +1157,7 @@ static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t
>              s->IntrStatus |= RxOverflow;
>              ++s->RxMissed;
>              rtl8139_update_irq(s);
> -            return size_;
> +            return 0;

Every .receive() return 0 must be paired with a
qemu_flush_queued_packets() call.

Is rtl8139_RxBufPtr_write() guaranteed to be called when the guest
refills rx buffers?

Re: [Qemu-devel] [PATCH 1/2] rtl8139: Do not consume the packet during overflow in standard mode.

[Vlad Yasevich]

On 08/26/2015 08:18 AM, Stefan Hajnoczi wrote:
> On Fri, Aug 21, 2015 at 02:59:24PM -0700, Vladislav Yasevich wrote:
>> When operation in standard mode, we currently return the size
>> of packet during buffer overflow.  This consumes the overflow
>> packet.  Return 0 instead so we can re-process the overflow packet
>> when we have room.
>>
>> This fixes issues with lost/dropped fragments of large messages.
>>
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>> ---
>>  hw/net/rtl8139.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
>> index edbb61c..359e001 100644
>> --- a/hw/net/rtl8139.c
>> +++ b/hw/net/rtl8139.c
>> @@ -1157,7 +1157,7 @@ static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t
>>              s->IntrStatus |= RxOverflow;
>>              ++s->RxMissed;
>>              rtl8139_update_irq(s);
>> -            return size_;
>> +            return 0;
> 
> Every .receive() return 0 must be paired with a
> qemu_flush_queued_packets() call.

Isn't that already there.  A few drivers already return 0 to trigger a requeue.  What's
missing?


> 
> Is rtl8139_RxBufPtr_write() guaranteed to be called when the guest
> refills rx buffers?
> 

It calls it on read, once it's consumed a packet, to advance to the next packet in the
buffer.

-vlad

[Qemu-devel] [PATCH 2/2] rtl8139: correctly track full receive buffer in standard mode

[Vladislav Yasevich]

In standard operation mode, when the receive ring buffer
is full, the buffer actually appears empty to the driver since
the RxBufAddr (the location we wirte new data to) and RxBufPtr
(the location guest would stat reading from) are the same.
As a result, the call to rtl8139_RxBufferEmpty ends up
returning true indicating that the receive buffer is empty.
This would result in the next packet overwriting the recevie buffer
again and stalling receive operations.

This patch catches the "receive buffer full" condition
using an unused C+ register.  This is done to simplify
migration and not require a new machine type.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 hw/net/rtl8139.c | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 359e001..3d572ab 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -816,6 +816,23 @@ static int rtl8139_can_receive(NetClientState *nc)
     }
 }
 
+static void rtl8139_set_rxbuf_full(RTL8139State *s, bool full)
+{
+    /* In standard mode, C+ RxDesc isn't used.  Reuse it
+     * to store the rx_buf_full status.
+     */
+    s->currCPlusRxDesc = full;
+    DPRINTF("received: rx buffer full\n");
+}
+
+static bool rtl8139_rxbuf_full(RTL8139State *s)
+{
+    /* In standard mode, C+ RxDesc isn't used.  Reuse it
+     * to store the rx_buf_full status.
+     */
+    return !!s->currCPlusRxDesc;
+}
+
 static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t size_, int do_interrupt)
 {
     RTL8139State *s = qemu_get_nic_opaque(nc);
@@ -1178,6 +1195,14 @@ static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t
         /* correct buffer write pointer */
         s->RxBufAddr = MOD2((s->RxBufAddr + 3) & ~0x3, s->RxBufferSize);
 
+        /* Catch buffer full condition.  Without this, we end up
+        * assuming that buffer is empty.
+        */
+        if (s->RxBufAddr == s->RxBufPtr)
+        {
+            rtl8139_set_rxbuf_full(s, true);
+        }
+
         /* now we can signal we have received something */
 
         DPRINTF("received: rx buffer length %d head 0x%04x read 0x%04x\n",
@@ -1414,9 +1439,10 @@ static int rtl8139_RxBufferEmpty(RTL8139State *s)
 {
     int unread = MOD2(s->RxBufferSize + s->RxBufAddr - s->RxBufPtr, s->RxBufferSize);
 
-    if (unread != 0)
+    if (unread != 0 || rtl8139_rxbuf_full(s))
     {
-        DPRINTF("receiver buffer data available 0x%04x\n", unread);
+        DPRINTF("receiver buffer data available 0x%04x\n",
+                unread ? : s->RxBufferSize);
         return 0;
     }
 
@@ -1430,7 +1456,10 @@ static uint32_t rtl8139_ChipCmd_read(RTL8139State *s)
     uint32_t ret = s->bChipCmdState;
 
     if (rtl8139_RxBufferEmpty(s))
+    {
         ret |= RxBufEmpty;
+        rtl8139_set_rxbuf_full(s, false);
+    }
 
     DPRINTF("ChipCmd read val=0x%04x\n", ret);
 
@@ -2601,6 +2630,9 @@ static void rtl8139_RxBufPtr_write(RTL8139State *s, uint32_t val)
     /* this value is off by 16 */
     s->RxBufPtr = MOD2(val + 0x10, s->RxBufferSize);
 
+    /* We just read data, clear full buffer state */
+    rtl8139_set_rxbuf_full(s, false);
+
     /* more buffer space may be available so try to receive */
     qemu_flush_queued_packets(qemu_get_queue(s->nic));
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH 2/2] rtl8139: correctly track full receive buffer in standard mode

[Stefan Hajnoczi]

On Fri, Aug 21, 2015 at 02:59:25PM -0700, Vladislav Yasevich wrote:
> In standard operation mode, when the receive ring buffer
> is full, the buffer actually appears empty to the driver since
> the RxBufAddr (the location we wirte new data to) and RxBufPtr
> (the location guest would stat reading from) are the same.
> As a result, the call to rtl8139_RxBufferEmpty ends up
> returning true indicating that the receive buffer is empty.
> This would result in the next packet overwriting the recevie buffer
> again and stalling receive operations.
> 
> This patch catches the "receive buffer full" condition
> using an unused C+ register.  This is done to simplify
> migration and not require a new machine type.
> 
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
>  hw/net/rtl8139.c | 36 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)

The rtl8139 code duplicates the following expression in several places:

  MOD2(s->RxBufferSize + s->RxBufAddr - s->RxBufPtr, s->RxBufferSize);

It may be cleaner to keep a rx_unread_bytes counter so that all these
users can simply look at that variable.

That cleanup also eliminates the rx full vs empty problem because then
we'll know whether rx_unread_bytes == 0 or rx_unread_bytes ==
s->RxBufferSize.

The same trick of stashing the value in s->currCPlusRxDesc could be
used.

> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> index 359e001..3d572ab 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -816,6 +816,23 @@ static int rtl8139_can_receive(NetClientState *nc)
>      }
>  }
>  
> +static void rtl8139_set_rxbuf_full(RTL8139State *s, bool full)
> +{
> +    /* In standard mode, C+ RxDesc isn't used.  Reuse it
> +     * to store the rx_buf_full status.
> +     */

assert(!s->cplus_enabled)?

> +    s->currCPlusRxDesc = full;
> +    DPRINTF("received: rx buffer full\n");
> +}
> +
> +static bool rtl8139_rxbuf_full(RTL8139State *s)
> +{
> +    /* In standard mode, C+ RxDesc isn't used.  Reuse it
> +     * to store the rx_buf_full status.
> +     */

assert(!s->cplus_enabled)?

> @@ -2601,6 +2630,9 @@ static void rtl8139_RxBufPtr_write(RTL8139State *s, uint32_t val)
>      /* this value is off by 16 */
>      s->RxBufPtr = MOD2(val + 0x10, s->RxBufferSize);
>  
> +    /* We just read data, clear full buffer state */
> +    rtl8139_set_rxbuf_full(s, false);
> +
>      /* more buffer space may be available so try to receive */
>      qemu_flush_queued_packets(qemu_get_queue(s->nic));

What if the guest writes this register while we're in C+ mode?

Re: [Qemu-devel] [PATCH 2/2] rtl8139: correctly track full receive buffer in standard mode

[Vlad Yasevich]

On 08/26/2015 08:36 AM, Stefan Hajnoczi wrote:
> On Fri, Aug 21, 2015 at 02:59:25PM -0700, Vladislav Yasevich wrote:
>> In standard operation mode, when the receive ring buffer
>> is full, the buffer actually appears empty to the driver since
>> the RxBufAddr (the location we wirte new data to) and RxBufPtr
>> (the location guest would stat reading from) are the same.
>> As a result, the call to rtl8139_RxBufferEmpty ends up
>> returning true indicating that the receive buffer is empty.
>> This would result in the next packet overwriting the recevie buffer
>> again and stalling receive operations.
>>
>> This patch catches the "receive buffer full" condition
>> using an unused C+ register.  This is done to simplify
>> migration and not require a new machine type.
>>
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>> ---
>>  hw/net/rtl8139.c | 36 ++++++++++++++++++++++++++++++++++--
>>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> The rtl8139 code duplicates the following expression in several places:
> 
>   MOD2(s->RxBufferSize + s->RxBufAddr - s->RxBufPtr, s->RxBufferSize);
> 
> It may be cleaner to keep a rx_unread_bytes counter so that all these
> users can simply look at that variable.
> 
> That cleanup also eliminates the rx full vs empty problem because then
> we'll know whether rx_unread_bytes == 0 or rx_unread_bytes ==
> s->RxBufferSize.
> 
> The same trick of stashing the value in s->currCPlusRxDesc could be
> used.
> 

Good idea.  I'll give it a try.


>> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
>> index 359e001..3d572ab 100644
>> --- a/hw/net/rtl8139.c
>> +++ b/hw/net/rtl8139.c
>> @@ -816,6 +816,23 @@ static int rtl8139_can_receive(NetClientState *nc)
>>      }
>>  }
>>  
>> +static void rtl8139_set_rxbuf_full(RTL8139State *s, bool full)
>> +{
>> +    /* In standard mode, C+ RxDesc isn't used.  Reuse it
>> +     * to store the rx_buf_full status.
>> +     */
> 
> assert(!s->cplus_enabled)?
> 
>> +    s->currCPlusRxDesc = full;
>> +    DPRINTF("received: rx buffer full\n");
>> +}
>> +
>> +static bool rtl8139_rxbuf_full(RTL8139State *s)
>> +{
>> +    /* In standard mode, C+ RxDesc isn't used.  Reuse it
>> +     * to store the rx_buf_full status.
>> +     */
> 
> assert(!s->cplus_enabled)?
> 
>> @@ -2601,6 +2630,9 @@ static void rtl8139_RxBufPtr_write(RTL8139State *s, uint32_t val)
>>      /* this value is off by 16 */
>>      s->RxBufPtr = MOD2(val + 0x10, s->RxBufferSize);
>>  
>> +    /* We just read data, clear full buffer state */
>> +    rtl8139_set_rxbuf_full(s, false);
>> +
>>      /* more buffer space may be available so try to receive */
>>      qemu_flush_queued_packets(qemu_get_queue(s->nic));
> 
> What if the guest writes this register while we're in C+ mode?
> 

In C+ mode the guest uses a descriptor ring instead of liner buffer so a well behaved
C+ guest wouldn't write this value.  Validated this with a linux guest.
I guess a malicious guest could do this, but then it could do a lot of other things too.

-vlad

[Qemu-devel] [PATCH v2 0/2] rtl8139: Fix buffer overflow in standard mode

[Vladislav Yasevich]

When rtl8139 card is running in standard mode, it is very easy
to overlflow and the receive buffer and get into a siutation
where all packets are dropped.  Simply reproduction case is
to ping the guest from the host with 6500 byte packets.  

There are actually 2 problems here.
 1) When the rtl8129 buffer is overflow, the card emulation
    returns the size of the packet back to queue transmission.
    This signals successful reception even though the packet
    has been dropped.  The proper solution is to return 0, so
    that the packet is re-queued and will be resubmitted later.

 2) When packets are sized such that the fragments end up completely
    filling the receive buffer without overflow, the device thinks
    that the buffer is actually empty (instead of full).  This causes
    next packet to over-write the existing packets.  With the above
    ping reproducer, ever ICMP packet fills the buffer and thus keeps
    overwriting the previous packet and never waking up the guest.
    The solution here is track the number of unread bytes separately
    so we would know if we have anything in buffer to read or not.

V2: instead of tracking buffer_full condition, changed the code, as
    suggested by Stefan Hajnoczi, to track the number of unread bytes
    instead.  We initialize it to 0 at the start, adjust it on every
    receive from the network and read from the guest and can set
    the number of unread of bytes to full buffer size when the buffer
    full.

Vladislav Yasevich (2):
  rtl8139: Do not consume the packet during overflow in standard mode.
  rtl8139: correctly track full receive buffer in standard mode

 hw/net/rtl8139.c | 44 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 39 insertions(+), 5 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/2] rtl8139: Do not consume the packet during overflow in standard mode.

[Vladislav Yasevich]

When operation in standard mode, we currently return the size
of packet during buffer overflow.  This consumes the overflow
packet.  Return 0 instead so we can re-process the overflow packet
when we have room.

This fixes issues with lost/dropped fragments of large messages.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 hw/net/rtl8139.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index edbb61c..359e001 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -1157,7 +1157,7 @@ static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t
             s->IntrStatus |= RxOverflow;
             ++s->RxMissed;
             rtl8139_update_irq(s);
-            return size_;
+            return 0;
         }
 
         packet_header |= RxStatusOK;
-- 
1.9.3