* [PATCH v3 0/2] net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr() @ 2021-01-26 11:18 Philippe Mathieu-Daudé 2021-01-26 11:18 ` [PATCH v3 1/2] net/eth: Simplify _eth_get_rss_ex_dst_addr() Philippe Mathieu-Daudé 2021-01-26 11:18 ` [PATCH v3 2/2] net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr() Philippe Mathieu-Daudé 0 siblings, 2 replies; 6+ messages in thread From: Philippe Mathieu-Daudé @ 2021-01-26 11:18 UTC (permalink / raw) To: qemu-devel Cc: Laurent Vivier, Thomas Huth, Dmitry Fleytman, Jason Wang, Li Qiang, Alexander Bulekov, Philippe Mathieu-Daudé I had a look at the patch from Miroslav trying to silence a compiler warning which in fact is a nasty bug. Here is a fix. https://www.mail-archive.com/qemu-devel@nongnu.org/msg772735.html v3: Added Thomas qtest Acked-by Philippe Mathieu-Daudé (2): net/eth: Simplify _eth_get_rss_ex_dst_addr() net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr() net/eth.c | 37 +++++++++++------------- tests/qtest/fuzz-e1000e-test.c | 53 ++++++++++++++++++++++++++++++++++ MAINTAINERS | 1 + tests/qtest/meson.build | 1 + 4 files changed, 72 insertions(+), 20 deletions(-) create mode 100644 tests/qtest/fuzz-e1000e-test.c -- 2.26.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/2] net/eth: Simplify _eth_get_rss_ex_dst_addr() 2021-01-26 11:18 [PATCH v3 0/2] net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr() Philippe Mathieu-Daudé @ 2021-01-26 11:18 ` Philippe Mathieu-Daudé 2021-01-26 13:38 ` Stefano Garzarella 2021-01-26 11:18 ` [PATCH v3 2/2] net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr() Philippe Mathieu-Daudé 1 sibling, 1 reply; 6+ messages in thread From: Philippe Mathieu-Daudé @ 2021-01-26 11:18 UTC (permalink / raw) To: qemu-devel Cc: Laurent Vivier, Thomas Huth, Dmitry Fleytman, Jason Wang, Li Qiang, Alexander Bulekov, Miroslav Rezanina, Philippe Mathieu-Daudé The length field is already contained in the ip6_ext_hdr structure. Check it direcly in eth_parse_ipv6_hdr() before calling _eth_get_rss_ex_dst_addr(), which gets a bit simplified. Reviewed-by: Miroslav Rezanina <mrezanin@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- net/eth.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/net/eth.c b/net/eth.c index 1e0821c5f81..7d4dd48c1ff 100644 --- a/net/eth.c +++ b/net/eth.c @@ -407,9 +407,7 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags, { struct ip6_ext_hdr_routing *rthdr = (struct ip6_ext_hdr_routing *) ext_hdr; - if ((rthdr->rtype == 2) && - (rthdr->len == sizeof(struct in6_address) / 8) && - (rthdr->segleft == 1)) { + if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) { size_t input_size = iov_size(pkt, pkt_frags); size_t bytes_read; @@ -528,10 +526,12 @@ bool eth_parse_ipv6_hdr(const struct iovec *pkt, int pkt_frags, } if (curr_ext_hdr_type == IP6_ROUTING) { - info->rss_ex_dst_valid = - _eth_get_rss_ex_dst_addr(pkt, pkt_frags, - ip6hdr_off + info->full_hdr_len, - &ext_hdr, &info->rss_ex_dst); + if (ext_hdr.ip6r_len == sizeof(struct in6_address) / 8) { + info->rss_ex_dst_valid = + _eth_get_rss_ex_dst_addr(pkt, pkt_frags, + ip6hdr_off + info->full_hdr_len, + &ext_hdr, &info->rss_ex_dst); + } } else if (curr_ext_hdr_type == IP6_DESTINATON) { info->rss_ex_src_valid = _eth_get_rss_ex_src_addr(pkt, pkt_frags, -- 2.26.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] net/eth: Simplify _eth_get_rss_ex_dst_addr() 2021-01-26 11:18 ` [PATCH v3 1/2] net/eth: Simplify _eth_get_rss_ex_dst_addr() Philippe Mathieu-Daudé @ 2021-01-26 13:38 ` Stefano Garzarella 0 siblings, 0 replies; 6+ messages in thread From: Stefano Garzarella @ 2021-01-26 13:38 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Laurent Vivier, Thomas Huth, Dmitry Fleytman, Jason Wang, Li Qiang, qemu-devel, Alexander Bulekov, Miroslav Rezanina On Tue, Jan 26, 2021 at 12:18:46PM +0100, Philippe Mathieu-Daudé wrote: >The length field is already contained in the ip6_ext_hdr structure. >Check it direcly in eth_parse_ipv6_hdr() before calling >_eth_get_rss_ex_dst_addr(), which gets a bit simplified. > >Reviewed-by: Miroslav Rezanina <mrezanin@redhat.com> >Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >--- > net/eth.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> > >diff --git a/net/eth.c b/net/eth.c >index 1e0821c5f81..7d4dd48c1ff 100644 >--- a/net/eth.c >+++ b/net/eth.c >@@ -407,9 +407,7 @@ _eth_get_rss_ex_dst_addr(const struct iovec *pkt, int pkt_frags, > { > struct ip6_ext_hdr_routing *rthdr = (struct ip6_ext_hdr_routing *) ext_hdr; > >- if ((rthdr->rtype == 2) && >- (rthdr->len == sizeof(struct in6_address) / 8) && >- (rthdr->segleft == 1)) { >+ if ((rthdr->rtype == 2) && (rthdr->segleft == 1)) { > > size_t input_size = iov_size(pkt, pkt_frags); > size_t bytes_read; >@@ -528,10 +526,12 @@ bool eth_parse_ipv6_hdr(const struct iovec *pkt, int pkt_frags, > } > > if (curr_ext_hdr_type == IP6_ROUTING) { >- info->rss_ex_dst_valid = >- _eth_get_rss_ex_dst_addr(pkt, pkt_frags, >- ip6hdr_off + info->full_hdr_len, >- &ext_hdr, &info->rss_ex_dst); >+ if (ext_hdr.ip6r_len == sizeof(struct in6_address) / 8) { >+ info->rss_ex_dst_valid = >+ _eth_get_rss_ex_dst_addr(pkt, pkt_frags, >+ ip6hdr_off + info->full_hdr_len, >+ &ext_hdr, &info->rss_ex_dst); >+ } > } else if (curr_ext_hdr_type == IP6_DESTINATON) { > info->rss_ex_src_valid = > _eth_get_rss_ex_src_addr(pkt, pkt_frags, >-- >2.26.2 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr() 2021-01-26 11:18 [PATCH v3 0/2] net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr() Philippe Mathieu-Daudé 2021-01-26 11:18 ` [PATCH v3 1/2] net/eth: Simplify _eth_get_rss_ex_dst_addr() Philippe Mathieu-Daudé @ 2021-01-26 11:18 ` Philippe Mathieu-Daudé 2021-01-26 14:35 ` Stefano Garzarella 1 sibling, 1 reply; 6+ messages in thread From: Philippe Mathieu-Daudé @ 2021-01-26 11:18 UTC (permalink / raw) To: qemu-devel Cc: Laurent Vivier, Thomas Huth, Dmitry Fleytman, Jason Wang, Li Qiang, qemu-stable, Alexander Bulekov, Paolo Bonzini, Miroslav Rezanina, Philippe Mathieu-Daudé 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); } 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', -- 2.26.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr() 2021-01-26 11:18 ` [PATCH v3 2/2] net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr() Philippe Mathieu-Daudé @ 2021-01-26 14:35 ` Stefano Garzarella 2021-03-09 18:08 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 6+ messages in thread From: Stefano Garzarella @ 2021-01-26 14:35 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Laurent Vivier, Thomas Huth, Dmitry Fleytman, Jason Wang, Li Qiang, qemu-devel, qemu-stable, Alexander Bulekov, Paolo Bonzini, Miroslav Rezanina 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr() 2021-01-26 14:35 ` Stefano Garzarella @ 2021-03-09 18:08 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 6+ messages in thread From: Philippe Mathieu-Daudé @ 2021-03-09 18:08 UTC (permalink / raw) To: Stefano Garzarella Cc: Laurent Vivier, Thomas Huth, Dmitry Fleytman, Jason Wang, Li Qiang, qemu-devel, qemu-stable, Alexander Bulekov, Paolo Bonzini, Miroslav Rezanina 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-09 19:49 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-26 11:18 [PATCH v3 0/2] net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr() Philippe Mathieu-Daudé 2021-01-26 11:18 ` [PATCH v3 1/2] net/eth: Simplify _eth_get_rss_ex_dst_addr() Philippe Mathieu-Daudé 2021-01-26 13:38 ` Stefano Garzarella 2021-01-26 11:18 ` [PATCH v3 2/2] net/eth: Fix stack-buffer-overflow in _eth_get_rss_ex_dst_addr() Philippe Mathieu-Daudé 2021-01-26 14:35 ` Stefano Garzarella 2021-03-09 18:08 ` Philippe Mathieu-Daudé
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.