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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 1/14/21 3:50 PM, Philippe Mathieu-Daudé wrote:
> QEMU fuzzer reported a buffer overflow in _eth_get_rss_ex_dst_addr()
> reproducible as:
> 
>   $ 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
> 
> Similarly GCC 11 reports:
> 
>   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;
>         |                        ^~~~~~~
> 
> In eth_parse_ipv6_hdr() we called iov_to_buf() to fill the 2 bytes of
> the 'ext_hdr' buffer, then _eth_get_rss_ex_dst_addr() tries to access
> beside the 2 filled bytes.
> 
> Fix by reworking the function, filling the full rt_hdr buffer on the
> stack calling iov_to_buf() again.
> 
> 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 | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/net/eth.c b/net/eth.c
> index 7d4dd48c1ff..ae4db37888e 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -401,26 +401,23 @@ 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)
>  {
> -    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 ((rthdr->rtype == 2) && (rthdr->segleft == 1)) {
> +    if (input_size < ext_hdr_offset + sizeof(rt_hdr)) {
> +        return false;
> +    }
>  
> -        size_t input_size = iov_size(pkt, pkt_frags);
> -        size_t bytes_read;
> +    bytes_read = iov_to_buf(pkt, pkt_frags, ext_hdr_offset,
> +                            &rt_hdr, sizeof(rt_hdr));
>  
> -        if (input_size < rthdr_offset + sizeof(*ext_hdr)) {
> -            return false;
> -        }
> -
> -        bytes_read = iov_to_buf(pkt, pkt_frags,
> -                                rthdr_offset + sizeof(*ext_hdr),
> -                                dst_addr, sizeof(*dst_addr));
> -
> -        return bytes_read == sizeof(*dst_addr);
> +    if ((rt_hdr.rtype == 2) && (rt_hdr.segleft == 1)) {
> +        return bytes_read == sizeof(*ext_hdr) + sizeof(*dst_addr);
>      }
>  
>      return false;
> 

Sorry I forgot to commit the qtest:

-- >8 --
diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
index cdb1100a0b8..61500ca2510 100644
--- a/tests/qtest/fuzz-test.c
+++ b/tests/qtest/fuzz-test.c
@@ -97,6 +97,32 @@ static void test_mmio_oob_from_memory_region_cache(void)
     qtest_quit(s);
 }

+/*
+ * 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 0x1 0x86);
+    qtest_writeb(s, 0x26 0x1 0xdd);
+    qtest_writeb(s, 0x4f 0x1 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_quit(s);
+}
+
 int main(int argc, char **argv)
 {
     const char *arch = qtest_get_arch();
@@ -110,6 +136,8 @@ int main(int argc, char **argv)
                        test_lp1878642_pci_bus_get_irq_level_assert);
         qtest_add_func("fuzz/test_mmio_oob_from_memory_region_cache",
                        test_mmio_oob_from_memory_region_cache);
+        qtest_add_func("fuzz/test_lp1879531_eth_get_rss_ex_dst_addr",
+                       test_lp1879531_eth_get_rss_ex_dst_addr);
     }

     return g_test_run();
---