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

[Qemu-devel] [PATCH v4 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) The RxBuffAddr pointer used to track where packet is to be written
    is aligned on a 4 byte boundary thus potentially adding some padding
    between packets.  This padding is ignored when checking for overflow
    condition.  It is possible to craft the packet such that we would
    fail to detect an overflow and trigger buffer overwrite.  Adding
    padding to the overflow check resolves this issues.

V4: As Jason Wang correctly pointed out, the overflow check should
    catch this.  The reason it wasn't catching was that it was ignorring
    the padding that may exist between each packet.  Adding the padding
    to the overflow check resolved the issue.  This becomes a much
    simpler fix.

V3: Fix the second patch to correctly track unread and available buffer
    speace.   Prior version used calculation for availble space to track
    unread space which was wrong.

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: Fix receive buffer overflow check
  rtl8139: Do not consume the packet during overflow in standard mode.

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

-- 
1.9.3

[Qemu-devel] [PATCH v4 1/2] rtl8139: Fix receive buffer overflow check

[Vladislav Yasevich]

rtl8139_do_receive() tries to check for the overflow condition
by making sure that packet_size + 8 does not exceed the
available buffer space.  The issue here is that RxBuffAddr,
used to calculate available buffer space, is aligned to a
a 4 byte boundry after every update.  So it is possible that
every packet ends up being slightly padded when written
to the receive buffer.  This padding is not taken into
account when checking for overflow and we may end up missing
the overflow condition can causing buffer overwrite.

This patch takes alignment into consideration when
checking for overflow condition.

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

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index edbb61c..8a33466 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -1148,7 +1148,9 @@ static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t
 
         /* if receiver buffer is empty then avail == 0 */
 
-        if (avail != 0 && size + 8 >= avail)
+#define RX_ALIGN(x) (((x) + 3) & ~0x3)
+
+        if (avail != 0 && RX_ALIGN(size + 8) >= avail)
         {
             DPRINTF("rx overflow: rx buffer length %d head 0x%04x "
                 "read 0x%04x === available 0x%04x need 0x%04x\n",
@@ -1176,7 +1178,7 @@ static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t
         rtl8139_write_buffer(s, (uint8_t *)&val, 4);
 
         /* correct buffer write pointer */
-        s->RxBufAddr = MOD2((s->RxBufAddr + 3) & ~0x3, s->RxBufferSize);
+        s->RxBufAddr = MOD2(RX_ALIGN(s->RxBufAddr), s->RxBufferSize);
 
         /* now we can signal we have received something */
 
-- 
1.9.3

[Qemu-devel] [PATCH v4 2/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 8a33466..cb51613 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -1159,7 +1159,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 v4 1/2] rtl8139: Fix receive buffer overflow check

[Jason Wang]

On 09/01/2015 11:26 PM, Vladislav Yasevich wrote:
> rtl8139_do_receive() tries to check for the overflow condition
> by making sure that packet_size + 8 does not exceed the
> available buffer space.  The issue here is that RxBuffAddr,
> used to calculate available buffer space, is aligned to a
> a 4 byte boundry after every update.  So it is possible that
> every packet ends up being slightly padded when written
> to the receive buffer.  This padding is not taken into
> account when checking for overflow and we may end up missing
> the overflow condition can causing buffer overwrite.
>
> This patch takes alignment into consideration when
> checking for overflow condition.
>
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
>  hw/net/rtl8139.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> index edbb61c..8a33466 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -1148,7 +1148,9 @@ static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t
>  
>          /* if receiver buffer is empty then avail == 0 */
>  
> -        if (avail != 0 && size + 8 >= avail)
> +#define RX_ALIGN(x) (((x) + 3) & ~0x3)
> +
> +        if (avail != 0 && RX_ALIGN(size + 8) >= avail)
>          {
>              DPRINTF("rx overflow: rx buffer length %d head 0x%04x "
>                  "read 0x%04x === available 0x%04x need 0x%04x\n",
> @@ -1176,7 +1178,7 @@ static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t
>          rtl8139_write_buffer(s, (uint8_t *)&val, 4);
>  
>          /* correct buffer write pointer */
> -        s->RxBufAddr = MOD2((s->RxBufAddr + 3) & ~0x3, s->RxBufferSize);
> +        s->RxBufAddr = MOD2(RX_ALIGN(s->RxBufAddr), s->RxBufferSize);
>  
>          /* now we can signal we have received something */
>  

Reviewed-by: Jason Wang <jasowang@redhat.com>

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

[Jason Wang]

On 09/01/2015 11:26 PM, 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 8a33466..cb51613 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -1159,7 +1159,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;

Reviewed-by: Jason Wang <jasowang@redhat.com>

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

[Stefan Hajnoczi]

On Tue, Sep 01, 2015 at 11:26:44AM -0400, Vladislav Yasevich wrote:
> 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) The RxBuffAddr pointer used to track where packet is to be written
>     is aligned on a 4 byte boundary thus potentially adding some padding
>     between packets.  This padding is ignored when checking for overflow
>     condition.  It is possible to craft the packet such that we would
>     fail to detect an overflow and trigger buffer overwrite.  Adding
>     padding to the overflow check resolves this issues.
> 
> V4: As Jason Wang correctly pointed out, the overflow check should
>     catch this.  The reason it wasn't catching was that it was ignorring
>     the padding that may exist between each packet.  Adding the padding
>     to the overflow check resolved the issue.  This becomes a much
>     simpler fix.
> 
> V3: Fix the second patch to correctly track unread and available buffer
>     speace.   Prior version used calculation for availble space to track
>     unread space which was wrong.
> 
> 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: Fix receive buffer overflow check
>   rtl8139: Do not consume the packet during overflow in standard mode.
> 
>  hw/net/rtl8139.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> -- 
> 1.9.3
> 

Thanks, applied to my net tree:
https://github.com/stefanha/qemu/commits/net

Stefan

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

[Stefan Hajnoczi]

On Tue, Sep 01, 2015 at 11:26:44AM -0400, Vladislav Yasevich wrote:
> 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) The RxBuffAddr pointer used to track where packet is to be written
>     is aligned on a 4 byte boundary thus potentially adding some padding
>     between packets.  This padding is ignored when checking for overflow
>     condition.  It is possible to craft the packet such that we would
>     fail to detect an overflow and trigger buffer overwrite.  Adding
>     padding to the overflow check resolves this issues.
> 
> V4: As Jason Wang correctly pointed out, the overflow check should
>     catch this.  The reason it wasn't catching was that it was ignorring
>     the padding that may exist between each packet.  Adding the padding
>     to the overflow check resolved the issue.  This becomes a much
>     simpler fix.
> 
> V3: Fix the second patch to correctly track unread and available buffer
>     speace.   Prior version used calculation for availble space to track
>     unread space which was wrong.
> 
> 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: Fix receive buffer overflow check
>   rtl8139: Do not consume the packet during overflow in standard mode.
> 
>  hw/net/rtl8139.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

CCing qemu-stable@nongnu.org