[RFC PATCH] rtl8139: fix stack overflow if RxBuf overlaps MMIO

[RFC PATCH] rtl8139: fix stack overflow if RxBuf overlaps MMIO

[Qiuhao Li]

Fix Bug 1910826 [1] / OSS-Fuzz Issue 29224 [2].

In rtl8139.c, the function rtl8139_RxBuf_write, which sets the RxBuf
(Receive Buffer Start Address), doesn't check if this buffer overlaps our
MMIO region. So if the guest machine set the transmit mode to loopback, put
the RxBuf at the address of TSD (Transmit Status of Descriptor, MMIO), and
trigger a frame transfer by directly writing to the TSD, an infinite
recursion will occur:

rtl8139_ioport_write (to TSD) -> rtl8139_io_writel -> rtl8139_transmit ->
rtl8139_transmit_one -> rtl8139_transfer_frame -> rtl8139_do_receive ->
rtl8139_write_buffer -> pci_dma_write (to TSD) -> ... ->
rtl8139_ioport_write (to TSD)

This patch adds a check to ensure the maximum possible RxBuf [3] won't
overlap the MMIO region.

P.S. There is a more concise reproducer with comments [4], which may help :)

[1] https://bugs.launchpad.net/bugs/1910826
[2] https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29224
[3] https://www.cs.usfca.edu/~cruse/cs326f04/RTL8139D_DataSheet.pdf
    5.7 Transmit Configuration Register
[4] https://bugs.launchpad.net/qemu/+bug/1910826/comments/1

Signed-off-by: Qiuhao Li <Qiuhao.Li@outlook.com>
Reported-by: Alexander Bulekov <alxndr@bu.edu>
---
 hw/net/rtl8139.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index ba5ace1ab7..aa7a38220d 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -2567,6 +2567,18 @@ static void rtl8139_RxBuf_write(RTL8139State *s, uint32_t val)
 {
     DPRINTF("RxBuf write val=0x%08x\n", val);
 
+    PCIDevice *d = PCI_DEVICE(s);
+    uint64_t mmio_addr = d->io_regions[1].addr;
+    uint64_t mmio_size = d->io_regions[1].size;
+
+    #define MAX_Rx_BUFFER_LENGTH (64 * 1024 + 16) /* RxConfig 12-11 = 0b11 */
+
+    if (val < mmio_addr + mmio_size && val + MAX_Rx_BUFFER_LENGTH > mmio_addr) {
+        DPRINTF("The receive buffer may overlap with the MMIO region.\n");
+        DPRINTF("mmio_addr: 0x%lx, mmio_size: 0x%lx\n", mmio_addr, mmio_size);
+        return;
+    }
+
     s->RxBuf = val;
 
     /* may need to reset rxring here */
-- 
2.25.1

Re: [RFC PATCH] rtl8139: fix stack overflow if RxBuf overlaps MMIO

[Peter Maydell]

On Tue, 12 Jan 2021 at 15:23, Qiuhao Li <Qiuhao.Li@outlook.com> wrote:
>
> Fix Bug 1910826 [1] / OSS-Fuzz Issue 29224 [2].
>
> In rtl8139.c, the function rtl8139_RxBuf_write, which sets the RxBuf
> (Receive Buffer Start Address), doesn't check if this buffer overlaps our
> MMIO region. So if the guest machine set the transmit mode to loopback, put
> the RxBuf at the address of TSD (Transmit Status of Descriptor, MMIO), and
> trigger a frame transfer by directly writing to the TSD, an infinite
> recursion will occur:
>
> rtl8139_ioport_write (to TSD) -> rtl8139_io_writel -> rtl8139_transmit ->
> rtl8139_transmit_one -> rtl8139_transfer_frame -> rtl8139_do_receive ->
> rtl8139_write_buffer -> pci_dma_write (to TSD) -> ... ->
> rtl8139_ioport_write (to TSD)
>
> This patch adds a check to ensure the maximum possible RxBuf [3] won't
> overlap the MMIO region.
>
> P.S. There is a more concise reproducer with comments [4], which may help :)
>
> [1] https://bugs.launchpad.net/bugs/1910826
> [2] https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29224
> [3] https://www.cs.usfca.edu/~cruse/cs326f04/RTL8139D_DataSheet.pdf
>     5.7 Transmit Configuration Register
> [4] https://bugs.launchpad.net/qemu/+bug/1910826/comments/1
>
> Signed-off-by: Qiuhao Li <Qiuhao.Li@outlook.com>
> Reported-by: Alexander Bulekov <alxndr@bu.edu>

This looks like a single-device workaround for the generic
class of problems where a device can be configured to
do DMA to itself. Why is rtl8139 special ?

(I have on my todo list to think about the general problem.)

thanks
-- PMM

Re: [RFC PATCH] rtl8139: fix stack overflow if RxBuf overlaps MMIO

[Qiuhao Li]

On Tue, 2021-01-12 at 16:02 +0000, Peter Maydell wrote:
> On Tue, 12 Jan 2021 at 15:23, Qiuhao Li <Qiuhao.Li@outlook.com>
> wrote:
> > Fix Bug 1910826 [1] / OSS-Fuzz Issue 29224 [2].
> > 
> > In rtl8139.c, the function rtl8139_RxBuf_write, which sets the
> > RxBuf
> > (Receive Buffer Start Address), doesn't check if this buffer
> > overlaps our
> > MMIO region. So if the guest machine set the transmit mode to
> > loopback, put
> > the RxBuf at the address of TSD (Transmit Status of Descriptor,
> > MMIO), and
> > trigger a frame transfer by directly writing to the TSD, an
> > infinite
> > recursion will occur:
> > 
> > rtl8139_ioport_write (to TSD) -> rtl8139_io_writel ->
> > rtl8139_transmit ->
> > rtl8139_transmit_one -> rtl8139_transfer_frame ->
> > rtl8139_do_receive ->
> > rtl8139_write_buffer -> pci_dma_write (to TSD) -> ... ->
> > rtl8139_ioport_write (to TSD)
> > 
> > This patch adds a check to ensure the maximum possible RxBuf [3]
> > won't
> > overlap the MMIO region.
> > 
> > P.S. There is a more concise reproducer with comments [4], which
> > may help :)
> > 
> > [1] https://bugs.launchpad.net/bugs/1910826
> > [2] https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29224
> > [3] https://www.cs.usfca.edu/~cruse/cs326f04/RTL8139D_DataSheet.pdf
> >     5.7 Transmit Configuration Register
> > [4] https://bugs.launchpad.net/qemu/+bug/1910826/comments/1
> > 
> > Signed-off-by: Qiuhao Li <Qiuhao.Li@outlook.com>
> > Reported-by: Alexander Bulekov <alxndr@bu.edu>
> 
> This looks like a single-device workaround for the generic
> class of problems where a device can be configured to
> do DMA to itself. Why is rtl8139 special ?

Understand. I thought it is the device's duty to avoid doing DMA to
itself.

Thank you.
  Qiuhao Li
> 
> (I have on my todo list to think about the general problem.)
> 
> thanks
> -- PMM