Message ID | 20210126111847.3142636-3-philmd@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr() | expand |
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? Thanks, Stefano
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.
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; 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 48c0ec41e93..e275c81fd49 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1968,6 +1968,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 f2090296597..e19e2913547 100644 --- a/tests/qtest/meson.build +++ b/tests/qtest/meson.build @@ -7,6 +7,7 @@ qtests_generic = \ (config_all_devices.has_key('CONFIG_MEGASAS_SCSI_PCI') ? ['fuzz-megasas-test'] : []) + \ (config_all_devices.has_key('CONFIG_VIRTIO_SCSI') ? ['fuzz-virtio-scsi-test'] : []) + \ + (config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ? ['fuzz-e1000e-test'] : []) + \ [ 'cdrom-test', 'device-introspect-test',