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

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

[Mauro Matteo Cascella]

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);
-- 
2.26.2

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

[Peter Maydell]

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

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

[Jason Wang]

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


>

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

[Peter Maydell]

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?

thanks
-- PMM

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

[Jason Wang]

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
>