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

[Jason Wang <jasowang@redhat.com>]

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>