[Qemu-devel] [PATCH v2 for-2.2 0/4] net: fix high impact outstanding defects reported by Coverity

[Qemu-devel] [PATCH v2 for-2.2 0/4] net: fix high impact outstanding defects reported by Coverity

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Please see details in every patch.

v2 -> v1:
 - rewrite patch 3 and patch 4 by Paolo's suggestion. Thanks.
 - add Jason's R-b tag in patch 1~3. Thanks too.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>

Gonglei (4):
  net/slirp: fix memory leak
  net/socket: fix Uninitialized scalar variable
  pcnet: fix Negative array index read
  rtl8139: fix Pointer to local outside scope

 hw/net/pcnet.c   | 55 ++++++++++++++++++++++++++++++-------------------------
 hw/net/rtl8139.c |  4 ++++
 net/slirp.c      |  3 +--
 net/socket.c     | 11 ++++++-----
 4 files changed, 41 insertions(+), 32 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH v2 for-2.2 1/4] net/slirp: fix memory leak

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

commit b412eb61 introduce 'cmd:' target for guestfwd,
and fwd don't be used in this scenario, and will leak
memory in true branch with 'cmd:'. Let's allocate memory
for fwd variable just in else statement.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Reviewed-by: Jason Wang <jasowang@redhat.com>
---
 net/slirp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/slirp.c b/net/slirp.c
index dc89e6b..377d7ef 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -643,17 +643,16 @@ static int slirp_guestfwd(SlirpState *s, const char *config_str,
         goto fail_syntax;
     }
 
-    fwd = g_malloc(sizeof(struct GuestFwd));
     snprintf(buf, sizeof(buf), "guestfwd.tcp.%d", port);
 
     if ((strlen(p) > 4) && !strncmp(p, "cmd:", 4)) {
         if (slirp_add_exec(s->slirp, 0, &p[4], &server, port) < 0) {
             error_report("conflicting/invalid host:port in guest forwarding "
                          "rule '%s'", config_str);
-            g_free(fwd);
             return -1;
         }
     } else {
+        fwd = g_malloc(sizeof(struct GuestFwd));
         fwd->hd = qemu_chr_new(buf, p, NULL);
         if (!fwd->hd) {
             error_report("could not open guest forwarding device '%s'", buf);
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 for-2.2 2/4] net/socket: fix Uninitialized scalar variable

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

If is_connected parameter is false, the saddr
variable will no initialize. Coverity report:
uninit_use: Using uninitialized value saddr.sin_port.

We don't need add saddr information to nc->info_str
when is_connected is false.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Reviewed-by: Jason Wang <jasowang@redhat.com>
---
 net/socket.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index ca4b8ba..68a93cd 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -389,11 +389,6 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
 
     nc = qemu_new_net_client(&net_dgram_socket_info, peer, model, name);
 
-    snprintf(nc->info_str, sizeof(nc->info_str),
-            "socket: fd=%d (%s mcast=%s:%d)",
-            fd, is_connected ? "cloned" : "",
-            inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));
-
     s = DO_UPCAST(NetSocketState, nc, nc);
 
     s->fd = fd;
@@ -404,6 +399,12 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
     /* mcast: save bound address as dst */
     if (is_connected) {
         s->dgram_dst = saddr;
+        snprintf(nc->info_str, sizeof(nc->info_str),
+                 "socket: fd=%d (cloned mcast=%s:%d)",
+                 fd, inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));
+    } else {
+        snprintf(nc->info_str, sizeof(nc->info_str),
+                 "socket: fd=%d", fd);
     }
 
     return s;
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 for-2.2 3/4] pcnet: fix Negative array index read

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

s->xmit_pos maybe assigned to a negative value (-1),
but in this branch variable s->xmit_pos as an index to
array s->buffer. Let's add a check for s->xmit_pos.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/pcnet.c | 55 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 25 deletions(-)

diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
index d344c15..f409b92 100644
--- a/hw/net/pcnet.c
+++ b/hw/net/pcnet.c
@@ -1212,7 +1212,7 @@ static void pcnet_transmit(PCNetState *s)
     hwaddr xmit_cxda = 0;
     int count = CSR_XMTRL(s)-1;
     int add_crc = 0;
-
+    int bcnt;
     s->xmit_pos = -1;
 
     if (!CSR_TXON(s)) {
@@ -1247,35 +1247,40 @@ static void pcnet_transmit(PCNetState *s)
             s->xmit_pos = -1;
             goto txdone;
         }
+
+        if (s->xmit_pos < 0) {
+            goto txdone;
+        }
+
+        bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT);
+        s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr),
+                         s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s));
+        s->xmit_pos += bcnt;
+        
         if (!GET_FIELD(tmd.status, TMDS, ENP)) {
-            int bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT);
-            s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr),
-                             s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s));
-            s->xmit_pos += bcnt;
-        } else if (s->xmit_pos >= 0) {
-            int bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT);
-            s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr),
-                             s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s));
-            s->xmit_pos += bcnt;
+            goto txdone;
+        }
+
 #ifdef PCNET_DEBUG
