qemu-stable
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4 4/6] net/eth: Check rt_hdr size before casting to ip6_ext_

From: Stefano Garzarella
Subject: Re: [PATCH v4 4/6] net/eth: Check rt_hdr size before casting to ip6_ext_hdr
Date: Wed, 10 Mar 2021 10:05:01 +0100 
On Tue, Mar 09, 2021 at 07:27:07PM +0100, Philippe Mathieu-Daudé wrote:

Do not cast our ip6_ext_hdr pointer to ip6_ext_hdr_routing if there
isn't enough data in the buffer for a such structure.

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>
Fixes: eb700029c78 ("net_pkt: Extend packet abstraction as required by e1000e 
functionality")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
net/eth.c                      |  7 ++++-
tests/qtest/fuzz-e1000e-test.c | 53 ++++++++++++++++++++++++++++++++++
MAINTAINERS                    |  1 +
tests/qtest/meson.build        |  1 +
4 files changed, 61 insertions(+), 1 deletion(-)
create mode 100644 tests/qtest/fuzz-e1000e-test.c

diff --git a/net/eth.c b/net/eth.c
index 77af2b673bb..f0c8dfe8df7 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -406,7 +406,12 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int 
pkt_frags,
                        struct in6_address *dst_addr)
{
    size_t input_size = iov_size(pkt, pkt_frags);
-    struct ip6_ext_hdr_routing *rthdr = (struct ip6_ext_hdr_routing *) ext_hdr;
+    struct ip6_ext_hdr_routing *rthdr;
+
+    if (input_size < ext_hdr_offset + sizeof(*rthdr)) {
+        return false;
+    }
+    rthdr = (struct ip6_ext_hdr_routing *) ext_hdr;
Also if we check 'input_size', I think this cast keeps having the 2 bytes buffer overrun issue since 'ext_hdr' contains only the first 2 bytes. I think we can remove the 'input_size' check and we should fix in this way: 

    struct ip6_ext_hdr_routing rthdr;

    bytes_read = iov_to_buf(pkt, pkt_frags, ext_hdr_offset, &rthdr,
                            sizeof(rthdr);
    if (bytes_read < sizeof(rthdr)) {
        return false;
    }



    if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) {
        size_t bytes_read;
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 f22d83c1782..4eb5784ff83 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
reply via email to
[Prev in Thread] Current Thread [Next in Thread]