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