[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Jason Wang
Subject: Re: [PATCH] hw/net/xgmac: Fix buffer overflow in xgmac_enet_send()
Date: Mon, 20 Jul 2020 17:27:48 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0

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, 
               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

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.


-- PMM

reply via email to

[Prev in Thread] Current Thread [Next in Thread]