[PATCH v6 0/7] net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr()

[PATCH v6 0/7] net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr()

[Philippe Mathieu-Daudé]

I had a look at the patch from Miroslav trying to silence a
compiler warning which in fact is a nasty bug. Here is a fix.
https://www.mail-archive.com/qemu-devel@nongnu.org/msg772735.html

Since v5:
- addressed Stefano's review comments:
- add now patch fixing in6_address offset

Since v4:
- reworked again, tested it with Fedora Raw Hide

Philippe Mathieu-Daudé (7):
  net/eth: Use correct in6_address offset in _eth_get_rss_ex_dst_addr()
  net/eth: Simplify _eth_get_rss_ex_dst_addr()
  net/eth: Better describe _eth_get_rss_ex_dst_addr's offset argument
  net/eth: Check size earlier in _eth_get_rss_ex_dst_addr()
  net/eth: Check iovec has enough data earlier
  net/eth: Read ip6_ext_hdr_routing buffer before accessing it
  net/eth: Add an assert() and invert if() statement to simplify code

 net/eth.c                      | 46 ++++++++++++++---------------
 tests/qtest/fuzz-e1000e-test.c | 53 ++++++++++++++++++++++++++++++++++
 MAINTAINERS                    |  1 +
 tests/qtest/meson.build        |  1 +
 4 files changed, 78 insertions(+), 23 deletions(-)
 create mode 100644 tests/qtest/fuzz-e1000e-test.c

-- 
2.26.2

[PATCH v6 1/7] net/eth: Use correct in6_address offset in _eth_get_rss_ex_dst_addr()

[Philippe Mathieu-Daudé]

The in6_address comes after the ip6_ext_hdr_routing header,
not after the ip6_ext_hdr one. Fix the offset.

Cc: qemu-stable@nongnu.org
Reported-by: Stefano Garzarella <sgarzare@redhat.com>
Fixes: eb700029c78 ("net_pkt: Extend packet abstraction as required by e1000e functionality")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 net/eth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/eth.c b/net/eth.c
index 1e0821c5f81..ef1b5136f1c 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -419,7 +419,7 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags,
         }
 
         bytes_read = iov_to_buf(pkt, pkt_frags,
-                                rthdr_offset + sizeof(*ext_hdr),
+                                rthdr_offset + sizeof(*rthdr),
                                 dst_addr, sizeof(*dst_addr));
 
         return bytes_read == sizeof(*dst_addr);
-- 
2.26.2

Re: [PATCH v6 1/7] net/eth: Use correct in6_address offset in _eth_get_rss_ex_dst_addr()

[Stefano Garzarella]

On Wed, Mar 10, 2021 at 07:31:17PM +0100, Philippe Mathieu-Daudé wrote:
>The in6_address comes after the ip6_ext_hdr_routing header,
>not after the ip6_ext_hdr one. Fix the offset.
>
>Cc: qemu-stable@nongnu.org
>Reported-by: Stefano Garzarella <sgarzare@redhat.com>
>Fixes: eb700029c78 ("net_pkt: Extend packet abstraction as required by e1000e functionality")
>Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>---
> net/eth.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

[PATCH v6 2/7] net/eth: Simplify _eth_get_rss_ex_dst_addr()

[Philippe Mathieu-Daudé]

The length field is already contained in the ip6_ext_hdr structure.
Check it direcly in eth_parse_ipv6_hdr() before calling
_eth_get_rss_ex_dst_addr(), which gets a bit simplified.

