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

[Stefano Garzarella <sgarzare@redhat.com>]

On Wed, Mar 10, 2021 at 05:01:34PM +0100, 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>
>Fixes: eb700029c78 ("net_pkt: Extend packet abstraction as required by e1000e functionality")
>Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>---
> net/eth.c                      | 10 +++++--
> tests/qtest/fuzz-e1000e-test.c | 53 ++++++++++++++++++++++++++++++++++
> MAINTAINERS                    |  1 +
> tests/qtest/meson.build        |  1 +
> 4 files changed, 62 insertions(+), 3 deletions(-)
> create mode 100644 tests/qtest/fuzz-e1000e-test.c

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