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

[Paolo Bonzini <pbonzini@redhat.com>]

On 20/11/2014 06:57, 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>
> ---
>  hw/net/pcnet.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
> index d344c15..2bb6417 100644
> --- a/hw/net/pcnet.c
> +++ b/hw/net/pcnet.c
> @@ -1247,7 +1247,7 @@ static void pcnet_transmit(PCNetState *s)
>              s->xmit_pos = -1;
>              goto txdone;
>          }
> -        if (!GET_FIELD(tmd.status, TMDS, ENP)) {
> +        if (!GET_FIELD(tmd.status, TMDS, ENP) && (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));
> 

Even better:

    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 (FIELD(tmd.status, TMDS, ENP)) {
#ifdef PCNET_DEBUG
        printf("pcnet_transmit size=%d\n", s->xmit_pos);
#endif
        if (CSR_LOOP(s)) {
            if (BCR_SWSTYLE(s) == 1)
        ...
    }

since there is duplicated code in the two "if" arms.  But the call is
Stefan's, as he's the maintainer.

Paolo