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

[Gonglei]

On 2014/11/20 14:36, Paolo Bonzini wrote:

> 
> 
> On 20/11/2014 06:57, address@hidden wrote:
>> From: Gonglei <address@hidden>
>>
>> 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 <address@hidden>
>> ---
>>  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.

Maybe not, since two branch are "if and else if" not "if and else",
so this change make the below code segment's wide ...
>     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;
... more extensive.

Best regards,
-Gonglei

>  But the call is
> Stefan's, as he's the maintainer.
> 
> Paolo