On Tue, 14 Jul 2020 at 10:09, Jason Wang <jasow...@redhat.com> wrote:
>
>
> On 2020/7/10 下午7:07, Peter Maydell wrote:
> > 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 ?
> 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.mayd...@linaro.org>
Were you planning to take this patch, Jason, or should I?
thanks
-- PMM
