Re: [PATCH] hw/net/dp8393x: fix integer underflow in dp8393x_do_transmit_packets()

[Philippe Mathieu-Daudé]

+Laurent/Finn

On 11/24/20 10:24 AM, Mauro Matteo Cascella wrote:
> An integer underflow could occur during packet transmission due to 'tx_len' 
> not
> being updated if SONIC_TFC register is set to zero. Check for negative 
> 'tx_len'
> when removing existing FCS.
> 
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1899722
> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> Reported-by: Gaoning Pan <pgn@zju.edu.cn>
> ---
>  hw/net/dp8393x.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 674b04b354..205c0decc5 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -495,6 +495,10 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
>          } else {
>              /* Remove existing FCS */
>              tx_len -= 4;
> +            if (tx_len < 0) {
> +                SONIC_ERROR("tx_len is %d\n", tx_len);
> +                break;
> +            }
>          }
>  
>          if (s->regs[SONIC_RCR] & (SONIC_RCR_LB1 | SONIC_RCR_LB0)) {
> 

Doesn't it make more sense to check 'tx_len >= 4'
and skip tx/rx when 'tx_len == 0'?

-- >8 --
@@ -488,25 +488,29 @@ static void
dp8393x_do_transmit_packets(dp8393xState *s)
             }
         }

-        /* Handle Ethernet checksum */
-        if (!(s->regs[SONIC_TCR] & SONIC_TCR_CRCI)) {
-            /* Don't append FCS there, to look like slirp packets
-             * which don't have one */
-        } else {
-            /* Remove existing FCS */
-            tx_len -= 4;
+        if (tx_len >= 4) {
+            /* Handle Ethernet checksum */
+            if (!(s->regs[SONIC_TCR] & SONIC_TCR_CRCI)) {
+                /* Don't append FCS there, to look like slirp packets
+                 * which don't have one */
+            } else {
+                /* Remove existing FCS */
+                tx_len -= 4;
+            }
         }

-        if (s->regs[SONIC_RCR] & (SONIC_RCR_LB1 | SONIC_RCR_LB0)) {
-            /* Loopback */
-            s->regs[SONIC_TCR] |= SONIC_TCR_CRSL;
-            if (nc->info->can_receive(nc)) {
-                s->loopback_packet = 1;
-                nc->info->receive(nc, s->tx_buffer, tx_len);
+        if (tx_len > 0) {
+            if (s->regs[SONIC_RCR] & (SONIC_RCR_LB1 | SONIC_RCR_LB0)) {
+                /* Loopback */
+                s->regs[SONIC_TCR] |= SONIC_TCR_CRSL;
+                if (nc->info->can_receive(nc)) {
+                    s->loopback_packet = 1;
+                    nc->info->receive(nc, s->tx_buffer, tx_len);
+                }
+            } else {
+                /* Transmit packet */
+                qemu_send_packet(nc, s->tx_buffer, tx_len);
             }
-        } else {
-            /* Transmit packet */
-            qemu_send_packet(nc, s->tx_buffer, tx_len);
         }
         s->regs[SONIC_TCR] |= SONIC_TCR_PTX;

---