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

[Jason Wang <jasowang@redhat.com>]

On 2020/7/20 下午5:14, Peter Maydell wrote:
> On Tue, 14 Jul 2020 at 10:09, Jason Wang <jasowang@redhat.com> wrote:
>>
>> 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 ?
>> 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?
> Yes, agreed, since nobody seems to have docs for this device.
> This is an only-happens-with-malicious-or-buggy-guest case anyway,
> and the highbank/midway (only users of the device) aren't important
> or widely used machine types any more.
>
> I suppose we could add a comment
> /*
>   * FIXME: these cases of malformed tx descriptors (bad sizes)
>   * should probably be reported back to the guest somehow
>   * rather than simply silently stopping processing, but we
>   * don't know what the hardware does in this situation.
>   * This will only happen for buggy guests anyway.
>   */
>
> but anyway
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> Were you planning to take this patch, Jason, or should I?


I've queued for rc1 this with the above suggested comment.

Thanks


>
> thanks
> -- PMM
>