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

[Stefano Garzarella <sgarzare@redhat.com>]

On Wed, Mar 10, 2021 at 10:05:01AM +0100, Stefano Garzarella wrote:
>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;
>    }
>

Just saw that was what you had done in v3 :-)

I think we need to call iov_to_buf 2 times, once to read rthdr and once 
to read dst_addr.

Thanks,
Stefano