From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57915) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UjlZu-0008VB-Fq for qemu-devel@nongnu.org; Tue, 04 Jun 2013 03:22:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UjlZt-0006Mx-Bz for qemu-devel@nongnu.org; Tue, 04 Jun 2013 03:22:46 -0400 Received: from e34.co.us.ibm.com ([32.97.110.152]:39955) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UjlL8-0005rV-EP for qemu-devel@nongnu.org; Tue, 04 Jun 2013 03:07:30 -0400 Received: from /spool/local by e34.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 4 Jun 2013 01:07:28 -0600 Received: from d03relay03.boulder.ibm.com (d03relay03.boulder.ibm.com [9.17.195.228]) by d03dlp03.boulder.ibm.com (Postfix) with ESMTP id 8BB1D19D8042 for ; Tue, 4 Jun 2013 01:07:16 -0600 (MDT) Received: from d03av04.boulder.ibm.com (d03av04.boulder.ibm.com [9.17.195.170]) by d03relay03.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r5477RBQ112886 for ; Tue, 4 Jun 2013 01:07:27 -0600 Received: from d03av04.boulder.ibm.com (loopback [127.0.0.1]) by d03av04.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r5477Oal001247 for ; Tue, 4 Jun 2013 01:07:24 -0600 Message-ID: <51AD9226.4010900@linux.vnet.ibm.com> Date: Tue, 04 Jun 2013 02:07:18 -0500 From: Jesse Larrew MIME-Version: 1.0 References: <1370272838-15373-1-git-send-email-drjones@redhat.com> In-Reply-To: <1370272838-15373-1-git-send-email-drjones@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] e1000: cleanup process_tx_desc List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andrew Jones Cc: aliguori@us.ibm.com, mst@redhat.com, jasowang@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, pbonzini@redhat.com On 06/03/2013 10:20 AM, Andrew Jones wrote: > Coverity complains about two overruns in process_tx_desc(). The > complaints are false positives, but we might as well eliminate > them. The problem is that "hdr" is defined as an unsigned int, > but then used to offset an array of size 65536, and another of > size 256 bytes. hdr will actually never be greater than 255 > though, as it's assigned only once and to the value of > tp->hdr_len, which is an uint8_t. This patch simply gets rid of > hdr, replacing it with tp->hdr_len, which makes it consistent > with all other tp member use in the function. > > Signed-off-by: Andrew Jones > --- > hw/net/e1000.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > The logic looks sound, but checkpatch detects some style issues. See below. > diff --git a/hw/net/e1000.c b/hw/net/e1000.c > index e6f46f0c511e8..eec3e7a4524d1 100644 > --- a/hw/net/e1000.c > +++ b/hw/net/e1000.c > @@ -556,7 +556,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp) > uint32_t txd_lower = le32_to_cpu(dp->lower.data); > uint32_t dtype = txd_lower & (E1000_TXD_CMD_DEXT | E1000_TXD_DTYP_D); > unsigned int split_size = txd_lower & 0xffff, bytes, sz, op; > - unsigned int msh = 0xfffff, hdr = 0; > + unsigned int msh = 0xfffff; > uint64_t addr; > struct e1000_context_desc *xp = (struct e1000_context_desc *)dp; > struct e1000_tx *tp = &s->tx; > @@ -603,8 +603,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp) > > addr = le64_to_cpu(dp->buffer_addr); > if (tp->tse && tp->cptse) { > - hdr = tp->hdr_len; > - msh = hdr + tp->mss; > + msh = tp->hdr_len + tp->mss; > do { > bytes = split_size; > if (tp->size + bytes > msh) > @@ -612,14 +611,15 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp) > > bytes = MIN(sizeof(tp->data) - tp->size, bytes); > pci_dma_read(&s->dev, addr, tp->data + tp->size, bytes); > - if ((sz = tp->size + bytes) >= hdr && tp->size < hdr) > - memmove(tp->header, tp->data, hdr); > + if ((sz = tp->size + bytes) >= tp->hdr_len > + && tp->size < tp->hdr_len) > + memmove(tp->header, tp->data, tp->hdr_len); The 'if' statement above needs some braces. Checkpatch also isn't wild about the assignment inside of the conditional. > tp->size = sz; > addr += bytes; > if (sz == msh) { > xmit_seg(s); > - memmove(tp->data, tp->header, hdr); > - tp->size = hdr; > + memmove(tp->data, tp->header, tp->hdr_len); > + tp->size = tp->hdr_len; > } > } while (split_size -= bytes); > } else if (!tp->tse && tp->cptse) { > @@ -633,7 +633,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp) > > if (!(txd_lower & E1000_TXD_CMD_EOP)) > return; > - if (!(tp->tse && tp->cptse && tp->size < hdr)) > + if (!(tp->tse && tp->cptse && tp->size < tp->hdr_len)) > xmit_seg(s); Braces here as well. > tp->tso_frames = 0; > tp->sum_needed = 0; > Although the style issues were present to begin with, we may as well take the opportunity to fix them. Sincerely, Jesse Larrew Software Engineer, KVM Team IBM Linux Technology Center Phone: (512) 973-2052 (T/L: 363-2052) jlarrew@linux.vnet.ibm.com