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