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

[Philippe Mathieu-Daudé]

On 1/26/21 3:35 PM, Stefano Garzarella wrote:
> On Tue, Jan 26, 2021 at 12:18:47PM +0100, 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.
>>
>> Add the corresponding qtest case with the fuzzer reproducer.
>>
>> 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")
>> Reviewed-by: Miroslav Rezanina <mrezanin@redhat.com>
>> Acked-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> net/eth.c                      | 25 +++++++---------
>> tests/qtest/fuzz-e1000e-test.c | 53 ++++++++++++++++++++++++++++++++++
>> MAINTAINERS                    |  1 +
>> tests/qtest/meson.build        |  1 +
>> 4 files changed, 66 insertions(+), 14 deletions(-)
>> create mode 100644 tests/qtest/fuzz-e1000e-test.c
>>
>> 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);
> 
> Maybe I missed something but bytes_read can be at most 8,
> sizeof(*ext_hdr) should be 2 and sizeof(*dst_addr) should be 16, so IIUC
> this check is always false.
> 
> Also, before this patch, the function filled dst_addr, now we don't, is
> that right?

Well I'll split in multiple trivial changes and KISS.

Thanks for the review!

Phil.