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

[Peter Maydell <peter.maydell@linaro.org>]

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