On Fri, 10 Jul 2020 at 10:20, Mauro Matteo Cascella <mcasc...@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 <ezrak...@gmail.com>
> Signed-off-by: Mauro Matteo Cascella <mcasc...@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