Reviewed-by: Miroslav Rezanina <mrezanin@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 net/eth.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/eth.c b/net/eth.c
index ef1b5136f1c..b9ceb30a194 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -407,9 +407,7 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags,
 {
     struct ip6_ext_hdr_routing *rthdr = (struct ip6_ext_hdr_routing *) ext_hdr;
 
-    if ((rthdr->rtype == 2) &&
-        (rthdr->len == sizeof(struct in6_address) / 8) &&
-        (rthdr->segleft == 1)) {
+    if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) {
 
         size_t input_size = iov_size(pkt, pkt_frags);
         size_t bytes_read;
@@ -528,10 +526,12 @@ bool eth_parse_ipv6_hdr(const struct iovec *pkt, int pkt_frags,
         }
 
         if (curr_ext_hdr_type == IP6_ROUTING) {
-            info->rss_ex_dst_valid =
-                _eth_get_rss_ex_dst_addr(pkt, pkt_frags,
-                                         ip6hdr_off + info->full_hdr_len,
-                                         &ext_hdr, &info->rss_ex_dst);
+            if (ext_hdr.ip6r_len == sizeof(struct in6_address) / 8) {
+                info->rss_ex_dst_valid =
+                    _eth_get_rss_ex_dst_addr(pkt, pkt_frags,
+                                             ip6hdr_off + info->full_hdr_len,
+                                             &ext_hdr, &info->rss_ex_dst);
+            }
         } else if (curr_ext_hdr_type == IP6_DESTINATON) {
             info->rss_ex_src_valid =
                 _eth_get_rss_ex_src_addr(pkt, pkt_frags,
-- 
2.26.2

[PATCH v6 3/7] net/eth: Better describe _eth_get_rss_ex_dst_addr's offset argument

[Philippe Mathieu-Daudé]

The 'offset' argument represents the offset to the ip6_ext_hdr
header, rename it as 'ext_hdr_offset'.

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 net/eth.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/eth.c b/net/eth.c
index b9ceb30a194..ed047b5760b 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -401,7 +401,7 @@ eth_is_ip6_extension_header_type(uint8_t hdr_type)
 
 static bool
 _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags,
-                        size_t rthdr_offset,
+                        size_t ext_hdr_offset,
                         struct ip6_ext_hdr *ext_hdr,
                         struct in6_address *dst_addr)
 {
@@ -412,12 +412,12 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags,
         size_t input_size = iov_size(pkt, pkt_frags);
         size_t bytes_read;
 
-        if (input_size < rthdr_offset + sizeof(*ext_hdr)) {
+        if (input_size < ext_hdr_offset + sizeof(*ext_hdr)) {
             return false;
         }
 
         bytes_read = iov_to_buf(pkt, pkt_frags,
-                                rthdr_offset + sizeof(*rthdr),
+                                ext_hdr_offset + sizeof(*rthdr),
                                 dst_addr, sizeof(*dst_addr));
 
         return bytes_read == sizeof(*dst_addr);
-- 
2.26.2

[PATCH v6 4/7] net/eth: Check size earlier in _eth_get_rss_ex_dst_addr()

[Philippe Mathieu-Daudé]

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 net/eth.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/net/eth.c b/net/eth.c
index ed047b5760b..492359afbbb 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -406,16 +406,14 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags,
                         struct in6_address *dst_addr)
 {
     struct ip6_ext_hdr_routing *rthdr = (struct ip6_ext_hdr_routing *) ext_hdr;
+    size_t input_size = iov_size(pkt, pkt_frags);
+    size_t bytes_read;
+
+    if (input_size < ext_hdr_offset + sizeof(*ext_hdr)) {
+        return false;
+    }
 
     if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) {
-
-        size_t input_size = iov_size(pkt, pkt_frags);
-        size_t bytes_read;
-
-        if (input_size < ext_hdr_offset + sizeof(*ext_hdr)) {
-            return false;
-        }
-
         bytes_read = iov_to_buf(pkt, pkt_frags,
                                 ext_hdr_offset + sizeof(*rthdr),
                                 dst_addr, sizeof(*dst_addr));
-- 
2.26.2

[PATCH v6 5/7] net/eth: Check iovec has enough data earlier

[Philippe Mathieu-Daudé]

We want to check fields from ip6_ext_hdr_routing structure
and if correct read the full in6_address. Let's directly check
if our iovec contains enough data for everything, else return
early.

Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 net/eth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/eth.c b/net/eth.c
index 492359afbbb..b78aa526efc 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -409,7 +409,7 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags,
     size_t input_size = iov_size(pkt, pkt_frags);
     size_t bytes_read;
 
-    if (input_size < ext_hdr_offset + sizeof(*ext_hdr)) {
+    if (input_size < ext_hdr_offset + sizeof(*rthdr) + sizeof(*dst_addr)) {
         return false;
     }
 
-- 
2.26.2

[PATCH v6 6/7] net/eth: Read ip6_ext_hdr_routing buffer before accessing it

[Philippe Mathieu-Daudé]

We can't know the caller read enough data in the memory pointed
by ext_hdr to cast it as a ip6_ext_hdr_routing.
Declare rt_hdr on the stack and fill it again from the iovec.

Since we already checked there is enough data in the iovec buffer,
simply add an assert() call to consume the bytes_read variable.

This fix a 2 bytes buffer overrun in eth_parse_ipv6_hdr() reported
by QEMU fuzzer:

  $ cat << EOF | ./qemu-system-i386 -M pc-q35-5.0 \
    -accel qtest -monitor none \
    -serial none -nographic -qtest stdio
  outl 0xcf8 0x80001010
  outl 0xcfc 0xe1020000
  outl 0xcf8 0x80001004
  outw 0xcfc 0x7
  write 0x25 0x1 0x86
  write 0x26 0x1 0xdd
  write 0x4f 0x1 0x2b
  write 0xe1020030 0x4 0x190002e1
  write 0xe102003a 0x2 0x0807
  write 0xe1020048 0x4 0x12077cdd
  write 0xe1020400 0x4 0xba077cdd
  write 0xe1020420 0x4 0x190002e1
  write 0xe1020428 0x4 0x3509d807
  write 0xe1020438 0x1 0xe2
  EOF
  =================================================================
  ==2859770==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffdef904902 at pc 0x561ceefa78de bp 0x7ffdef904820 sp 0x7ffdef904818
  READ of size 1 at 0x7ffdef904902 thread T0
      #0 0x561ceefa78dd in _eth_get_rss_ex_dst_addr net/eth.c:410:17
      #1 0x561ceefa41fb in eth_parse_ipv6_hdr net/eth.c:532:17
      #2 0x561cef7de639 in net_tx_pkt_parse_headers hw/net/net_tx_pkt.c:228:14
      #3 0x561cef7dbef4 in net_tx_pkt_parse hw/net/net_tx_pkt.c:273:9
      #4 0x561ceec29f22 in e1000e_process_tx_desc hw/net/e1000e_core.c:730:29
      #5 0x561ceec28eac in e1000e_start_xmit hw/net/e1000e_core.c:927:9
      #6 0x561ceec1baab in e1000e_set_tdt hw/net/e1000e_core.c:2444:9
      #7 0x561ceebf300e in e1000e_core_write hw/net/e1000e_core.c:3256:9
      #8 0x561cef3cd4cd in e1000e_mmio_write hw/net/e1000e.c:110:5

  Address 0x7ffdef904902 is located in stack of thread T0 at offset 34 in frame
      #0 0x561ceefa320f in eth_parse_ipv6_hdr net/eth.c:486

    This frame has 1 object(s):
      [32, 34) 'ext_hdr' (line 487) <== Memory access at offset 34 overflows this variable
  HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
        (longjmp and C++ exceptions *are* supported)
  SUMMARY: AddressSanitizer: stack-buffer-overflow net/eth.c:410:17 in _eth_get_rss_ex_dst_addr
  Shadow bytes around the buggy address:
    0x10003df188d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x10003df188e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x10003df188f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x10003df18900: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x10003df18910: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
  =>0x10003df18920:[02]f3 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
    0x10003df18930: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x10003df18940: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x10003df18950: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x10003df18960: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x10003df18970: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  Shadow byte legend (one shadow byte represents 8 application bytes):
    Addressable:           00
    Partially addressable: 01 02 03 04 05 06 07
    Stack left redzone:      f1
    Stack right redzone:     f3
  ==2859770==ABORTING

Add the corresponding qtest case with the fuzzer reproducer.

FWIW GCC 11 similarly reported:

  net/eth.c: In function 'eth_parse_ipv6_hdr':
  net/eth.c:410:15: error: array subscript 'struct ip6_ext_hdr_routing[0]' is partly outside array bounds of 'struct ip6_ext_hdr[1]' [-Werror=array-bounds]
    410 |     if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) {
        |          ~~~~~^~~~~~~
  net/eth.c:485:24: note: while referencing 'ext_hdr'
    485 |     struct ip6_ext_hdr ext_hdr;
        |                        ^~~~~~~
  net/eth.c:410:38: error: array subscript 'struct ip6_ext_hdr_routing[0]' is partly outside array bounds of 'struct ip6_ext_hdr[1]' [-Werror=array-bounds]
    410 |     if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) {
        |                                 ~~~~~^~~~~~~~~
  net/eth.c:485:24: note: while referencing 'ext_hdr'
    485 |     struct ip6_ext_hdr ext_hdr;
        |                        ^~~~~~~

Cc: qemu-stable@nongnu.org
Buglink: https://bugs.launchpad.net/qemu/+bug/1879531
Reported-by: Alexander Bulekov <alxndr@bu.edu>
Reported-by: Miroslav Rezanina <mrezanin@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Fixes: eb700029c78 ("net_pkt: Extend packet abstraction as required by e1000e functionality")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 net/eth.c                      | 13 +++++----
 tests/qtest/fuzz-e1000e-test.c | 53 ++++++++++++++++++++++++++++++++++
 MAINTAINERS                    |  1 +
 tests/qtest/meson.build        |  1 +
 4 files changed, 63 insertions(+), 5 deletions(-)
 create mode 100644 tests/qtest/fuzz-e1000e-test.c

diff --git a/net/eth.c b/net/eth.c
index b78aa526efc..284ade4ab0b 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -405,17 +405,20 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags,
                         struct ip6_ext_hdr *ext_hdr,
                         struct in6_address *dst_addr)
 {
-    struct ip6_ext_hdr_routing *rthdr = (struct ip6_ext_hdr_routing *) ext_hdr;
+    struct ip6_ext_hdr_routing rt_hdr;
     size_t input_size = iov_size(pkt, pkt_frags);
     size_t bytes_read;
 
-    if (input_size < ext_hdr_offset + sizeof(*rthdr) + sizeof(*dst_addr)) {
+    if (input_size < ext_hdr_offset + sizeof(rt_hdr) + sizeof(*dst_addr)) {
         return false;
     }
 
-    if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) {
-        bytes_read = iov_to_buf(pkt, pkt_frags,
-                                ext_hdr_offset + sizeof(*rthdr),
+    bytes_read = iov_to_buf(pkt, pkt_frags, ext_hdr_offset,
+                            &rt_hdr, sizeof(rt_hdr));
+    assert(bytes_read == sizeof(rt_hdr));
+
+    if ((rt_hdr.rtype == 2) && (rt_hdr.segleft == 1)) {
+        bytes_read = iov_to_buf(pkt, pkt_frags, ext_hdr_offset + sizeof(rt_hdr),
                                 dst_addr, sizeof(*dst_addr));
 
         return bytes_read == sizeof(*dst_addr);
diff --git a/tests/qtest/fuzz-e1000e-test.c b/tests/qtest/fuzz-e1000e-test.c
new file mode 100644
index 00000000000..66229e60964
--- /dev/null
+++ b/tests/qtest/fuzz-e1000e-test.c
@@ -0,0 +1,53 @@
+/*
+ * QTest testcase for e1000e device generated by fuzzer
+ *
+ * Copyright (c) 2021 Red Hat, Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+
+#include "libqos/libqtest.h"
+
+/*
+ * https://bugs.launchpad.net/qemu/+bug/1879531
+ */
+static void test_lp1879531_eth_get_rss_ex_dst_addr(void)
+{
+    QTestState *s;
+
+    s = qtest_init("-nographic -monitor none -serial none -M pc-q35-5.0");
+
+    qtest_outl(s, 0xcf8, 0x80001010);
+    qtest_outl(s, 0xcfc, 0xe1020000);
+    qtest_outl(s, 0xcf8, 0x80001004);
+    qtest_outw(s, 0xcfc, 0x7);
+    qtest_writeb(s, 0x25, 0x86);
+    qtest_writeb(s, 0x26, 0xdd);
+    qtest_writeb(s, 0x4f, 0x2b);
+
+    qtest_writel(s, 0xe1020030, 0x190002e1);
+    qtest_writew(s, 0xe102003a, 0x0807);
+    qtest_writel(s, 0xe1020048, 0x12077cdd);
+    qtest_writel(s, 0xe1020400, 0xba077cdd);
+    qtest_writel(s, 0xe1020420, 0x190002e1);
+    qtest_writel(s, 0xe1020428, 0x3509d807);
+    qtest_writeb(s, 0xe1020438, 0xe2);
+    qtest_writeb(s, 0x4f, 0x2b);
+    qtest_quit(s);
+}
+
+int main(int argc, char **argv)
+{
+    const char *arch = qtest_get_arch();
+
+    g_test_init(&argc, &argv, NULL);
+
+    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
+        qtest_add_func("fuzz/test_lp1879531_eth_get_rss_ex_dst_addr",
+                       test_lp1879531_eth_get_rss_ex_dst_addr);
+    }
+
+    return g_test_run();
+}
diff --git a/MAINTAINERS b/MAINTAINERS
index 738786146d6..cc5f3aa6b60 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2003,6 +2003,7 @@ e1000e
 M: Dmitry Fleytman <dmitry.fleytman@gmail.com>
 S: Maintained
 F: hw/net/e1000e*
+F: tests/qtest/fuzz-e1000e-test.c
 
 eepro100
 M: Stefan Weil <sw@weilnetz.de>
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 58efc46144e..7997d895449 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -60,6 +60,7 @@
   (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-test'] : []) +              \
   (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-swtpm-test'] : []) +        \
   (config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) +              \
+  (config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ? ['fuzz-e1000e-test'] : []) +   \
   qtests_pci +                                                                              \
   ['fdc-test',
    'ide-test',
-- 
2.26.2

Re: [PATCH v6 6/7] net/eth: Read ip6_ext_hdr_routing buffer before accessing it

[Alexander Bulekov]

Just noticed that I also reported this to QEMU-Security on 2020-05-17.
The problem was acknowledged, but I don't think there was any
communication after that, so I'm not sure whether this is also stuck in
some private issue tracker. Seems pretty tame as far as
memory-corrputions go, but I'll send a followup to the private report,
to see if it went anywhere..
-Alex

On 210310 1931, Philippe Mathieu-Daudé wrote:
> We can't know the caller read enough data in the memory pointed
> by ext_hdr to cast it as a ip6_ext_hdr_routing.
> Declare rt_hdr on the stack and fill it again from the iovec.
> 
> Since we already checked there is enough data in the iovec buffer,
> simply add an assert() call to consume the bytes_read variable.
> 
> This fix a 2 bytes buffer overrun in eth_parse_ipv6_hdr() reported
> by QEMU fuzzer:
> 
>   $ cat << EOF | ./qemu-system-i386 -M pc-q35-5.0 \
>     -accel qtest -monitor none \
>     -serial none -nographic -qtest stdio
>   outl 0xcf8 0x80001010
>   outl 0xcfc 0xe1020000
>   outl 0xcf8 0x80001004
>   outw 0xcfc 0x7
>   write 0x25 0x1 0x86
>   write 0x26 0x1 0xdd
>   write 0x4f 0x1 0x2b
>   write 0xe1020030 0x4 0x190002e1
>   write 0xe102003a 0x2 0x0807
>   write 0xe1020048 0x4 0x12077cdd
>   write 0xe1020400 0x4 0xba077cdd
>   write 0xe1020420 0x4 0x190002e1
>   write 0xe1020428 0x4 0x3509d807
>   write 0xe1020438 0x1 0xe2
>   EOF
>   =================================================================
>   ==2859770==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffdef904902 at pc 0x561ceefa78de bp 0x7ffdef904820 sp 0x7ffdef904818
>   READ of size 1 at 0x7ffdef904902 thread T0
>       #0 0x561ceefa78dd in _eth_get_rss_ex_dst_addr net/eth.c:410:17
>       #1 0x561ceefa41fb in eth_parse_ipv6_hdr net/eth.c:532:17
>       #2 0x561cef7de639 in net_tx_pkt_parse_headers hw/net/net_tx_pkt.c:228:14
>       #3 0x561cef7dbef4 in net_tx_pkt_parse hw/net/net_tx_pkt.c:273:9
>       #4 0x561ceec29f22 in e1000e_process_tx_desc hw/net/e1000e_core.c:730:29
>       #5 0x561ceec28eac in e1000e_start_xmit hw/net/e1000e_core.c:927:9
>       #6 0x561ceec1baab in e1000e_set_tdt hw/net/e1000e_core.c:2444:9
>       #7 0x561ceebf300e in e1000e_core_write hw/net/e1000e_core.c:3256:9
>       #8 0x561cef3cd4cd in e1000e_mmio_write hw/net/e1000e.c:110:5
> 
>   Address 0x7ffdef904902 is located in stack of thread T0 at offset 34 in frame
>       #0 0x561ceefa320f in eth_parse_ipv6_hdr net/eth.c:486
> 
>     This frame has 1 object(s):
>       [32, 34) 'ext_hdr' (line 487) <== Memory access at offset 34 overflows this variable
>   HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
>         (longjmp and C++ exceptions *are* supported)
>   SUMMARY: AddressSanitizer: stack-buffer-overflow net/eth.c:410:17 in _eth_get_rss_ex_dst_addr
>   Shadow bytes around the buggy address:
>     0x10003df188d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>     0x10003df188e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>     0x10003df188f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>     0x10003df18900: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>     0x10003df18910: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
>   =>0x10003df18920:[02]f3 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
>     0x10003df18930: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>     0x10003df18940: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>     0x10003df18950: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>     0x10003df18960: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>     0x10003df18970: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   Shadow byte legend (one shadow byte represents 8 application bytes):
>     Addressable:           00
>     Partially addressable: 01 02 03 04 05 06 07
>     Stack left redzone:      f1
>     Stack right redzone:     f3
>   ==2859770==ABORTING
> 
> Add the corresponding qtest case with the fuzzer reproducer.
> 
> FWIW GCC 11 similarly reported:
> 
>   net/eth.c: In function 'eth_parse_ipv6_hdr':
>   net/eth.c:410:15: error: array subscript 'struct ip6_ext_hdr_routing[0]' is partly outside array bounds of 'struct ip6_ext_hdr[1]' [-Werror=array-bounds]
>     410 |     if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) {
>         |          ~~~~~^~~~~~~
>   net/eth.c:485:24: note: while referencing 'ext_hdr'
>     485 |     struct ip6_ext_hdr ext_hdr;
>         |                        ^~~~~~~
>   net/eth.c:410:38: error: array subscript 'struct ip6_ext_hdr_routing[0]' is partly outside array bounds of 'struct ip6_ext_hdr[1]' [-Werror=array-bounds]
>     410 |     if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) {
>         |                                 ~~~~~^~~~~~~~~
>   net/eth.c:485:24: note: while referencing 'ext_hdr'
>     485 |     struct ip6_ext_hdr ext_hdr;
>         |                        ^~~~~~~
> 
> Cc: qemu-stable@nongnu.org
> Buglink: https://bugs.launchpad.net/qemu/+bug/1879531
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Reported-by: Miroslav Rezanina <mrezanin@redhat.com>
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> Fixes: eb700029c78 ("net_pkt: Extend packet abstraction as required by e1000e functionality")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  net/eth.c                      | 13 +++++----
>  tests/qtest/fuzz-e1000e-test.c | 53 ++++++++++++++++++++++++++++++++++
>  MAINTAINERS                    |  1 +
>  tests/qtest/meson.build        |  1 +
>  4 files changed, 63 insertions(+), 5 deletions(-)
>  create mode 100644 tests/qtest/fuzz-e1000e-test.c
> 
> diff --git a/net/eth.c b/net/eth.c
> index b78aa526efc..284ade4ab0b 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -405,17 +405,20 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags,
>                          struct ip6_ext_hdr *ext_hdr,
>                          struct in6_address *dst_addr)
>  {
> -    struct ip6_ext_hdr_routing *rthdr = (struct ip6_ext_hdr_routing *) ext_hdr;
> +    struct ip6_ext_hdr_routing rt_hdr;
>      size_t input_size = iov_size(pkt, pkt_frags);
>      size_t bytes_read;
>  
> -    if (input_size < ext_hdr_offset + sizeof(*rthdr) + sizeof(*dst_addr)) {
> +    if (input_size < ext_hdr_offset + sizeof(rt_hdr) + sizeof(*dst_addr)) {
>          return false;
>      }
>  
> -    if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) {
> -        bytes_read = iov_to_buf(pkt, pkt_frags,
> -                                ext_hdr_offset + sizeof(*rthdr),
> +    bytes_read = iov_to_buf(pkt, pkt_frags, ext_hdr_offset,
> +                            &rt_hdr, sizeof(rt_hdr));
> +    assert(bytes_read == sizeof(rt_hdr));
> +
> +    if ((rt_hdr.rtype == 2) && (rt_hdr.segleft == 1)) {
> +        bytes_read = iov_to_buf(pkt, pkt_frags, ext_hdr_offset + sizeof(rt_hdr),
>                                  dst_addr, sizeof(*dst_addr));
>  
>          return bytes_read == sizeof(*dst_addr);
> diff --git a/tests/qtest/fuzz-e1000e-test.c b/tests/qtest/fuzz-e1000e-test.c
> new file mode 100644
> index 00000000000..66229e60964
> --- /dev/null
> +++ b/tests/qtest/fuzz-e1000e-test.c
> @@ -0,0 +1,53 @@
> +/*
> + * QTest testcase for e1000e device generated by fuzzer
> + *
> + * Copyright (c) 2021 Red Hat, Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "libqos/libqtest.h"
> +
> +/*
> + * https://bugs.launchpad.net/qemu/+bug/1879531
> + */
> +static void test_lp1879531_eth_get_rss_ex_dst_addr(void)
> +{
> +    QTestState *s;
> +
> +    s = qtest_init("-nographic -monitor none -serial none -M pc-q35-5.0");
> +
> +    qtest_outl(s, 0xcf8, 0x80001010);
> +    qtest_outl(s, 0xcfc, 0xe1020000);
> +    qtest_outl(s, 0xcf8, 0x80001004);
> +    qtest_outw(s, 0xcfc, 0x7);
> +    qtest_writeb(s, 0x25, 0x86);
> +    qtest_writeb(s, 0x26, 0xdd);
> +    qtest_writeb(s, 0x4f, 0x2b);
> +
> +    qtest_writel(s, 0xe1020030, 0x190002e1);
> +    qtest_writew(s, 0xe102003a, 0x0807);
> +    qtest_writel(s, 0xe1020048, 0x12077cdd);
> +    qtest_writel(s, 0xe1020400, 0xba077cdd);
> +    qtest_writel(s, 0xe1020420, 0x190002e1);
> +    qtest_writel(s, 0xe1020428, 0x3509d807);
> +    qtest_writeb(s, 0xe1020438, 0xe2);
> +    qtest_writeb(s, 0x4f, 0x2b);
> +    qtest_quit(s);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    const char *arch = qtest_get_arch();
> +
> +    g_test_init(&argc, &argv, NULL);
> +
> +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> +        qtest_add_func("fuzz/test_lp1879531_eth_get_rss_ex_dst_addr",
> +                       test_lp1879531_eth_get_rss_ex_dst_addr);
> +    }
> +
> +    return g_test_run();
> +}
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 738786146d6..cc5f3aa6b60 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2003,6 +2003,7 @@ e1000e
>  M: Dmitry Fleytman <dmitry.fleytman@gmail.com>
>  S: Maintained
>  F: hw/net/e1000e*
> +F: tests/qtest/fuzz-e1000e-test.c
>  
>  eepro100
>  M: Stefan Weil <sw@weilnetz.de>
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 58efc46144e..7997d895449 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -60,6 +60,7 @@
>    (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-test'] : []) +              \
>    (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-swtpm-test'] : []) +        \
>    (config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) +              \
> +  (config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ? ['fuzz-e1000e-test'] : []) +   \
>    qtests_pci +                                                                              \
>    ['fdc-test',
>     'ide-test',
> -- 
> 2.26.2
> 

Re: [PATCH v6 6/7] net/eth: Read ip6_ext_hdr_routing buffer before accessing it

[Alexander Bulekov]

Correction: there was a response suggesting to add padding to ip6_ext_hdr.

On 210317 1233, Alexander Bulekov wrote:
> Just noticed that I also reported this to QEMU-Security on 2020-05-17.
> The problem was acknowledged, but I don't think there was any
> communication after that, so I'm not sure whether this is also stuck in
> some private issue tracker. Seems pretty tame as far as
> memory-corrputions go, but I'll send a followup to the private report,
> to see if it went anywhere..
> -Alex
> 
> On 210310 1931, Philippe Mathieu-Daudé wrote:
> > We can't know the caller read enough data in the memory pointed
> > by ext_hdr to cast it as a ip6_ext_hdr_routing.
> > Declare rt_hdr on the stack and fill it again from the iovec.
> > 
> > Since we already checked there is enough data in the iovec buffer,
> > simply add an assert() call to consume the bytes_read variable.
> > 
> > This fix a 2 bytes buffer overrun in eth_parse_ipv6_hdr() reported
> > by QEMU fuzzer:
> > 
> >   $ cat << EOF | ./qemu-system-i386 -M pc-q35-5.0 \
> >     -accel qtest -monitor none \
> >     -serial none -nographic -qtest stdio
> >   outl 0xcf8 0x80001010
> >   outl 0xcfc 0xe1020000
> >   outl 0xcf8 0x80001004
> >   outw 0xcfc 0x7
> >   write 0x25 0x1 0x86
> >   write 0x26 0x1 0xdd
> >   write 0x4f 0x1 0x2b
> >   write 0xe1020030 0x4 0x190002e1
> >   write 0xe102003a 0x2 0x0807
> >   write 0xe1020048 0x4 0x12077cdd
> >   write 0xe1020400 0x4 0xba077cdd
> >   write 0xe1020420 0x4 0x190002e1
> >   write 0xe1020428 0x4 0x3509d807
> >   write 0xe1020438 0x1 0xe2
> >   EOF
> >   =================================================================
> >   ==2859770==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffdef904902 at pc 0x561ceefa78de bp 0x7ffdef904820 sp 0x7ffdef904818
> >   READ of size 1 at 0x7ffdef904902 thread T0
> >       #0 0x561ceefa78dd in _eth_get_rss_ex_dst_addr net/eth.c:410:17
> >       #1 0x561ceefa41fb in eth_parse_ipv6_hdr net/eth.c:532:17
> >       #2 0x561cef7de639 in net_tx_pkt_parse_headers hw/net/net_tx_pkt.c:228:14
> >       #3 0x561cef7dbef4 in net_tx_pkt_parse hw/net/net_tx_pkt.c:273:9
> >       #4 0x561ceec29f22 in e1000e_process_tx_desc hw/net/e1000e_core.c:730:29
> >       #5 0x561ceec28eac in e1000e_start_xmit hw/net/e1000e_core.c:927:9
> >       #6 0x561ceec1baab in e1000e_set_tdt hw/net/e1000e_core.c:2444:9
> >       #7 0x561ceebf300e in e1000e_core_write hw/net/e1000e_core.c:3256:9
> >       #8 0x561cef3cd4cd in e1000e_mmio_write hw/net/e1000e.c:110:5
> > 
> >   Address 0x7ffdef904902 is located in stack of thread T0 at offset 34 in frame
> >       #0 0x561ceefa320f in eth_parse_ipv6_hdr net/eth.c:486
> > 
> >     This frame has 1 object(s):
> >       [32, 34) 'ext_hdr' (line 487) <== Memory access at offset 34 overflows this variable
> >   HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
> >         (longjmp and C++ exceptions *are* supported)
> >   SUMMARY: AddressSanitizer: stack-buffer-overflow net/eth.c:410:17 in _eth_get_rss_ex_dst_addr
> >   Shadow bytes around the buggy address:
> >     0x10003df188d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >     0x10003df188e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >     0x10003df188f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >     0x10003df18900: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >     0x10003df18910: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
> >   =>0x10003df18920:[02]f3 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
> >     0x10003df18930: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >     0x10003df18940: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >     0x10003df18950: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >     0x10003df18960: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >     0x10003df18970: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >   Shadow byte legend (one shadow byte represents 8 application bytes):
> >     Addressable:           00
> >     Partially addressable: 01 02 03 04 05 06 07
> >     Stack left redzone:      f1
> >     Stack right redzone:     f3
> >   ==2859770==ABORTING
> > 
> > Add the corresponding qtest case with the fuzzer reproducer.
> > 
> > FWIW GCC 11 similarly reported:
> > 
> >   net/eth.c: In function 'eth_parse_ipv6_hdr':
> >   net/eth.c:410:15: error: array subscript 'struct ip6_ext_hdr_routing[0]' is partly outside array bounds of 'struct ip6_ext_hdr[1]' [-Werror=array-bounds]
> >     410 |     if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) {
> >         |          ~~~~~^~~~~~~
> >   net/eth.c:485:24: note: while referencing 'ext_hdr'
> >     485 |     struct ip6_ext_hdr ext_hdr;
> >         |                        ^~~~~~~
> >   net/eth.c:410:38: error: array subscript 'struct ip6_ext_hdr_routing[0]' is partly outside array bounds of 'struct ip6_ext_hdr[1]' [-Werror=array-bounds]
> >     410 |     if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) {
> >         |                                 ~~~~~^~~~~~~~~
> >   net/eth.c:485:24: note: while referencing 'ext_hdr'
> >     485 |     struct ip6_ext_hdr ext_hdr;
> >         |                        ^~~~~~~
> > 
> > Cc: qemu-stable@nongnu.org
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1879531
> > Reported-by: Alexander Bulekov <alxndr@bu.edu>
> > Reported-by: Miroslav Rezanina <mrezanin@redhat.com>
> > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> > Fixes: eb700029c78 ("net_pkt: Extend packet abstraction as required by e1000e functionality")
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> >  net/eth.c                      | 13 +++++----
> >  tests/qtest/fuzz-e1000e-test.c | 53 ++++++++++++++++++++++++++++++++++
> >  MAINTAINERS                    |  1 +
> >  tests/qtest/meson.build        |  1 +
> >  4 files changed, 63 insertions(+), 5 deletions(-)
> >  create mode 100644 tests/qtest/fuzz-e1000e-test.c
> > 
> > diff --git a/net/eth.c b/net/eth.c
> > index b78aa526efc..284ade4ab0b 100644
> > --- a/net/eth.c
> > +++ b/net/eth.c
> > @@ -405,17 +405,20 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags,
> >                          struct ip6_ext_hdr *ext_hdr,
> >                          struct in6_address *dst_addr)
> >  {
> > -    struct ip6_ext_hdr_routing *rthdr = (struct ip6_ext_hdr_routing *) ext_hdr;
> > +    struct ip6_ext_hdr_routing rt_hdr;
> >      size_t input_size = iov_size(pkt, pkt_frags);
> >      size_t bytes_read;
> >  
> > -    if (input_size < ext_hdr_offset + sizeof(*rthdr) + sizeof(*dst_addr)) {
> > +    if (input_size < ext_hdr_offset + sizeof(rt_hdr) + sizeof(*dst_addr)) {
> >          return false;
> >      }
> >  
> > -    if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) {
> > -        bytes_read = iov_to_buf(pkt, pkt_frags,
> > -                                ext_hdr_offset + sizeof(*rthdr),
> > +    bytes_read = iov_to_buf(pkt, pkt_frags, ext_hdr_offset,
> > +                            &rt_hdr, sizeof(rt_hdr));
> > +    assert(bytes_read == sizeof(rt_hdr));
> > +
> > +    if ((rt_hdr.rtype == 2) && (rt_hdr.segleft == 1)) {
> > +        bytes_read = iov_to_buf(pkt, pkt_frags, ext_hdr_offset + sizeof(rt_hdr),
> >                                  dst_addr, sizeof(*dst_addr));
> >  
> >          return bytes_read == sizeof(*dst_addr);
> > diff --git a/tests/qtest/fuzz-e1000e-test.c b/tests/qtest/fuzz-e1000e-test.c
> > new file mode 100644
> > index 00000000000..66229e60964
> > --- /dev/null
> > +++ b/tests/qtest/fuzz-e1000e-test.c
> > @@ -0,0 +1,53 @@
> > +/*
> > + * QTest testcase for e1000e device generated by fuzzer
> > + *
> > + * Copyright (c) 2021 Red Hat, Inc.
> > + *
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +
> > +#include "libqos/libqtest.h"
> > +
> > +/*
> > + * https://bugs.launchpad.net/qemu/+bug/1879531
> > + */
> > +static void test_lp1879531_eth_get_rss_ex_dst_addr(void)
> > +{
> > +    QTestState *s;
> > +
> > +    s = qtest_init("-nographic -monitor none -serial none -M pc-q35-5.0");
> > +
> > +    qtest_outl(s, 0xcf8, 0x80001010);
> > +    qtest_outl(s, 0xcfc, 0xe1020000);
> > +    qtest_outl(s, 0xcf8, 0x80001004);
> > +    qtest_outw(s, 0xcfc, 0x7);
> > +    qtest_writeb(s, 0x25, 0x86);
> > +    qtest_writeb(s, 0x26, 0xdd);
> > +    qtest_writeb(s, 0x4f, 0x2b);
> > +
> > +    qtest_writel(s, 0xe1020030, 0x190002e1);
> > +    qtest_writew(s, 0xe102003a, 0x0807);
> > +    qtest_writel(s, 0xe1020048, 0x12077cdd);
> > +    qtest_writel(s, 0xe1020400, 0xba077cdd);
> > +    qtest_writel(s, 0xe1020420, 0x190002e1);
> > +    qtest_writel(s, 0xe1020428, 0x3509d807);
> > +    qtest_writeb(s, 0xe1020438, 0xe2);
> > +    qtest_writeb(s, 0x4f, 0x2b);
> > +    qtest_quit(s);
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > +    const char *arch = qtest_get_arch();
> > +
> > +    g_test_init(&argc, &argv, NULL);
> > +
> > +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> > +        qtest_add_func("fuzz/test_lp1879531_eth_get_rss_ex_dst_addr",
> > +                       test_lp1879531_eth_get_rss_ex_dst_addr);
> > +    }
> > +
> > +    return g_test_run();
> > +}
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 738786146d6..cc5f3aa6b60 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2003,6 +2003,7 @@ e1000e
> >  M: Dmitry Fleytman <dmitry.fleytman@gmail.com>
> >  S: Maintained
> >  F: hw/net/e1000e*
> > +F: tests/qtest/fuzz-e1000e-test.c
> >  
> >  eepro100
> >  M: Stefan Weil <sw@weilnetz.de>
> > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> > index 58efc46144e..7997d895449 100644
> > --- a/tests/qtest/meson.build
> > +++ b/tests/qtest/meson.build
> > @@ -60,6 +60,7 @@
> >    (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-test'] : []) +              \
> >    (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-swtpm-test'] : []) +        \
> >    (config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) +              \
> > +  (config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ? ['fuzz-e1000e-test'] : []) +   \
> >    qtests_pci +                                                                              \
> >    ['fdc-test',
> >     'ide-test',
> > -- 
> > 2.26.2
> > 

Re: [PATCH v6 6/7] net/eth: Read ip6_ext_hdr_routing buffer before accessing it

[Philippe Mathieu-Daudé]

On 3/17/21 5:35 PM, Alexander Bulekov wrote:
> Correction: there was a response suggesting to add padding to ip6_ext_hdr.

Was the response on the public list or the private security one?

If it was public I missed it. On a private list such comment isn't
very helpful if nobody sends patches to fix it. Maybe we need to
review the security list process.

> On 210317 1233, Alexander Bulekov wrote:
>> Just noticed that I also reported this to QEMU-Security on 2020-05-17.
>> The problem was acknowledged, but I don't think there was any
>> communication after that, so I'm not sure whether this is also stuck in
>> some private issue tracker. Seems pretty tame as far as
>> memory-corrputions go, but I'll send a followup to the private report,
>> to see if it went anywhere..
>> -Alex
>>
>> On 210310 1931, Philippe Mathieu-Daudé wrote:
>>> We can't know the caller read enough data in the memory pointed
>>> by ext_hdr to cast it as a ip6_ext_hdr_routing.
>>> Declare rt_hdr on the stack and fill it again from the iovec.
>>>
>>> Since we already checked there is enough data in the iovec buffer,
>>> simply add an assert() call to consume the bytes_read variable.
>>>
>>> This fix a 2 bytes buffer overrun in eth_parse_ipv6_hdr() reported
>>> by QEMU fuzzer:
>>>
>>>   $ cat << EOF | ./qemu-system-i386 -M pc-q35-5.0 \
>>>     -accel qtest -monitor none \
>>>     -serial none -nographic -qtest stdio
>>>   outl 0xcf8 0x80001010
>>>   outl 0xcfc 0xe1020000
>>>   outl 0xcf8 0x80001004
>>>   outw 0xcfc 0x7
>>>   write 0x25 0x1 0x86
>>>   write 0x26 0x1 0xdd
>>>   write 0x4f 0x1 0x2b
>>>   write 0xe1020030 0x4 0x190002e1
>>>   write 0xe102003a 0x2 0x0807
>>>   write 0xe1020048 0x4 0x12077cdd
>>>   write 0xe1020400 0x4 0xba077cdd
>>>   write 0xe1020420 0x4 0x190002e1
>>>   write 0xe1020428 0x4 0x3509d807
>>>   write 0xe1020438 0x1 0xe2
>>>   EOF
>>>   =================================================================
>>>   ==2859770==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffdef904902 at pc 0x561ceefa78de bp 0x7ffdef904820 sp 0x7ffdef904818
>>>   READ of size 1 at 0x7ffdef904902 thread T0
>>>       #0 0x561ceefa78dd in _eth_get_rss_ex_dst_addr net/eth.c:410:17
>>>       #1 0x561ceefa41fb in eth_parse_ipv6_hdr net/eth.c:532:17
>>>       #2 0x561cef7de639 in net_tx_pkt_parse_headers hw/net/net_tx_pkt.c:228:14
>>>       #3 0x561cef7dbef4 in net_tx_pkt_parse hw/net/net_tx_pkt.c:273:9
>>>       #4 0x561ceec29f22 in e1000e_process_tx_desc hw/net/e1000e_core.c:730:29
>>>       #5 0x561ceec28eac in e1000e_start_xmit hw/net/e1000e_core.c:927:9
>>>       #6 0x561ceec1baab in e1000e_set_tdt hw/net/e1000e_core.c:2444:9
>>>       #7 0x561ceebf300e in e1000e_core_write hw/net/e1000e_core.c:3256:9
>>>       #8 0x561cef3cd4cd in e1000e_mmio_write hw/net/e1000e.c:110:5
>>>
>>>   Address 0x7ffdef904902 is located in stack of thread T0 at offset 34 in frame
>>>       #0 0x561ceefa320f in eth_parse_ipv6_hdr net/eth.c:486
>>>
>>>     This frame has 1 object(s):
>>>       [32, 34) 'ext_hdr' (line 487) <== Memory access at offset 34 overflows this variable
>>>   HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
>>>         (longjmp and C++ exceptions *are* supported)
>>>   SUMMARY: AddressSanitizer: stack-buffer-overflow net/eth.c:410:17 in _eth_get_rss_ex_dst_addr
>>>   Shadow bytes around the buggy address:
>>>     0x10003df188d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>>     0x10003df188e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>>     0x10003df188f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>>     0x10003df18900: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>>     0x10003df18910: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
>>>   =>0x10003df18920:[02]f3 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
>>>     0x10003df18930: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>>     0x10003df18940: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>>     0x10003df18950: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>>     0x10003df18960: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>>     0x10003df18970: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>>   Shadow byte legend (one shadow byte represents 8 application bytes):
>>>     Addressable:           00
>>>     Partially addressable: 01 02 03 04 05 06 07
>>>     Stack left redzone:      f1
>>>     Stack right redzone:     f3
>>>   ==2859770==ABORTING
>>>
>>> Add the corresponding qtest case with the fuzzer reproducer.
>>>
>>> FWIW GCC 11 similarly reported:
>>>
>>>   net/eth.c: In function 'eth_parse_ipv6_hdr':
>>>   net/eth.c:410:15: error: array subscript 'struct ip6_ext_hdr_routing[0]' is partly outside array bounds of 'struct ip6_ext_hdr[1]' [-Werror=array-bounds]
>>>     410 |     if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) {
>>>         |          ~~~~~^~~~~~~
>>>   net/eth.c:485:24: note: while referencing 'ext_hdr'
>>>     485 |     struct ip6_ext_hdr ext_hdr;
>>>         |                        ^~~~~~~
>>>   net/eth.c:410:38: error: array subscript 'struct ip6_ext_hdr_routing[0]' is partly outside array bounds of 'struct ip6_ext_hdr[1]' [-Werror=array-bounds]
>>>     410 |     if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) {
>>>         |                                 ~~~~~^~~~~~~~~
>>>   net/eth.c:485:24: note: while referencing 'ext_hdr'
>>>     485 |     struct ip6_ext_hdr ext_hdr;
>>>         |                        ^~~~~~~
>>>
>>> Cc: qemu-stable@nongnu.org
>>> Buglink: https://bugs.launchpad.net/qemu/+bug/1879531
>>> Reported-by: Alexander Bulekov <alxndr@bu.edu>
>>> Reported-by: Miroslav Rezanina <mrezanin@redhat.com>
>>> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>>> Fixes: eb700029c78 ("net_pkt: Extend packet abstraction as required by e1000e functionality")
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>  net/eth.c                      | 13 +++++----
>>>  tests/qtest/fuzz-e1000e-test.c | 53 ++++++++++++++++++++++++++++++++++
>>>  MAINTAINERS                    |  1 +
>>>  tests/qtest/meson.build        |  1 +
>>>  4 files changed, 63 insertions(+), 5 deletions(-)
>>>  create mode 100644 tests/qtest/fuzz-e1000e-test.c

Re: [PATCH v6 6/7] net/eth: Read ip6_ext_hdr_routing buffer before accessing it

[Alexander Bulekov]

On 210317 1742, Philippe Mathieu-Daudé wrote:
> On 3/17/21 5:35 PM, Alexander Bulekov wrote:
> > Correction: there was a response suggesting to add padding to ip6_ext_hdr.
> 
> Was the response on the public list or the private security one?

It was private, but I just CC-ed you on the response. Since this
predates the switch to qemu-security@nongnu.org, maybe it's not as much
of a problem anymore

> 
> If it was public I missed it. On a private list such comment isn't
> very helpful if nobody sends patches to fix it. Maybe we need to
> review the security list process.
> 
> > On 210317 1233, Alexander Bulekov wrote:
> >> Just noticed that I also reported this to QEMU-Security on 2020-05-17.
> >> The problem was acknowledged, but I don't think there was any
> >> communication after that, so I'm not sure whether this is also stuck in
> >> some private issue tracker. Seems pretty tame as far as
> >> memory-corrputions go, but I'll send a followup to the private report,
> >> to see if it went anywhere..
> >> -Alex
> >>
> >> On 210310 1931, Philippe Mathieu-Daudé wrote:
> >>> We can't know the caller read enough data in the memory pointed
> >>> by ext_hdr to cast it as a ip6_ext_hdr_routing.
> >>> Declare rt_hdr on the stack and fill it again from the iovec.
> >>>
> >>> Since we already checked there is enough data in the iovec buffer,
> >>> simply add an assert() call to consume the bytes_read variable.
> >>>
> >>> This fix a 2 bytes buffer overrun in eth_parse_ipv6_hdr() reported
> >>> by QEMU fuzzer:
> >>>
> >>>   $ cat << EOF | ./qemu-system-i386 -M pc-q35-5.0 \
> >>>     -accel qtest -monitor none \
> >>>     -serial none -nographic -qtest stdio
> >>>   outl 0xcf8 0x80001010
> >>>   outl 0xcfc 0xe1020000
> >>>   outl 0xcf8 0x80001004
> >>>   outw 0xcfc 0x7
> >>>   write 0x25 0x1 0x86
> >>>   write 0x26 0x1 0xdd
> >>>   write 0x4f 0x1 0x2b
> >>>   write 0xe1020030 0x4 0x190002e1
> >>>   write 0xe102003a 0x2 0x0807
> >>>   write 0xe1020048 0x4 0x12077cdd
> >>>   write 0xe1020400 0x4 0xba077cdd
> >>>   write 0xe1020420 0x4 0x190002e1
> >>>   write 0xe1020428 0x4 0x3509d807
> >>>   write 0xe1020438 0x1 0xe2
> >>>   EOF
> >>>   =================================================================
> >>>   ==2859770==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffdef904902 at pc 0x561ceefa78de bp 0x7ffdef904820 sp 0x7ffdef904818
> >>>   READ of size 1 at 0x7ffdef904902 thread T0
> >>>       #0 0x561ceefa78dd in _eth_get_rss_ex_dst_addr net/eth.c:410:17
> >>>       #1 0x561ceefa41fb in eth_parse_ipv6_hdr net/eth.c:532:17
> >>>       #2 0x561cef7de639 in net_tx_pkt_parse_headers hw/net/net_tx_pkt.c:228:14
> >>>       #3 0x561cef7dbef4 in net_tx_pkt_parse hw/net/net_tx_pkt.c:273:9
> >>>       #4 0x561ceec29f22 in e1000e_process_tx_desc hw/net/e1000e_core.c:730:29
> >>>       #5 0x561ceec28eac in e1000e_start_xmit hw/net/e1000e_core.c:927:9
> >>>       #6 0x561ceec1baab in e1000e_set_tdt hw/net/e1000e_core.c:2444:9
> >>>       #7 0x561ceebf300e in e1000e_core_write hw/net/e1000e_core.c:3256:9
> >>>       #8 0x561cef3cd4cd in e1000e_mmio_write hw/net/e1000e.c:110:5
> >>>
> >>>   Address 0x7ffdef904902 is located in stack of thread T0 at offset 34 in frame
> >>>       #0 0x561ceefa320f in eth_parse_ipv6_hdr net/eth.c:486
> >>>
> >>>     This frame has 1 object(s):
> >>>       [32, 34) 'ext_hdr' (line 487) <== Memory access at offset 34 overflows this variable
> >>>   HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
> >>>         (longjmp and C++ exceptions *are* supported)
> >>>   SUMMARY: AddressSanitizer: stack-buffer-overflow net/eth.c:410:17 in _eth_get_rss_ex_dst_addr
> >>>   Shadow bytes around the buggy address:
> >>>     0x10003df188d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>     0x10003df188e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>     0x10003df188f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>     0x10003df18900: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>     0x10003df18910: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
> >>>   =>0x10003df18920:[02]f3 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
> >>>     0x10003df18930: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>     0x10003df18940: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>     0x10003df18950: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>     0x10003df18960: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>     0x10003df18970: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>   Shadow byte legend (one shadow byte represents 8 application bytes):
> >>>     Addressable:           00
> >>>     Partially addressable: 01 02 03 04 05 06 07
> >>>     Stack left redzone:      f1
> >>>     Stack right redzone:     f3
> >>>   ==2859770==ABORTING
> >>>
> >>> Add the corresponding qtest case with the fuzzer reproducer.
> >>>
> >>> FWIW GCC 11 similarly reported:
> >>>
> >>>   net/eth.c: In function 'eth_parse_ipv6_hdr':
> >>>   net/eth.c:410:15: error: array subscript 'struct ip6_ext_hdr_routing[0]' is partly outside array bounds of 'struct ip6_ext_hdr[1]' [-Werror=array-bounds]
> >>>     410 |     if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) {
> >>>         |          ~~~~~^~~~~~~
> >>>   net/eth.c:485:24: note: while referencing 'ext_hdr'
> >>>     485 |     struct ip6_ext_hdr ext_hdr;
> >>>         |                        ^~~~~~~
> >>>   net/eth.c:410:38: error: array subscript 'struct ip6_ext_hdr_routing[0]' is partly outside array bounds of 'struct ip6_ext_hdr[1]' [-Werror=array-bounds]
> >>>     410 |     if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) {
> >>>         |                                 ~~~~~^~~~~~~~~
> >>>   net/eth.c:485:24: note: while referencing 'ext_hdr'
> >>>     485 |     struct ip6_ext_hdr ext_hdr;
> >>>         |                        ^~~~~~~
> >>>
> >>> Cc: qemu-stable@nongnu.org
> >>> Buglink: https://bugs.launchpad.net/qemu/+bug/1879531
> >>> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> >>> Reported-by: Miroslav Rezanina <mrezanin@redhat.com>
> >>> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> >>> Fixes: eb700029c78 ("net_pkt: Extend packet abstraction as required by e1000e functionality")
> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >>> ---
> >>>  net/eth.c                      | 13 +++++----
> >>>  tests/qtest/fuzz-e1000e-test.c | 53 ++++++++++++++++++++++++++++++++++
> >>>  MAINTAINERS                    |  1 +
> >>>  tests/qtest/meson.build        |  1 +
> >>>  4 files changed, 63 insertions(+), 5 deletions(-)
> >>>  create mode 100644 tests/qtest/fuzz-e1000e-test.c
> 

[PATCH v6 7/7] net/eth: Add an assert() and invert if() statement to simplify code

[Philippe Mathieu-Daudé]

To simplify the function body, invert the if() statement, returning
earlier.
Since we already checked there is enough data in the iovec buffer,
simply add an assert() call to consume the bytes_read variable.

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 net/eth.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/net/eth.c b/net/eth.c
index 284ade4ab0b..f5fde6d83e3 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -416,15 +416,14 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags,
     bytes_read = iov_to_buf(pkt, pkt_frags, ext_hdr_offset,
                             &rt_hdr, sizeof(rt_hdr));
     assert(bytes_read == sizeof(rt_hdr));
-
-    if ((rt_hdr.rtype == 2) && (rt_hdr.segleft == 1)) {
-        bytes_read = iov_to_buf(pkt, pkt_frags, ext_hdr_offset + sizeof(rt_hdr),
-                                dst_addr, sizeof(*dst_addr));
-
-        return bytes_read == sizeof(*dst_addr);
+    if ((rt_hdr.rtype != 2) || (rt_hdr.segleft != 1)) {
+        return false;
     }
+    bytes_read = iov_to_buf(pkt, pkt_frags, ext_hdr_offset + sizeof(rt_hdr),
+                            dst_addr, sizeof(*dst_addr));
+    assert(bytes_read == sizeof(*dst_addr));
 
-    return false;
+    return true;
 }
 
 static bool
-- 
2.26.2

Re: [PATCH v6 0/7] net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr()

[Miroslav Rezanina]

----- Original Message -----
> From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
> To: qemu-devel@nongnu.org
> Cc: "Jason Wang" <jasowang@redhat.com>, "Stefano Garzarella" <sgarzare@redhat.com>, "Thomas Huth" <thuth@redhat.com>,
> "Miroslav Rezanina" <mrezanin@redhat.com>, "Dmitry Fleytman" <dmitry.fleytman@gmail.com>, "Paolo Bonzini"
> <pbonzini@redhat.com>, "Laurent Vivier" <lvivier@redhat.com>, "Philippe Mathieu-Daudé" <philmd@redhat.com>
> Sent: Wednesday, March 10, 2021 7:31:16 PM
> Subject: [PATCH v6 0/7] net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr()
> 
> I had a look at the patch from Miroslav trying to silence a
> compiler warning which in fact is a nasty bug. Here is a fix.
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg772735.html
>

Reviewed-by: Miroslav Rezanina <mrezanin@redhat.com>
 
> Since v5:
> - addressed Stefano's review comments:
> - add now patch fixing in6_address offset
> 
> Since v4:
> - reworked again, tested it with Fedora Raw Hide
> 
> Philippe Mathieu-Daudé (7):
>   net/eth: Use correct in6_address offset in _eth_get_rss_ex_dst_addr()
>   net/eth: Simplify _eth_get_rss_ex_dst_addr()
>   net/eth: Better describe _eth_get_rss_ex_dst_addr's offset argument
>   net/eth: Check size earlier in _eth_get_rss_ex_dst_addr()
>   net/eth: Check iovec has enough data earlier
>   net/eth: Read ip6_ext_hdr_routing buffer before accessing it
>   net/eth: Add an assert() and invert if() statement to simplify code
> 
>  net/eth.c                      | 46 ++++++++++++++---------------
>  tests/qtest/fuzz-e1000e-test.c | 53 ++++++++++++++++++++++++++++++++++
>  MAINTAINERS                    |  1 +
>  tests/qtest/meson.build        |  1 +
>  4 files changed, 78 insertions(+), 23 deletions(-)
>  create mode 100644 tests/qtest/fuzz-e1000e-test.c
> 
> --
> 2.26.2
> 
> 
> 

-- 
Miroslav Rezanina
Software Engineer - Virtualization Team Maintainer

Re: [PATCH v6 0/7] net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr()

[Jason Wang]

在 2021/3/11 上午2:31, Philippe Mathieu-Daudé 写道:
> I had a look at the patch from Miroslav trying to silence a
> compiler warning which in fact is a nasty bug. Here is a fix.
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg772735.html
>
> Since v5:
> - addressed Stefano's review comments:
> - add now patch fixing in6_address offset
>
> Since v4:
> - reworked again, tested it with Fedora Raw Hide
>
> Philippe Mathieu-Daudé (7):
>    net/eth: Use correct in6_address offset in _eth_get_rss_ex_dst_addr()
>    net/eth: Simplify _eth_get_rss_ex_dst_addr()
>    net/eth: Better describe _eth_get_rss_ex_dst_addr's offset argument
>    net/eth: Check size earlier in _eth_get_rss_ex_dst_addr()
>    net/eth: Check iovec has enough data earlier
>    net/eth: Read ip6_ext_hdr_routing buffer before accessing it
>    net/eth: Add an assert() and invert if() statement to simplify code
>
>   net/eth.c                      | 46 ++++++++++++++---------------
>   tests/qtest/fuzz-e1000e-test.c | 53 ++++++++++++++++++++++++++++++++++
>   MAINTAINERS                    |  1 +
>   tests/qtest/meson.build        |  1 +
>   4 files changed, 78 insertions(+), 23 deletions(-)
>   create mode 100644 tests/qtest/fuzz-e1000e-test.c


Applied.

Thanks


>