-            printf("pcnet_transmit size=%d\n", s->xmit_pos);
+        printf("pcnet_transmit size=%d\n", s->xmit_pos);
 #endif
-            if (CSR_LOOP(s)) {
-                if (BCR_SWSTYLE(s) == 1)
-                    add_crc = !GET_FIELD(tmd.status, TMDS, NOFCS);
-                s->looptest = add_crc ? PCNET_LOOPTEST_CRC : PCNET_LOOPTEST_NOCRC;
-                pcnet_receive(qemu_get_queue(s->nic), s->buffer, s->xmit_pos);
-                s->looptest = 0;
-            } else
-                if (s->nic)
-                    qemu_send_packet(qemu_get_queue(s->nic), s->buffer,
-                                     s->xmit_pos);
-
-            s->csr[0] &= ~0x0008;   /* clear TDMD */
-            s->csr[4] |= 0x0004;    /* set TXSTRT */
-            s->xmit_pos = -1;
+        if (CSR_LOOP(s)) {
+            if (BCR_SWSTYLE(s) == 1)
+                add_crc = !GET_FIELD(tmd.status, TMDS, NOFCS);
+            s->looptest = add_crc ? PCNET_LOOPTEST_CRC : PCNET_LOOPTEST_NOCRC;
+            pcnet_receive(qemu_get_queue(s->nic), s->buffer, s->xmit_pos);
+            s->looptest = 0;
+        } else {
+            if (s->nic) {
+                qemu_send_packet(qemu_get_queue(s->nic), s->buffer,
+                                 s->xmit_pos);
+            }
         }
 
+        s->csr[0] &= ~0x0008;   /* clear TDMD */
+        s->csr[4] |= 0x0004;    /* set TXSTRT */
+        s->xmit_pos = -1;
+
     txdone:
         SET_FIELD(&tmd.status, TMDS, OWN, 0);
         TMDSTORE(&tmd, PHYSADDR(s,CSR_CXDA(s)));
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 for-2.2 4/4] rtl8139: fix Pointer to local outside scope

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Coverity spot:
 Assigning: iov = struct iovec [3]({{buf, 12UL},
                       {(void *)dot1q_buf, 4UL},
                       {buf + 12, size - 12}})
 (address of temporary variable of type struct iovec [3]).
 out_of_scope: Temporary variable of type struct iovec [3] goes out of scope.

Pointer to local outside scope (RETURN_LOCAL)
use_invalid:
 Using iov, which points to an out-of-scope temporary variable of type struct iovec [3].

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/net/rtl8139.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 8b8a1b1..5f0197c 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -1775,6 +1775,7 @@ static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size,
     int do_interrupt, const uint8_t *dot1q_buf)
 {
     struct iovec *iov = NULL;
+    struct iovec vlan_iov[3];
 
     if (!size)
     {
@@ -1789,6 +1790,9 @@ static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size,
             { .iov_base = buf + ETHER_ADDR_LEN * 2,
                 .iov_len = size - ETHER_ADDR_LEN * 2 },
         };
+
+        memcpy(vlan_iov, iov, sizeof(vlan_iov));
+        iov = vlan_iov;
     }
 
     if (TxLoopBack == (s->TxConfig & TxLoopBack))
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v2 for-2.2 0/4] net: fix high impact outstanding defects reported by Coverity

[Paolo Bonzini]

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks!

Paolo

On 20/11/2014 12:34, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Please see details in every patch.
> 
> v2 -> v1:
>  - rewrite patch 3 and patch 4 by Paolo's suggestion. Thanks.
>  - add Jason's R-b tag in patch 1~3. Thanks too.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> 
> Gonglei (4):
>   net/slirp: fix memory leak
>   net/socket: fix Uninitialized scalar variable
>   pcnet: fix Negative array index read
>   rtl8139: fix Pointer to local outside scope
> 
>  hw/net/pcnet.c   | 55 ++++++++++++++++++++++++++++++-------------------------
>  hw/net/rtl8139.c |  4 ++++
>  net/slirp.c      |  3 +--
>  net/socket.c     | 11 ++++++-----
>  4 files changed, 41 insertions(+), 32 deletions(-)
> 

Re: [Qemu-devel] [PATCH v2 for-2.2 3/4] pcnet: fix Negative array index read

[Jason Wang]

