Re: [PATCH] hw/net/xgmac: Fix buffer overflow in xgmac_enet_send()

[Jason Wang <jasowang@redhat.com>]

On 2020/7/10 下午7:07, Peter Maydell wrote:
> On Fri, 10 Jul 2020 at 10:20, Mauro Matteo Cascella <mcascell@redhat.com> wrote:
>> A buffer overflow issue was reported by Mr. Ziming Zhang, CC'd here. It
>> occurs while sending an Ethernet frame due to missing break statements
>> and improper checking of the buffer size.
>>
>> Reported-by: Ziming Zhang <ezrakiez@gmail.com>
>> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
>> ---
>>   hw/net/xgmac.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/net/xgmac.c b/hw/net/xgmac.c
>> index 574dd47b41..b872afbb1a 100644
>> --- a/hw/net/xgmac.c
>> +++ b/hw/net/xgmac.c
>> @@ -224,17 +224,20 @@ static void xgmac_enet_send(XgmacState *s)
>>               DEBUGF_BRK("qemu:%s:ERROR...ERROR...ERROR... -- "
>>                           "xgmac buffer 1 len on send > 2048 (0x%x)\n",
>>                            __func__, bd.buffer1_size & 0xfff);
>> +            break;
>>           }
>>           if ((bd.buffer2_size & 0xfff) != 0) {
>>               DEBUGF_BRK("qemu:%s:ERROR...ERROR...ERROR... -- "
>>                           "xgmac buffer 2 len on send != 0 (0x%x)\n",
>>                           __func__, bd.buffer2_size & 0xfff);
>> +            break;
>>           }
>> -        if (len >= sizeof(frame)) {
>> +        if (frame_size + len >= sizeof(frame)) {
>>               DEBUGF_BRK("qemu:%s: buffer overflow %d read into %zu "
>> -                        "buffer\n" , __func__, len, sizeof(frame));
>> +                        "buffer\n" , __func__, frame_size + len, sizeof(frame));
>>               DEBUGF_BRK("qemu:%s: buffer1.size=%d; buffer2.size=%d\n",
>>                           __func__, bd.buffer1_size, bd.buffer2_size);
>> +            break;
>>           }
>>
>>           cpu_physical_memory_read(bd.buffer1_addr, ptr, len);
> This is correct in the sense that it avoids the buffer overflow.
>
> I suspect that we should probably also be reporting the error
> back to the guest via some kind of error flag in the descriptor
> and/or in a status register. Unfortunately I don't have a copy
> of the datasheet and it doesn't seem to be available online :-(
> Does anybody have a copy to check ?
>
> thanks
> -- PMM


I tried to download the datasheet from [1] but it's not a programmer 
manual.

I think we can apply this patch first and do follow-up fixes on top?

Thanks

[1] https://www.synopsys.com/dw/ipdir.php?ds=dwc_ether_xgmac


>