| Message ID | 20200710091940.641296-1-mcascell@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/net/xgmac: Fix buffer overflow in xgmac_enet_send() | expand |
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
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 ? > > thanks > -- PMM 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? Thanks [1] https://www.synopsys.com/dw/ipdir.php?ds=dwc_ether_xgmac >
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
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 >
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);
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(-)