On 11/20/2014 07:35 PM, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
>
> s->xmit_pos maybe assigned to a negative value (-1),
> but in this branch variable s->xmit_pos as an index to
> array s->buffer. Let's add a check for s->xmit_pos.
>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/net/pcnet.c | 55 ++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 30 insertions(+), 25 deletions(-)
>
> diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
> index d344c15..f409b92 100644
> --- a/hw/net/pcnet.c
> +++ b/hw/net/pcnet.c
> @@ -1212,7 +1212,7 @@ static void pcnet_transmit(PCNetState *s)
>      hwaddr xmit_cxda = 0;
>      int count = CSR_XMTRL(s)-1;
>      int add_crc = 0;
> -
> +    int bcnt;
>      s->xmit_pos = -1;
>  
>      if (!CSR_TXON(s)) {
> @@ -1247,35 +1247,40 @@ static void pcnet_transmit(PCNetState *s)
>              s->xmit_pos = -1;
>              goto txdone;
>          }
> +
> +        if (s->xmit_pos < 0) {
> +            goto txdone;
> +        }
> +
> +        bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT);
> +        s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr),
> +                         s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s));
> +        s->xmit_pos += bcnt;
> +        
>          if (!GET_FIELD(tmd.status, TMDS, ENP)) {
> -            int bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT);
> -            s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr),
> -                             s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s));
> -            s->xmit_pos += bcnt;
> -        } else if (s->xmit_pos >= 0) {
> -            int bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT);
> -            s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr),
> -                             s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s));
> -            s->xmit_pos += bcnt;
> +            goto txdone;
> +        }
> +
>  #ifdef PCNET_DEBUG
> -            printf("pcnet_transmit size=%d\n", s->xmit_pos);
> +        printf("pcnet_transmit size=%d\n", s->xmit_pos);
>  #endif
> -            if (CSR_LOOP(s)) {
> -                if (BCR_SWSTYLE(s) == 1)
> -                    add_crc = !GET_FIELD(tmd.status, TMDS, NOFCS);
> -                s->looptest = add_crc ? PCNET_LOOPTEST_CRC : PCNET_LOOPTEST_NOCRC;
> -                pcnet_receive(qemu_get_queue(s->nic), s->buffer, s->xmit_pos);
> -                s->looptest = 0;
> -            } else
> -                if (s->nic)
> -                    qemu_send_packet(qemu_get_queue(s->nic), s->buffer,
> -                                     s->xmit_pos);
> -
> -            s->csr[0] &= ~0x0008;   /* clear TDMD */
> -            s->csr[4] |= 0x0004;    /* set TXSTRT */
> -            s->xmit_pos = -1;
> +        if (CSR_LOOP(s)) {
> +            if (BCR_SWSTYLE(s) == 1)
> +                add_crc = !GET_FIELD(tmd.status, TMDS, NOFCS);
> +            s->looptest = add_crc ? PCNET_LOOPTEST_CRC : PCNET_LOOPTEST_NOCRC;
> +            pcnet_receive(qemu_get_queue(s->nic), s->buffer, s->xmit_pos);
> +            s->looptest = 0;
> +        } else {
> +            if (s->nic) {
> +                qemu_send_packet(qemu_get_queue(s->nic), s->buffer,
> +                                 s->xmit_pos);
> +            }
>          }
>  
> +        s->csr[0] &= ~0x0008;   /* clear TDMD */
> +        s->csr[4] |= 0x0004;    /* set TXSTRT */
> +        s->xmit_pos = -1;
> +
>      txdone:
>          SET_FIELD(&tmd.status, TMDS, OWN, 0);
>          TMDSTORE(&tmd, PHYSADDR(s,CSR_CXDA(s)));

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

Re: [Qemu-devel] [PATCH v2 for-2.2 4/4] rtl8139: fix Pointer to local outside scope

[Jason Wang]

On 11/20/2014 07:35 PM, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
>
> Coverity spot:
>  Assigning: iov = struct iovec [3]({{buf, 12UL},
>                        {(void *)dot1q_buf, 4UL},
>                        {buf + 12, size - 12}})
>  (address of temporary variable of type struct iovec [3]).
>  out_of_scope: Temporary variable of type struct iovec [3] goes out of scope.
>
> Pointer to local outside scope (RETURN_LOCAL)
> use_invalid:
>  Using iov, which points to an out-of-scope temporary variable of type struct iovec [3].
>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/net/rtl8139.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> index 8b8a1b1..5f0197c 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -1775,6 +1775,7 @@ static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size,
>      int do_interrupt, const uint8_t *dot1q_buf)
>  {
>      struct iovec *iov = NULL;
> +    struct iovec vlan_iov[3];
>  
>      if (!size)
>      {
> @@ -1789,6 +1790,9 @@ static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size,
>              { .iov_base = buf + ETHER_ADDR_LEN * 2,
>                  .iov_len = size - ETHER_ADDR_LEN * 2 },
>          };
> +
> +        memcpy(vlan_iov, iov, sizeof(vlan_iov));
> +        iov = vlan_iov;
>      }
>  
>      if (TxLoopBack == (s->TxConfig & TxLoopBack))

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