From d1781652dc7039b2ac92fa80695091b97dbf1918 Mon Sep 17 00:00:00 2001
From: orion cai <orion@orions-MacBook-Air.local>
Date: Sat, 24 Jan 2026 11:19:17 +0800
Subject: [PATCH] hw/net/rtl8139: Fix integer overflow in rtl8139_write_buffer

Fix integer overflow vulnerability in rtl8139_write_buffer() that could allow
a malicious guest to bypass DMA bounds checks and write to arbitrary memory
locations.

The vulnerability occurs when:
- RxBufAddr is near UINT_MAX (e.g., 0xFFFF0000)
- size is a moderate value (e.g., 0x20000)
- The addition RxBufAddr + size overflows and wraps to a small value
- This bypasses the bounds check (s->RxBufAddr + size > s->RxBufferSize)

Fix uses subtraction instead of addition for the bounds check and applies
modulo to each term individually before computing the wrapped position to
prevent overflow in all code paths.

Signed-off-by: orionhubble@163.com
---
 hw/net/rtl8139.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 9fd00574d2..41b8407acc 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -745,9 +745,16 @@ static void rtl8139_write_buffer(RTL8139State *s, const void *buf, int size)
 {
     PCIDevice *d = PCI_DEVICE(s);
 
-    if (s->RxBufAddr + size > s->RxBufferSize)
+    if (size < 0 || s->RxBufAddr > s->RxBufferSize - (uint32_t)size)
     {
-        int wrapped = MOD2(s->RxBufAddr + size, s->RxBufferSize);
+        /* Calculate wrapped position safely without overflow.
+         * Use modulo on each term to prevent overflow.
+         * RxBufferSize is always a power of 2, so MOD2 is safe. */
+        int wrapped = MOD2((uint32_t)s->RxBufAddr, s->RxBufferSize) +
+                    MOD2((uint32_t)size, s->RxBufferSize);
+        if (wrapped >= s->RxBufferSize) {
+            wrapped -= s->RxBufferSize;
+        }
 
         /* write packet data */
         if (wrapped && !(s->RxBufferSize < 65536 && rtl8139_RxWrap(s)))
-- 
2.50.1 (Apple Git-155)

On Sat, 24 Jan 2026 at 06:10, orion cai <orion@orions-macbook-air.local> wrote:
>
> From d1781652dc7039b2ac92fa80695091b97dbf1918 Mon Sep 17 00:00:00 2001
> From: orion cai <orion@orions-MacBook-Air.local>
> Date: Sat, 24 Jan 2026 11:19:17 +0800
> Subject: [PATCH] hw/net/rtl8139: Fix integer overflow in rtl8139_write_buffer

Hi; thanks for this bug report and patch.

> Fix integer overflow vulnerability in rtl8139_write_buffer() that could allow
> a malicious guest to bypass DMA bounds checks and write to arbitrary memory
> locations.

For clarity, this is an arbitrary write to guest memory,
not to host (QEMU) memory, since we're doing a pci_dma_write(),
correct ?

That renders it a bit less critical, because if you can
program the RTL8139 then you almost certainly already
have kernel level privilege in the guest and can write
to arbitrary guest memory directly if you want...

> - RxBufAddr is near UINT_MAX (e.g., 0xFFFF0000)

How can this happen? RxBufAddr is the device's
"Current Buffer Address" register, which is supposed to
be a 16-bit read-only register indicating the total
received byte count in the buffer. If it has ever got
larger than the size of the buffer, then I think we have
a bug somewhere else, and the correct fix is to make
sure we are not letting it get set incorrectly (with
perhaps an assertion in case of other bugs).

Do you have a repro case that triggers this bug? The
ideal here is a qtest repro case which gives the set
of memory writes etc needed to trigger it; for an example
of what that looks like see e.g. this bug:
https://gitlab.com/qemu-project/qemu/-/issues/2777
A guest binary that shows the problem would be OK too.

> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> index 9fd00574d2..41b8407acc 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -745,9 +745,16 @@ static void rtl8139_write_buffer(RTL8139State *s, const void *buf, int size)
>  {
>      PCIDevice *d = PCI_DEVICE(s);
>
> -    if (s->RxBufAddr + size > s->RxBufferSize)
> +    if (size < 0 || s->RxBufAddr > s->RxBufferSize - (uint32_t)size)

Is it possible to get here with a negative size ?
This is supposed to be the packet size, so if we get here
and it is negative something has gone badly wrong in the
caller. I think it would be better to assert that size >= 0
at the top of the function.

>      {
> -        int wrapped = MOD2(s->RxBufAddr + size, s->RxBufferSize);
> +        /* Calculate wrapped position safely without overflow.
> +         * Use modulo on each term to prevent overflow.
> +         * RxBufferSize is always a power of 2, so MOD2 is safe. */
> +        int wrapped = MOD2((uint32_t)s->RxBufAddr, s->RxBufferSize) +
> +                    MOD2((uint32_t)size, s->RxBufferSize);
> +        if (wrapped >= s->RxBufferSize) {
> +            wrapped -= s->RxBufferSize;
> +        }

When does this produce a different result to the existing code,
given that RxBufferSize is a power of 2 ? (For QEMU, we
always use -fwrapv, so we do not need to worry about a
signed integer calculation overflowing, as long as it
still produces the right answer, which I think it does here
if I'm not getting confused.)

>          /* write packet data */
>          if (wrapped && !(s->RxBufferSize < 65536 && rtl8139_RxWrap(s)))
> --

thanks
-- PMM

On 1/24/26 06:21, orion cai wrote:
>  From d1781652dc7039b2ac92fa80695091b97dbf1918 Mon Sep 17 00:00:00 2001
> From: orion cai <orion@orions-MacBook-Air.local>
> Date: Sat, 24 Jan 2026 11:19:17 +0800
> Subject: [PATCH] hw/net/rtl8139: Fix integer overflow in rtl8139_write_buffer
> 
> Fix integer overflow vulnerability in rtl8139_write_buffer() that could allow
> a malicious guest to bypass DMA bounds checks and write to arbitrary memory
> locations.
> 
> The vulnerability occurs when:
> - RxBufAddr is near UINT_MAX (e.g., 0xFFFF0000)
> - size is a moderate value (e.g., 0x20000)
> - The addition RxBufAddr + size overflows and wraps to a small value
> - This bypasses the bounds check (s->RxBufAddr + size > s->RxBufferSize)
> 
> Fix uses subtraction instead of addition for the bounds check and applies
> modulo to each term individually before computing the wrapped position to
> prevent overflow in all code paths.
> 
> Signed-off-by: orionhubble@163.com
> ---
>   hw/net/rtl8139.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> index 9fd00574d2..41b8407acc 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -745,9 +745,16 @@ static void rtl8139_write_buffer(RTL8139State *s, const void *buf, int size)
>   {
>       PCIDevice *d = PCI_DEVICE(s);
>   
> -    if (s->RxBufAddr + size > s->RxBufferSize)
> +    if (size < 0 || s->RxBufAddr > s->RxBufferSize - (uint32_t)size)

Can size be > RxBufferSize here?

Thanks,

/mjt