* [PATCH] net: eepro100: validate various address values @ 2021-02-18 14:06 P J P 2021-02-18 14:18 ` no-reply 2021-02-18 14:41 ` Peter Maydell 0 siblings, 2 replies; 14+ messages in thread From: P J P @ 2021-02-18 14:06 UTC (permalink / raw) To: Stefan Weil Cc: Jason Wang, Ruhr-University Bochum, QEMU Developers, Prasad J Pandit From: Prasad J Pandit <pjp@fedoraproject.org> While processing controller commands, eepro100 emulator gets command unit(CU) base address OR receive unit (RU) base address OR command block (CB) address from guest. If these values are not checked, it may lead to an infinite loop kind of issues. Add checks to avoid it. Reported-by: Ruhr-University Bochum <bugs-syssec@rub.de> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> --- hw/net/eepro100.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c index 16e95ef9cc..afa1c9b2aa 100644 --- a/hw/net/eepro100.c +++ b/hw/net/eepro100.c @@ -843,7 +843,8 @@ static void action_command(EEPRO100State *s) bool bit_i; bool bit_nc; uint16_t ok_status = STATUS_OK; - s->cb_address = s->cu_base + s->cu_offset; + s->cb_address = s->cu_base + s->cu_offset; /* uint32_t overflow */ + assert (s->cb_address >= s->cu_base); read_cb(s); bit_el = ((s->tx.command & COMMAND_EL) != 0); bit_s = ((s->tx.command & COMMAND_S) != 0); @@ -860,6 +861,7 @@ static void action_command(EEPRO100State *s) } s->cu_offset = s->tx.link; + assert(s->cu_offset > 0); TRACE(OTHER, logout("val=(cu start), status=0x%04x, command=0x%04x, link=0x%08x\n", s->tx.status, s->tx.command, s->tx.link)); @@ -990,8 +992,10 @@ static void eepro100_cu_command(EEPRO100State * s, uint8_t val) break; case CU_CMD_BASE: /* Load CU base. */ + assert(get_cu_state(s) == cu_idle); TRACE(OTHER, logout("val=0x%02x (CU base address)\n", val)); s->cu_base = e100_read_reg4(s, SCBPointer); + assert(!s->cu_base); break; case CU_DUMPSTATS: /* Dump and reset statistical counters. */ @@ -1048,8 +1052,10 @@ static void eepro100_ru_command(EEPRO100State * s, uint8_t val) break; case RX_ADDR_LOAD: /* Load RU base. */ + assert(get_ru_state(s) == ru_idle); TRACE(OTHER, logout("val=0x%02x (RU base address)\n", val)); s->ru_base = e100_read_reg4(s, SCBPointer); + assert(!s->ru_base); break; default: logout("val=0x%02x (undefined RU command)\n", val); -- 2.29.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] net: eepro100: validate various address values 2021-02-18 14:06 [PATCH] net: eepro100: validate various address values P J P @ 2021-02-18 14:18 ` no-reply 2021-02-18 14:41 ` Peter Maydell 1 sibling, 0 replies; 14+ messages in thread From: no-reply @ 2021-02-18 14:18 UTC (permalink / raw) To: ppandit; +Cc: sw, jasowang, bugs-syssec, qemu-devel, pjp Patchew URL: https://patchew.org/QEMU/20210218140629.373646-1-ppandit@redhat.com/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20210218140629.373646-1-ppandit@redhat.com Subject: [PATCH] net: eepro100: validate various address values === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/20210218140629.373646-1-ppandit@redhat.com -> patchew/20210218140629.373646-1-ppandit@redhat.com Switched to a new branch 'test' 1cccc2d net: eepro100: validate various address values === OUTPUT BEGIN === ERROR: space prohibited between function name and open parenthesis '(' #30: FILE: hw/net/eepro100.c:847: + assert (s->cb_address >= s->cu_base); total: 1 errors, 0 warnings, 36 lines checked Commit 1cccc2d7c7da (net: eepro100: validate various address values) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20210218140629.373646-1-ppandit@redhat.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: eepro100: validate various address values 2021-02-18 14:06 [PATCH] net: eepro100: validate various address values P J P 2021-02-18 14:18 ` no-reply @ 2021-02-18 14:41 ` Peter Maydell 2021-02-18 16:10 ` Stefan Weil 2021-02-19 1:54 ` Alexander Bulekov 1 sibling, 2 replies; 14+ messages in thread From: Peter Maydell @ 2021-02-18 14:41 UTC (permalink / raw) To: P J P Cc: Stefan Weil, Jason Wang, Ruhr-University Bochum, QEMU Developers, Prasad J Pandit On Thu, 18 Feb 2021 at 14:13, P J P <ppandit@redhat.com> wrote: > > From: Prasad J Pandit <pjp@fedoraproject.org> > > While processing controller commands, eepro100 emulator gets > command unit(CU) base address OR receive unit (RU) base address > OR command block (CB) address from guest. If these values are not > checked, it may lead to an infinite loop kind of issues. Add checks > to avoid it. > > Reported-by: Ruhr-University Bochum <bugs-syssec@rub.de> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/net/eepro100.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c > index 16e95ef9cc..afa1c9b2aa 100644 > --- a/hw/net/eepro100.c > +++ b/hw/net/eepro100.c > @@ -843,7 +843,8 @@ static void action_command(EEPRO100State *s) > bool bit_i; > bool bit_nc; > uint16_t ok_status = STATUS_OK; > - s->cb_address = s->cu_base + s->cu_offset; > + s->cb_address = s->cu_base + s->cu_offset; /* uint32_t overflow */ > + assert (s->cb_address >= s->cu_base); We get these values from the guest; you can't just assert() on them. You need to do something else. My reading of the 8255x data sheet is that there is nothing in the hardware that forbids the guest from programming the device such that the cu_base + cu_offset wraps around: http://www.intel.com/content/dam/doc/manual/8255x-10-100-mbps-ethernet-controller-software-dev-manual.pdf -- page 30 says that this is all doing 32-bit arithmetic on addresses and doesn't say that there is any special case handling by the device of overflow of that addition. Your commit message isn't very clear about what the failure case is here, but I think the fix has to be something different from this. thanks -- PMM ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: eepro100: validate various address values 2021-02-18 14:41 ` Peter Maydell @ 2021-02-18 16:10 ` Stefan Weil 2021-02-19 1:54 ` Alexander Bulekov 1 sibling, 0 replies; 14+ messages in thread From: Stefan Weil @ 2021-02-18 16:10 UTC (permalink / raw) To: Peter Maydell, P J P Cc: Jason Wang, Ruhr-University Bochum, QEMU Developers, Prasad J Pandit Am 18.02.21 um 15:41 schrieb Peter Maydell: > On Thu, 18 Feb 2021 at 14:13, P J P <ppandit@redhat.com> wrote: >> From: Prasad J Pandit <pjp@fedoraproject.org> >> >> While processing controller commands, eepro100 emulator gets >> command unit(CU) base address OR receive unit (RU) base address >> OR command block (CB) address from guest. If these values are not >> checked, it may lead to an infinite loop kind of issues. Add checks >> to avoid it. >> >> Reported-by: Ruhr-University Bochum <bugs-syssec@rub.de> >> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> >> --- >> hw/net/eepro100.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c >> index 16e95ef9cc..afa1c9b2aa 100644 >> --- a/hw/net/eepro100.c >> +++ b/hw/net/eepro100.c >> @@ -843,7 +843,8 @@ static void action_command(EEPRO100State *s) >> bool bit_i; >> bool bit_nc; >> uint16_t ok_status = STATUS_OK; >> - s->cb_address = s->cu_base + s->cu_offset; >> + s->cb_address = s->cu_base + s->cu_offset; /* uint32_t overflow */ >> + assert (s->cb_address >= s->cu_base); > We get these values from the guest; you can't just assert() on them. > You need to do something else. > > My reading of the 8255x data sheet is that there is nothing > in the hardware that forbids the guest from programming the > device such that the cu_base + cu_offset wraps around: > http://www.intel.com/content/dam/doc/manual/8255x-10-100-mbps-ethernet-controller-software-dev-manual.pdf > -- page 30 says that this is all doing 32-bit arithmetic > on addresses and doesn't say that there is any special case > handling by the device of overflow of that addition. > > Your commit message isn't very clear about what the failure > case is here, but I think the fix has to be something > different from this. I agree with Peter. The hardware emulation in QEMU should try to do the same as the real hardware. Assertions (not with assert(), but with g_assert() and related functions) are good to catch implementation errors in QEMU code, but not to catch bad programming in guest code. In this special case the emulation code already includes a hack to catch endless loops caused by buggy guest code: it has a limit of 16 cycles and then prints a log message. This is a compromise to emulate the real hardware as good as possible without making QEMU running an endless loop or requiring an extra thread to emulate the hardware loop until it is stopped by new I/O operations. Stefan ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: eepro100: validate various address values 2021-02-18 14:41 ` Peter Maydell 2021-02-18 16:10 ` Stefan Weil @ 2021-02-19 1:54 ` Alexander Bulekov 2021-02-19 2:06 ` Li Qiang 2021-02-19 6:11 ` P J P 1 sibling, 2 replies; 14+ messages in thread From: Alexander Bulekov @ 2021-02-19 1:54 UTC (permalink / raw) To: Peter Maydell Cc: Prasad J Pandit, Stefan Weil, Jason Wang, QEMU Developers, P J P, Ruhr-University Bochum On 210218 1441, Peter Maydell wrote: > On Thu, 18 Feb 2021 at 14:13, P J P <ppandit@redhat.com> wrote: > > > > From: Prasad J Pandit <pjp@fedoraproject.org> > > > > While processing controller commands, eepro100 emulator gets > > command unit(CU) base address OR receive unit (RU) base address > > OR command block (CB) address from guest. If these values are not > > checked, it may lead to an infinite loop kind of issues. Add checks > > to avoid it. > > > > Reported-by: Ruhr-University Bochum <bugs-syssec@rub.de> > > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > > --- > > hw/net/eepro100.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c > > index 16e95ef9cc..afa1c9b2aa 100644 > > --- a/hw/net/eepro100.c > > +++ b/hw/net/eepro100.c > > @@ -843,7 +843,8 @@ static void action_command(EEPRO100State *s) > > bool bit_i; > > bool bit_nc; > > uint16_t ok_status = STATUS_OK; > > - s->cb_address = s->cu_base + s->cu_offset; > > + s->cb_address = s->cu_base + s->cu_offset; /* uint32_t overflow */ > > + assert (s->cb_address >= s->cu_base); > > We get these values from the guest; you can't just assert() on them. > You need to do something else. > > My reading of the 8255x data sheet is that there is nothing > in the hardware that forbids the guest from programming the > device such that the cu_base + cu_offset wraps around: > http://www.intel.com/content/dam/doc/manual/8255x-10-100-mbps-ethernet-controller-software-dev-manual.pdf > -- page 30 says that this is all doing 32-bit arithmetic > on addresses and doesn't say that there is any special case > handling by the device of overflow of that addition. > > Your commit message isn't very clear about what the failure > case is here, but I think the fix has to be something > different from this. Maybe the infinite loop mentioned in the commit message is actually a DMA recursion issue? I'm providing a reproducer for a DMA re-entracy issue below. With this patch applied, the reproducer triggers the assert(), rather than overflowing the stack, so maybe it is the same issue? -Alex cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m \ 512M -device i82559er,netdev=net0 -netdev user,id=net0 -nodefaults \ -qtest stdio outl 0xcf8 0x80001014 outl 0xcfc 0xc000 outl 0xcf8 0x80001010 outl 0xcfc 0xe0020000 outl 0xcf8 0x80001004 outw 0xcfc 0x7 write 0x1ffffc0b 0x1 0x55 write 0x1ffffc0c 0x1 0xfc write 0x1ffffc0d 0x1 0x46 write 0x1ffffc0e 0x1 0x07 write 0x746fc59 0x1 0x02 write 0x746fc5b 0x1 0x02 write 0x746fc5c 0x1 0xe0 write 0x4 0x1 0x07 write 0x5 0x1 0xfc write 0x6 0x1 0xff write 0x7 0x1 0x1f outw 0xc002 0x20 EOF Formatted for committing a regression-test: static void test_fuzz(void) { QTestState *s = qtest_init("-display none , -m 512M -device i82559er,netdev=net0 " "-netdev user,id=net0 -nodefaults"); qtest_outl(s, 0xcf8, 0x80001014); qtest_outl(s, 0xcfc, 0xc000); qtest_outl(s, 0xcf8, 0x80001010); qtest_outl(s, 0xcfc, 0xe0020000); qtest_outl(s, 0xcf8, 0x80001004); qtest_outw(s, 0xcfc, 0x7); qtest_bufwrite(s, 0x1ffffc0b, "\x55", 0x1); qtest_bufwrite(s, 0x1ffffc0c, "\xfc", 0x1); qtest_bufwrite(s, 0x1ffffc0d, "\x46", 0x1); qtest_bufwrite(s, 0x1ffffc0e, "\x07", 0x1); qtest_bufwrite(s, 0x746fc59, "\x02", 0x1); qtest_bufwrite(s, 0x746fc5b, "\x02", 0x1); qtest_bufwrite(s, 0x746fc5c, "\xe0", 0x1); qtest_bufwrite(s, 0x4, "\x07", 0x1); qtest_bufwrite(s, 0x5, "\xfc", 0x1); qtest_bufwrite(s, 0x6, "\xff", 0x1); qtest_bufwrite(s, 0x7, "\x1f", 0x1); qtest_outw(s, 0xc002, 0x20); qtest_quit(s); } > > thanks > -- PMM > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: eepro100: validate various address values 2021-02-19 1:54 ` Alexander Bulekov @ 2021-02-19 2:06 ` Li Qiang 2021-02-19 2:14 ` Alexander Bulekov 2021-02-19 6:11 ` P J P 1 sibling, 1 reply; 14+ messages in thread From: Li Qiang @ 2021-02-19 2:06 UTC (permalink / raw) To: Alexander Bulekov Cc: Peter Maydell, Prasad J Pandit, Stefan Weil, Jason Wang, QEMU Developers, P J P, Ruhr-University Bochum Alexander Bulekov <alxndr@bu.edu> 于2021年2月19日周五 上午9:56写道： > > On 210218 1441, Peter Maydell wrote: > > On Thu, 18 Feb 2021 at 14:13, P J P <ppandit@redhat.com> wrote: > > > > > > From: Prasad J Pandit <pjp@fedoraproject.org> > > > > > > While processing controller commands, eepro100 emulator gets > > > command unit(CU) base address OR receive unit (RU) base address > > > OR command block (CB) address from guest. If these values are not > > > checked, it may lead to an infinite loop kind of issues. Add checks > > > to avoid it. So could you please provide a backtrack? Thanks, Li Qiang > > > > > > Reported-by: Ruhr-University Bochum <bugs-syssec@rub.de> > > > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > > > --- > > > hw/net/eepro100.c | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c > > > index 16e95ef9cc..afa1c9b2aa 100644 > > > --- a/hw/net/eepro100.c > > > +++ b/hw/net/eepro100.c > > > @@ -843,7 +843,8 @@ static void action_command(EEPRO100State *s) > > > bool bit_i; > > > bool bit_nc; > > > uint16_t ok_status = STATUS_OK; > > > - s->cb_address = s->cu_base + s->cu_offset; > > > + s->cb_address = s->cu_base + s->cu_offset; /* uint32_t overflow */ > > > + assert (s->cb_address >= s->cu_base); > > > > We get these values from the guest; you can't just assert() on them. > > You need to do something else. > > > > My reading of the 8255x data sheet is that there is nothing > > in the hardware that forbids the guest from programming the > > device such that the cu_base + cu_offset wraps around: > > http://www.intel.com/content/dam/doc/manual/8255x-10-100-mbps-ethernet-controller-software-dev-manual.pdf > > -- page 30 says that this is all doing 32-bit arithmetic > > on addresses and doesn't say that there is any special case > > handling by the device of overflow of that addition. > > > > Your commit message isn't very clear about what the failure > > case is here, but I think the fix has to be something > > different from this. > > Maybe the infinite loop mentioned in the commit message is actually a > DMA recursion issue? I'm providing a reproducer for a DMA re-entracy > issue below. With this patch applied, the reproducer triggers the > assert(), rather than overflowing the stack, so maybe it is the same > issue? > -Alex > > cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m \ > 512M -device i82559er,netdev=net0 -netdev user,id=net0 -nodefaults \ > -qtest stdio > outl 0xcf8 0x80001014 > outl 0xcfc 0xc000 > outl 0xcf8 0x80001010 > outl 0xcfc 0xe0020000 > outl 0xcf8 0x80001004 > outw 0xcfc 0x7 > write 0x1ffffc0b 0x1 0x55 > write 0x1ffffc0c 0x1 0xfc > write 0x1ffffc0d 0x1 0x46 > write 0x1ffffc0e 0x1 0x07 > write 0x746fc59 0x1 0x02 > write 0x746fc5b 0x1 0x02 > write 0x746fc5c 0x1 0xe0 > write 0x4 0x1 0x07 > write 0x5 0x1 0xfc > write 0x6 0x1 0xff > write 0x7 0x1 0x1f > outw 0xc002 0x20 > EOF > > Formatted for committing a regression-test: > > static void test_fuzz(void) > { > QTestState *s = > qtest_init("-display none , -m 512M -device i82559er,netdev=net0 " > "-netdev user,id=net0 -nodefaults"); > qtest_outl(s, 0xcf8, 0x80001014); > qtest_outl(s, 0xcfc, 0xc000); > qtest_outl(s, 0xcf8, 0x80001010); > qtest_outl(s, 0xcfc, 0xe0020000); > qtest_outl(s, 0xcf8, 0x80001004); > qtest_outw(s, 0xcfc, 0x7); > qtest_bufwrite(s, 0x1ffffc0b, "\x55", 0x1); > qtest_bufwrite(s, 0x1ffffc0c, "\xfc", 0x1); > qtest_bufwrite(s, 0x1ffffc0d, "\x46", 0x1); > qtest_bufwrite(s, 0x1ffffc0e, "\x07", 0x1); > qtest_bufwrite(s, 0x746fc59, "\x02", 0x1); > qtest_bufwrite(s, 0x746fc5b, "\x02", 0x1); > qtest_bufwrite(s, 0x746fc5c, "\xe0", 0x1); > qtest_bufwrite(s, 0x4, "\x07", 0x1); > qtest_bufwrite(s, 0x5, "\xfc", 0x1); > qtest_bufwrite(s, 0x6, "\xff", 0x1); > qtest_bufwrite(s, 0x7, "\x1f", 0x1); > qtest_outw(s, 0xc002, 0x20); > qtest_quit(s); > } > > > > > > thanks > > -- PMM > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: eepro100: validate various address values 2021-02-19 2:06 ` Li Qiang @ 2021-02-19 2:14 ` Alexander Bulekov 2021-02-19 4:43 ` Li Qiang 0 siblings, 1 reply; 14+ messages in thread From: Alexander Bulekov @ 2021-02-19 2:14 UTC (permalink / raw) To: Li Qiang Cc: Peter Maydell, Prasad J Pandit, Stefan Weil, Jason Wang, QEMU Developers, P J P, Ruhr-University Bochum On 210219 1006, Li Qiang wrote: > Alexander Bulekov <alxndr@bu.edu> 于2021年2月19日周五 上午9:56写道： > > > > On 210218 1441, Peter Maydell wrote: > > > On Thu, 18 Feb 2021 at 14:13, P J P <ppandit@redhat.com> wrote: > > > > > > > > From: Prasad J Pandit <pjp@fedoraproject.org> > > > > > > > > While processing controller commands, eepro100 emulator gets > > > > command unit(CU) base address OR receive unit (RU) base address > > > > OR command block (CB) address from guest. If these values are not > > > > checked, it may lead to an infinite loop kind of issues. Add checks > > > > to avoid it. > > > So could you please provide a backtrack? > I don't know if you are asking me or Prasad, but here is the stacktrace for the one I provided: ==2715275==ERROR: AddressSanitizer: stack-overflow on address 0x7ffc5262ba28 (pc 0x55d83b103ac6 bp 0x7ffc5262c270 sp 0x7ffc5262ba30 T0) #0 in __asan_memcpy (qemu-system-i386+0x2aa3ac6) #1 in flatview_do_translate ../softmmu/physmem.c:518:12 #2 in flatview_translate ../softmmu/physmem.c:568:15 #3 in flatview_read ../softmmu/physmem.c:2878:10 #4 in address_space_read_full ../softmmu/physmem.c:2892:18 #5 in dma_memory_rw_relaxed include/sysemu/dma.h:88:12 #6 in dma_memory_rw include/sysemu/dma.h:127:12 #7 in pci_dma_rw include/hw/pci/pci.h:803:12 #8 in pci_dma_read include/hw/pci/pci.h:821:12 #9 in read_cb ../hw/net/eepro100.c:726:5 #10 in action_command ../hw/net/eepro100.c:847:9 #11 in eepro100_cu_command ../hw/net/eepro100.c:969:13 #12 in eepro100_write_command ../hw/net/eepro100.c:1063:5 #13 in eepro100_write2 ../hw/net/eepro100.c:1510:9 #14 in eepro100_write ../hw/net/eepro100.c:1593:9 #15 in memory_region_write_accessor ../softmmu/memory.c:491:5 #16 in access_with_adjusted_size ../softmmu/memory.c:552:18 #17 in memory_region_dispatch_write ../softmmu/memory.c #18 in flatview_write_continue ../softmmu/physmem.c:2776:23 #19 in flatview_write ../softmmu/physmem.c:2816:14 #20 in address_space_write ../softmmu/physmem.c:2908:18 #21 in dma_memory_rw_relaxed include/sysemu/dma.h:88:12 #22 in dma_memory_rw include/sysemu/dma.h:127:12 #23 in dma_memory_write include/sysemu/dma.h:163:12 #24 in stw_le_dma include/sysemu/dma.h:259:1 #25 in stw_le_pci_dma include/hw/pci/pci.h:855:1 #26 in action_command ../hw/net/eepro100.c:913:9 #27 in eepro100_cu_command ../hw/net/eepro100.c:969:13 #28 in eepro100_write_command ../hw/net/eepro100.c:1063:5 #29 in eepro100_write2 ../hw/net/eepro100.c:1510:9 #30 in eepro100_write ../hw/net/eepro100.c:1593:9 ... till there's no more stack ... > > Thanks, > Li Qiang > > > > > > > > > Reported-by: Ruhr-University Bochum <bugs-syssec@rub.de> > > > > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > > > > --- > > > > hw/net/eepro100.c | 8 +++++++- > > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c > > > > index 16e95ef9cc..afa1c9b2aa 100644 > > > > --- a/hw/net/eepro100.c > > > > +++ b/hw/net/eepro100.c > > > > @@ -843,7 +843,8 @@ static void action_command(EEPRO100State *s) > > > > bool bit_i; > > > > bool bit_nc; > > > > uint16_t ok_status = STATUS_OK; > > > > - s->cb_address = s->cu_base + s->cu_offset; > > > > + s->cb_address = s->cu_base + s->cu_offset; /* uint32_t overflow */ > > > > + assert (s->cb_address >= s->cu_base); > > > > > > We get these values from the guest; you can't just assert() on them. > > > You need to do something else. > > > > > > My reading of the 8255x data sheet is that there is nothing > > > in the hardware that forbids the guest from programming the > > > device such that the cu_base + cu_offset wraps around: > > > http://www.intel.com/content/dam/doc/manual/8255x-10-100-mbps-ethernet-controller-software-dev-manual.pdf > > > -- page 30 says that this is all doing 32-bit arithmetic > > > on addresses and doesn't say that there is any special case > > > handling by the device of overflow of that addition. > > > > > > Your commit message isn't very clear about what the failure > > > case is here, but I think the fix has to be something > > > different from this. > > > > Maybe the infinite loop mentioned in the commit message is actually a > > DMA recursion issue? I'm providing a reproducer for a DMA re-entracy > > issue below. With this patch applied, the reproducer triggers the > > assert(), rather than overflowing the stack, so maybe it is the same > > issue? > > -Alex > > > > cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m \ > > 512M -device i82559er,netdev=net0 -netdev user,id=net0 -nodefaults \ > > -qtest stdio > > outl 0xcf8 0x80001014 > > outl 0xcfc 0xc000 > > outl 0xcf8 0x80001010 > > outl 0xcfc 0xe0020000 > > outl 0xcf8 0x80001004 > > outw 0xcfc 0x7 > > write 0x1ffffc0b 0x1 0x55 > > write 0x1ffffc0c 0x1 0xfc > > write 0x1ffffc0d 0x1 0x46 > > write 0x1ffffc0e 0x1 0x07 > > write 0x746fc59 0x1 0x02 > > write 0x746fc5b 0x1 0x02 > > write 0x746fc5c 0x1 0xe0 > > write 0x4 0x1 0x07 > > write 0x5 0x1 0xfc > > write 0x6 0x1 0xff > > write 0x7 0x1 0x1f > > outw 0xc002 0x20 > > EOF > > > > Formatted for committing a regression-test: > > > > static void test_fuzz(void) > > { > > QTestState *s = > > qtest_init("-display none , -m 512M -device i82559er,netdev=net0 " > > "-netdev user,id=net0 -nodefaults"); > > qtest_outl(s, 0xcf8, 0x80001014); > > qtest_outl(s, 0xcfc, 0xc000); > > qtest_outl(s, 0xcf8, 0x80001010); > > qtest_outl(s, 0xcfc, 0xe0020000); > > qtest_outl(s, 0xcf8, 0x80001004); > > qtest_outw(s, 0xcfc, 0x7); > > qtest_bufwrite(s, 0x1ffffc0b, "\x55", 0x1); > > qtest_bufwrite(s, 0x1ffffc0c, "\xfc", 0x1); > > qtest_bufwrite(s, 0x1ffffc0d, "\x46", 0x1); > > qtest_bufwrite(s, 0x1ffffc0e, "\x07", 0x1); > > qtest_bufwrite(s, 0x746fc59, "\x02", 0x1); > > qtest_bufwrite(s, 0x746fc5b, "\x02", 0x1); > > qtest_bufwrite(s, 0x746fc5c, "\xe0", 0x1); > > qtest_bufwrite(s, 0x4, "\x07", 0x1); > > qtest_bufwrite(s, 0x5, "\xfc", 0x1); > > qtest_bufwrite(s, 0x6, "\xff", 0x1); > > qtest_bufwrite(s, 0x7, "\x1f", 0x1); > > qtest_outw(s, 0xc002, 0x20); > > qtest_quit(s); > > } > > > > > > > > > > thanks > > > -- PMM > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: eepro100: validate various address values 2021-02-19 2:14 ` Alexander Bulekov @ 2021-02-19 4:43 ` Li Qiang 2021-02-20 3:05 ` Alexander Bulekov 0 siblings, 1 reply; 14+ messages in thread From: Li Qiang @ 2021-02-19 4:43 UTC (permalink / raw) To: Alexander Bulekov Cc: Peter Maydell, Prasad J Pandit, Stefan Weil, Jason Wang, QEMU Developers, P J P, Ruhr-University Bochum Alexander Bulekov <alxndr@bu.edu> 于2021年2月19日周五 上午10:15写道： > > On 210219 1006, Li Qiang wrote: > > Alexander Bulekov <alxndr@bu.edu> 于2021年2月19日周五 上午9:56写道： > > > > > > On 210218 1441, Peter Maydell wrote: > > > > On Thu, 18 Feb 2021 at 14:13, P J P <ppandit@redhat.com> wrote: > > > > > > > > > > From: Prasad J Pandit <pjp@fedoraproject.org> > > > > > > > > > > While processing controller commands, eepro100 emulator gets > > > > > command unit(CU) base address OR receive unit (RU) base address > > > > > OR command block (CB) address from guest. If these values are not > > > > > checked, it may lead to an infinite loop kind of issues. Add checks > > > > > to avoid it. > > > > > > So could you please provide a backtrack? > > > > I don't know if you are asking me or Prasad, but here is the stacktrace Yes, a typical DMA reentry issue. Any progress to solve these DMA reentry issues? seems more and more this kind of issues. Just return from the busy things as a new father and not focus this quite a time. Thanks, Li Qiang > for the one I provided: > ==2715275==ERROR: AddressSanitizer: stack-overflow on address > 0x7ffc5262ba28 (pc 0x55d83b103ac6 bp 0x7ffc5262c270 sp 0x7ffc5262ba30 > T0) > #0 in __asan_memcpy (qemu-system-i386+0x2aa3ac6) > #1 in flatview_do_translate ../softmmu/physmem.c:518:12 > #2 in flatview_translate ../softmmu/physmem.c:568:15 > #3 in flatview_read ../softmmu/physmem.c:2878:10 > #4 in address_space_read_full ../softmmu/physmem.c:2892:18 > #5 in dma_memory_rw_relaxed include/sysemu/dma.h:88:12 > #6 in dma_memory_rw include/sysemu/dma.h:127:12 > #7 in pci_dma_rw include/hw/pci/pci.h:803:12 > #8 in pci_dma_read include/hw/pci/pci.h:821:12 > #9 in read_cb ../hw/net/eepro100.c:726:5 > #10 in action_command ../hw/net/eepro100.c:847:9 > #11 in eepro100_cu_command ../hw/net/eepro100.c:969:13 > #12 in eepro100_write_command ../hw/net/eepro100.c:1063:5 > #13 in eepro100_write2 ../hw/net/eepro100.c:1510:9 > #14 in eepro100_write ../hw/net/eepro100.c:1593:9 > #15 in memory_region_write_accessor ../softmmu/memory.c:491:5 > #16 in access_with_adjusted_size ../softmmu/memory.c:552:18 > #17 in memory_region_dispatch_write ../softmmu/memory.c > #18 in flatview_write_continue ../softmmu/physmem.c:2776:23 > #19 in flatview_write ../softmmu/physmem.c:2816:14 > #20 in address_space_write ../softmmu/physmem.c:2908:18 > #21 in dma_memory_rw_relaxed include/sysemu/dma.h:88:12 > #22 in dma_memory_rw include/sysemu/dma.h:127:12 > #23 in dma_memory_write include/sysemu/dma.h:163:12 > #24 in stw_le_dma include/sysemu/dma.h:259:1 > #25 in stw_le_pci_dma include/hw/pci/pci.h:855:1 > #26 in action_command ../hw/net/eepro100.c:913:9 > #27 in eepro100_cu_command ../hw/net/eepro100.c:969:13 > #28 in eepro100_write_command ../hw/net/eepro100.c:1063:5 > #29 in eepro100_write2 ../hw/net/eepro100.c:1510:9 > #30 in eepro100_write ../hw/net/eepro100.c:1593:9 > ... till there's no more stack ... > > > > > Thanks, > > Li Qiang > > > > > > > > > > > > Reported-by: Ruhr-University Bochum <bugs-syssec@rub.de> > > > > > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > > > > > --- > > > > > hw/net/eepro100.c | 8 +++++++- > > > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c > > > > > index 16e95ef9cc..afa1c9b2aa 100644 > > > > > --- a/hw/net/eepro100.c > > > > > +++ b/hw/net/eepro100.c > > > > > @@ -843,7 +843,8 @@ static void action_command(EEPRO100State *s) > > > > > bool bit_i; > > > > > bool bit_nc; > > > > > uint16_t ok_status = STATUS_OK; > > > > > - s->cb_address = s->cu_base + s->cu_offset; > > > > > + s->cb_address = s->cu_base + s->cu_offset; /* uint32_t overflow */ > > > > > + assert (s->cb_address >= s->cu_base); > > > > > > > > We get these values from the guest; you can't just assert() on them. > > > > You need to do something else. > > > > > > > > My reading of the 8255x data sheet is that there is nothing > > > > in the hardware that forbids the guest from programming the > > > > device such that the cu_base + cu_offset wraps around: > > > > http://www.intel.com/content/dam/doc/manual/8255x-10-100-mbps-ethernet-controller-software-dev-manual.pdf > > > > -- page 30 says that this is all doing 32-bit arithmetic > > > > on addresses and doesn't say that there is any special case > > > > handling by the device of overflow of that addition. > > > > > > > > Your commit message isn't very clear about what the failure > > > > case is here, but I think the fix has to be something > > > > different from this. > > > > > > Maybe the infinite loop mentioned in the commit message is actually a > > > DMA recursion issue? I'm providing a reproducer for a DMA re-entracy > > > issue below. With this patch applied, the reproducer triggers the > > > assert(), rather than overflowing the stack, so maybe it is the same > > > issue? > > > -Alex > > > > > > cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m \ > > > 512M -device i82559er,netdev=net0 -netdev user,id=net0 -nodefaults \ > > > -qtest stdio > > > outl 0xcf8 0x80001014 > > > outl 0xcfc 0xc000 > > > outl 0xcf8 0x80001010 > > > outl 0xcfc 0xe0020000 > > > outl 0xcf8 0x80001004 > > > outw 0xcfc 0x7 > > > write 0x1ffffc0b 0x1 0x55 > > > write 0x1ffffc0c 0x1 0xfc > > > write 0x1ffffc0d 0x1 0x46 > > > write 0x1ffffc0e 0x1 0x07 > > > write 0x746fc59 0x1 0x02 > > > write 0x746fc5b 0x1 0x02 > > > write 0x746fc5c 0x1 0xe0 > > > write 0x4 0x1 0x07 > > > write 0x5 0x1 0xfc > > > write 0x6 0x1 0xff > > > write 0x7 0x1 0x1f > > > outw 0xc002 0x20 > > > EOF > > > > > > Formatted for committing a regression-test: > > > > > > static void test_fuzz(void) > > > { > > > QTestState *s = > > > qtest_init("-display none , -m 512M -device i82559er,netdev=net0 " > > > "-netdev user,id=net0 -nodefaults"); > > > qtest_outl(s, 0xcf8, 0x80001014); > > > qtest_outl(s, 0xcfc, 0xc000); > > > qtest_outl(s, 0xcf8, 0x80001010); > > > qtest_outl(s, 0xcfc, 0xe0020000); > > > qtest_outl(s, 0xcf8, 0x80001004); > > > qtest_outw(s, 0xcfc, 0x7); > > > qtest_bufwrite(s, 0x1ffffc0b, "\x55", 0x1); > > > qtest_bufwrite(s, 0x1ffffc0c, "\xfc", 0x1); > > > qtest_bufwrite(s, 0x1ffffc0d, "\x46", 0x1); > > > qtest_bufwrite(s, 0x1ffffc0e, "\x07", 0x1); > > > qtest_bufwrite(s, 0x746fc59, "\x02", 0x1); > > > qtest_bufwrite(s, 0x746fc5b, "\x02", 0x1); > > > qtest_bufwrite(s, 0x746fc5c, "\xe0", 0x1); > > > qtest_bufwrite(s, 0x4, "\x07", 0x1); > > > qtest_bufwrite(s, 0x5, "\xfc", 0x1); > > > qtest_bufwrite(s, 0x6, "\xff", 0x1); > > > qtest_bufwrite(s, 0x7, "\x1f", 0x1); > > > qtest_outw(s, 0xc002, 0x20); > > > qtest_quit(s); > > > } > > > > > > > > > > > > > > thanks > > > > -- PMM > > > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: eepro100: validate various address values 2021-02-19 4:43 ` Li Qiang @ 2021-02-20 3:05 ` Alexander Bulekov 0 siblings, 0 replies; 14+ messages in thread From: Alexander Bulekov @ 2021-02-20 3:05 UTC (permalink / raw) To: Li Qiang Cc: Peter Maydell, Prasad J Pandit, Stefan Weil, Jason Wang, QEMU Developers, P J P, Ruhr-University Bochum On 210219 1243, Li Qiang wrote: > Alexander Bulekov <alxndr@bu.edu> 于2021年2月19日周五 上午10:15写道： > > > > On 210219 1006, Li Qiang wrote: > > > Alexander Bulekov <alxndr@bu.edu> 于2021年2月19日周五 上午9:56写道： > > > > > > > > On 210218 1441, Peter Maydell wrote: > > > > > On Thu, 18 Feb 2021 at 14:13, P J P <ppandit@redhat.com> wrote: > > > > > > > > > > > > From: Prasad J Pandit <pjp@fedoraproject.org> > > > > > > > > > > > > While processing controller commands, eepro100 emulator gets > > > > > > command unit(CU) base address OR receive unit (RU) base address > > > > > > OR command block (CB) address from guest. If these values are not > > > > > > checked, it may lead to an infinite loop kind of issues. Add checks > > > > > > to avoid it. > > > > > > > > > So could you please provide a backtrack? > > > > > > > I don't know if you are asking me or Prasad, but here is the stacktrace > > > Yes, a typical DMA reentry issue. > Any progress to solve these DMA reentry issues? seems more and more > this kind of issues. Unfortuantely, I don't think there's a solution yet. > Just return from the busy things as a new father and not focus this > quite a time. Congrats! > > Thanks, > Li Qiang > > > for the one I provided: > > ==2715275==ERROR: AddressSanitizer: stack-overflow on address > > 0x7ffc5262ba28 (pc 0x55d83b103ac6 bp 0x7ffc5262c270 sp 0x7ffc5262ba30 > > T0) > > #0 in __asan_memcpy (qemu-system-i386+0x2aa3ac6) > > #1 in flatview_do_translate ../softmmu/physmem.c:518:12 > > #2 in flatview_translate ../softmmu/physmem.c:568:15 > > #3 in flatview_read ../softmmu/physmem.c:2878:10 > > #4 in address_space_read_full ../softmmu/physmem.c:2892:18 > > #5 in dma_memory_rw_relaxed include/sysemu/dma.h:88:12 > > #6 in dma_memory_rw include/sysemu/dma.h:127:12 > > #7 in pci_dma_rw include/hw/pci/pci.h:803:12 > > #8 in pci_dma_read include/hw/pci/pci.h:821:12 > > #9 in read_cb ../hw/net/eepro100.c:726:5 > > #10 in action_command ../hw/net/eepro100.c:847:9 > > #11 in eepro100_cu_command ../hw/net/eepro100.c:969:13 > > #12 in eepro100_write_command ../hw/net/eepro100.c:1063:5 > > #13 in eepro100_write2 ../hw/net/eepro100.c:1510:9 > > #14 in eepro100_write ../hw/net/eepro100.c:1593:9 > > #15 in memory_region_write_accessor ../softmmu/memory.c:491:5 > > #16 in access_with_adjusted_size ../softmmu/memory.c:552:18 > > #17 in memory_region_dispatch_write ../softmmu/memory.c > > #18 in flatview_write_continue ../softmmu/physmem.c:2776:23 > > #19 in flatview_write ../softmmu/physmem.c:2816:14 > > #20 in address_space_write ../softmmu/physmem.c:2908:18 > > #21 in dma_memory_rw_relaxed include/sysemu/dma.h:88:12 > > #22 in dma_memory_rw include/sysemu/dma.h:127:12 > > #23 in dma_memory_write include/sysemu/dma.h:163:12 > > #24 in stw_le_dma include/sysemu/dma.h:259:1 > > #25 in stw_le_pci_dma include/hw/pci/pci.h:855:1 > > #26 in action_command ../hw/net/eepro100.c:913:9 > > #27 in eepro100_cu_command ../hw/net/eepro100.c:969:13 > > #28 in eepro100_write_command ../hw/net/eepro100.c:1063:5 > > #29 in eepro100_write2 ../hw/net/eepro100.c:1510:9 > > #30 in eepro100_write ../hw/net/eepro100.c:1593:9 > > ... till there's no more stack ... > > > > > > > > Thanks, > > > Li Qiang > > > > > > > > > > > > > > > Reported-by: Ruhr-University Bochum <bugs-syssec@rub.de> > > > > > > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > > > > > > --- > > > > > > hw/net/eepro100.c | 8 +++++++- > > > > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c > > > > > > index 16e95ef9cc..afa1c9b2aa 100644 > > > > > > --- a/hw/net/eepro100.c > > > > > > +++ b/hw/net/eepro100.c > > > > > > @@ -843,7 +843,8 @@ static void action_command(EEPRO100State *s) > > > > > > bool bit_i; > > > > > > bool bit_nc; > > > > > > uint16_t ok_status = STATUS_OK; > > > > > > - s->cb_address = s->cu_base + s->cu_offset; > > > > > > + s->cb_address = s->cu_base + s->cu_offset; /* uint32_t overflow */ > > > > > > + assert (s->cb_address >= s->cu_base); > > > > > > > > > > We get these values from the guest; you can't just assert() on them. > > > > > You need to do something else. > > > > > > > > > > My reading of the 8255x data sheet is that there is nothing > > > > > in the hardware that forbids the guest from programming the > > > > > device such that the cu_base + cu_offset wraps around: > > > > > http://www.intel.com/content/dam/doc/manual/8255x-10-100-mbps-ethernet-controller-software-dev-manual.pdf > > > > > -- page 30 says that this is all doing 32-bit arithmetic > > > > > on addresses and doesn't say that there is any special case > > > > > handling by the device of overflow of that addition. > > > > > > > > > > Your commit message isn't very clear about what the failure > > > > > case is here, but I think the fix has to be something > > > > > different from this. > > > > > > > > Maybe the infinite loop mentioned in the commit message is actually a > > > > DMA recursion issue? I'm providing a reproducer for a DMA re-entracy > > > > issue below. With this patch applied, the reproducer triggers the > > > > assert(), rather than overflowing the stack, so maybe it is the same > > > > issue? > > > > -Alex > > > > > > > > cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m \ > > > > 512M -device i82559er,netdev=net0 -netdev user,id=net0 -nodefaults \ > > > > -qtest stdio > > > > outl 0xcf8 0x80001014 > > > > outl 0xcfc 0xc000 > > > > outl 0xcf8 0x80001010 > > > > outl 0xcfc 0xe0020000 > > > > outl 0xcf8 0x80001004 > > > > outw 0xcfc 0x7 > > > > write 0x1ffffc0b 0x1 0x55 > > > > write 0x1ffffc0c 0x1 0xfc > > > > write 0x1ffffc0d 0x1 0x46 > > > > write 0x1ffffc0e 0x1 0x07 > > > > write 0x746fc59 0x1 0x02 > > > > write 0x746fc5b 0x1 0x02 > > > > write 0x746fc5c 0x1 0xe0 > > > > write 0x4 0x1 0x07 > > > > write 0x5 0x1 0xfc > > > > write 0x6 0x1 0xff > > > > write 0x7 0x1 0x1f > > > > outw 0xc002 0x20 > > > > EOF > > > > > > > > Formatted for committing a regression-test: > > > > > > > > static void test_fuzz(void) > > > > { > > > > QTestState *s = > > > > qtest_init("-display none , -m 512M -device i82559er,netdev=net0 " > > > > "-netdev user,id=net0 -nodefaults"); > > > > qtest_outl(s, 0xcf8, 0x80001014); > > > > qtest_outl(s, 0xcfc, 0xc000); > > > > qtest_outl(s, 0xcf8, 0x80001010); > > > > qtest_outl(s, 0xcfc, 0xe0020000); > > > > qtest_outl(s, 0xcf8, 0x80001004); > > > > qtest_outw(s, 0xcfc, 0x7); > > > > qtest_bufwrite(s, 0x1ffffc0b, "\x55", 0x1); > > > > qtest_bufwrite(s, 0x1ffffc0c, "\xfc", 0x1); > > > > qtest_bufwrite(s, 0x1ffffc0d, "\x46", 0x1); > > > > qtest_bufwrite(s, 0x1ffffc0e, "\x07", 0x1); > > > > qtest_bufwrite(s, 0x746fc59, "\x02", 0x1); > > > > qtest_bufwrite(s, 0x746fc5b, "\x02", 0x1); > > > > qtest_bufwrite(s, 0x746fc5c, "\xe0", 0x1); > > > > qtest_bufwrite(s, 0x4, "\x07", 0x1); > > > > qtest_bufwrite(s, 0x5, "\xfc", 0x1); > > > > qtest_bufwrite(s, 0x6, "\xff", 0x1); > > > > qtest_bufwrite(s, 0x7, "\x1f", 0x1); > > > > qtest_outw(s, 0xc002, 0x20); > > > > qtest_quit(s); > > > > } > > > > > > > > > > > > > > > > > > thanks > > > > > -- PMM > > > > > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: eepro100: validate various address values 2021-02-19 1:54 ` Alexander Bulekov 2021-02-19 2:06 ` Li Qiang @ 2021-02-19 6:11 ` P J P 2021-02-19 8:08 ` Stefan Weil 1 sibling, 1 reply; 14+ messages in thread From: P J P @ 2021-02-19 6:11 UTC (permalink / raw) To: Alexander Bulekov Cc: Peter Maydell, Stefan Weil, Jason Wang, Li Qiang, QEMU Developers, Ruhr-University Bochum Hello Alex, Stefan, all +-- On Thu, 18 Feb 2021, Alexander Bulekov wrote --+ | Maybe the infinite loop mentioned in the commit message is actually a DMA | recursion issue? I'm providing a reproducer for a DMA re-entracy issue | below. With this patch applied, the reproducer triggers the assert(), rather | than overflowing the stack, so maybe it is the same issue? -Alex | | cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m \ | 512M -device i82559er,netdev=net0 -netdev user,id=net0 -nodefaults \ | -qtest stdio | outl 0xcf8 0x80001014 | outl 0xcfc 0xc000 | outl 0xcf8 0x80001010 | outl 0xcfc 0xe0020000 | outl 0xcf8 0x80001004 | outw 0xcfc 0x7 | write 0x1ffffc0b 0x1 0x55 | write 0x1ffffc0c 0x1 0xfc | write 0x1ffffc0d 0x1 0x46 | write 0x1ffffc0e 0x1 0x07 | write 0x746fc59 0x1 0x02 | write 0x746fc5b 0x1 0x02 | write 0x746fc5c 0x1 0xe0 | write 0x4 0x1 0x07 | write 0x5 0x1 0xfc | write 0x6 0x1 0xff | write 0x7 0x1 0x1f | outw 0xc002 0x20 | EOF | * Yes, it is an infinite recursion induced stack overflow. I should've said recursion instead of loop. Thank you for sharing a reproducer and the stack trace. +-- On Thu, 18 Feb 2021, Stefan Weil wrote --+ | Am 18.02.21 um 15:41 schrieb Peter Maydell: || + assert (s->cb_address >= s->cu_base); | > We get these values from the guest; you can't just assert() on them. You | > need to do something else. | > http://www.intel.com/content/dam/doc/manual/8255x-10-100-mbps-ethernet-controller-software-dev-manual.pdf | | I agree with Peter. The hardware emulation in QEMU should try to do the same | as the real hardware. * Agreed. * While the manual does not say how to handle uint32_t overflow in above 'cb_address' calculation, I doubt if overflow is expected. + if (s->cb_address < s->cu_base) { + logout ("invalid cb_address: %s: %u\n", __func__, s->cb_address); + break; + } I tried above fix first, it does not seem to arrest the recurssion induced stack overflow. Hence assert(3). * I also tried to find out if there's any cap on the 'cu_offset' value OR number of command units (cu) that can be addressed. But in linear addressing mode offset is a 32bit value with base address set to zero(0). And in 32bit segmented addressing mode 'offset' is 16bit value with non-zero(0) base address. ie. maximum offset could be about ~4K for 16byte command block IIUC. I'm not sure if segmented addressing mode is supported. * I'd appreciate if you could suggest a right way to fix it OR propose/post another patch. I'm okay either way. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: eepro100: validate various address values 2021-02-19 6:11 ` P J P @ 2021-02-19 8:08 ` Stefan Weil 2021-02-19 8:26 ` Stefan Weil 0 siblings, 1 reply; 14+ messages in thread From: Stefan Weil @ 2021-02-19 8:08 UTC (permalink / raw) To: P J P, Alexander Bulekov Cc: Peter Maydell, Jason Wang, Li Qiang, Ruhr-University Bochum, QEMU Developers Am 19.02.21 um 07:11 schrieb P J P: > Hello Alex, Stefan, all > > +-- On Thu, 18 Feb 2021, Alexander Bulekov wrote --+ > | Maybe the infinite loop mentioned in the commit message is actually a DMA > | recursion issue? I'm providing a reproducer for a DMA re-entracy issue > | below. With this patch applied, the reproducer triggers the assert(), rather > | than overflowing the stack, so maybe it is the same issue? -Alex > | > | cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m \ > | 512M -device i82559er,netdev=net0 -netdev user,id=net0 -nodefaults \ > | -qtest stdio > | outl 0xcf8 0x80001014 > | outl 0xcfc 0xc000 > | outl 0xcf8 0x80001010 > | outl 0xcfc 0xe0020000 > | outl 0xcf8 0x80001004 > | outw 0xcfc 0x7 > | write 0x1ffffc0b 0x1 0x55 > | write 0x1ffffc0c 0x1 0xfc > | write 0x1ffffc0d 0x1 0x46 > | write 0x1ffffc0e 0x1 0x07 > | write 0x746fc59 0x1 0x02 > | write 0x746fc5b 0x1 0x02 > | write 0x746fc5c 0x1 0xe0 > | write 0x4 0x1 0x07 > | write 0x5 0x1 0xfc > | write 0x6 0x1 0xff > | write 0x7 0x1 0x1f > | outw 0xc002 0x20 > | EOF > | > > * Yes, it is an infinite recursion induced stack overflow. I should've said > recursion instead of loop. > > Thank you for sharing a reproducer and the stack trace. Okay, I can confirm the infinite recursion now. The test case triggers memory writes by the hardware which cause new actions of the same hardware and so on. I don't know how the real hardware would handle that case. For QEMU we can extend the current code which tries to prevent endless loops: the device status EEPRO100State can be extended by a recursion counter to limit the number of recursions, or maybe a boolean flag could be used to stop any recursion of action_command(). I prefer the second variant (no recursion at all) and suggest to add a diagnostic message as well like it is done for the endless loop case. Stefan ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: eepro100: validate various address values 2021-02-19 8:08 ` Stefan Weil @ 2021-02-19 8:26 ` Stefan Weil 2021-02-19 9:26 ` P J P 0 siblings, 1 reply; 14+ messages in thread From: Stefan Weil @ 2021-02-19 8:26 UTC (permalink / raw) To: P J P, Alexander Bulekov Cc: Peter Maydell, Jason Wang, Li Qiang, Ruhr-University Bochum, QEMU Developers Am 19.02.21 um 09:08 schrieb Stefan Weil: > Okay, I can confirm the infinite recursion now. > > The test case triggers memory writes by the hardware which cause new > actions of the same hardware and so on. > > I don't know how the real hardware would handle that case. > > For QEMU we can extend the current code which tries to prevent endless > loops: the device status EEPRO100State can be extended by a recursion > counter to limit the number of recursions, or maybe a boolean flag > could be used to stop any recursion of action_command(). I prefer the > second variant (no recursion at all) and suggest to add a diagnostic > message as well like it is done for the endless loop case. If there are no recursions in normal use, the following patch should work: diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c index 16e95ef9cc..2474cf3dc2 100644 --- a/hw/net/eepro100.c +++ b/hw/net/eepro100.c @@ -279,6 +279,9 @@ typedef struct { /* Quasi static device properties (no need to save them). */ uint16_t stats_size; bool has_extended_tcb_support; + + /* Flag to avoid recursions. */ + bool busy; } EEPRO100State; /* Word indices in EEPROM. */ @@ -837,6 +840,14 @@ static void action_command(EEPRO100State *s) Therefore we limit the number of iterations. */ unsigned max_loop_count = 16; + if (s->busy) { + /* Prevent recursions. */ + logout("recursion in %s:%u\n", __FILE__, __LINE__); + return; + } + + s->busy = true; + for (;;) { bool bit_el; bool bit_s; @@ -933,6 +944,7 @@ static void action_command(EEPRO100State *s) } TRACE(OTHER, logout("CU list empty\n")); /* List is empty. Now CU is idle or suspended. */ + s->busy = false; } static void eepro100_cu_command(EEPRO100State * s, uint8_t val) ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] net: eepro100: validate various address values 2021-02-19 8:26 ` Stefan Weil @ 2021-02-19 9:26 ` P J P 2021-02-19 9:52 ` Stefan Weil 0 siblings, 1 reply; 14+ messages in thread From: P J P @ 2021-02-19 9:26 UTC (permalink / raw) To: Stefan Weil Cc: Peter Maydell, Jason Wang, Li Qiang, QEMU Developers, Alexander Bulekov, Ruhr-University Bochum [-- Attachment #1: Type: text/plain, Size: 1624 bytes --] Hello Stefan, +-- On Fri, 19 Feb 2021, Stefan Weil wrote --+ | If there are no recursions in normal use, the following patch should work: | | diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c | index 16e95ef9cc..2474cf3dc2 100644 | --- a/hw/net/eepro100.c | +++ b/hw/net/eepro100.c | @@ -279,6 +279,9 @@ typedef struct { | /* Quasi static device properties (no need to save them). */ | uint16_t stats_size; | bool has_extended_tcb_support; | + | + /* Flag to avoid recursions. */ | + bool busy; | } EEPRO100State; | | /* Word indices in EEPROM. */ | @@ -837,6 +840,14 @@ static void action_command(EEPRO100State *s) | Therefore we limit the number of iterations. */ | unsigned max_loop_count = 16; | | + if (s->busy) { | + /* Prevent recursions. */ | + logout("recursion in %s:%u\n", __FILE__, __LINE__); | + return; | + } | + | + s->busy = true; | + | for (;;) { | bool bit_el; | bool bit_s; | @@ -933,6 +944,7 @@ static void action_command(EEPRO100State *s) | } | TRACE(OTHER, logout("CU list empty\n")); | /* List is empty. Now CU is idle or suspended. */ | + s->busy = false; | } | | static void eepro100_cu_command(EEPRO100State * s, uint8_t val) Please see: -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Feepro100_stackoverflow1 * It does not seem to address above case. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: eepro100: validate various address values 2021-02-19 9:26 ` P J P @ 2021-02-19 9:52 ` Stefan Weil 0 siblings, 0 replies; 14+ messages in thread From: Stefan Weil @ 2021-02-19 9:52 UTC (permalink / raw) To: P J P Cc: Peter Maydell, Jason Wang, Li Qiang, QEMU Developers, Alexander Bulekov, Ruhr-University Bochum Am 19.02.21 um 10:26 schrieb P J P: > Hello Stefan, > > +-- On Fri, 19 Feb 2021, Stefan Weil wrote --+ > | If there are no recursions in normal use, the following patch should work: > | > | diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c > | index 16e95ef9cc..2474cf3dc2 100644 > | --- a/hw/net/eepro100.c > | +++ b/hw/net/eepro100.c > | @@ -279,6 +279,9 @@ typedef struct { > | /* Quasi static device properties (no need to save them). */ > | uint16_t stats_size; > | bool has_extended_tcb_support; > | + > | + /* Flag to avoid recursions. */ > | + bool busy; > | } EEPRO100State; > | > | /* Word indices in EEPROM. */ > | @@ -837,6 +840,14 @@ static void action_command(EEPRO100State *s) > | Therefore we limit the number of iterations. */ > | unsigned max_loop_count = 16; > | > | + if (s->busy) { > | + /* Prevent recursions. */ > | + logout("recursion in %s:%u\n", __FILE__, __LINE__); > | + return; > | + } > | + > | + s->busy = true; > | + > | for (;;) { > | bool bit_el; > | bool bit_s; > | @@ -933,6 +944,7 @@ static void action_command(EEPRO100State *s) > | } > | TRACE(OTHER, logout("CU list empty\n")); > | /* List is empty. Now CU is idle or suspended. */ > | + s->busy = false; > | } > | > | static void eepro100_cu_command(EEPRO100State * s, uint8_t val) > > Please see: > -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Feepro100_stackoverflow1 > > * It does not seem to address above case. My suggested patch fixes that test case: it no longer crashes because of an endless recursion. Stefan ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-02-20 3:07 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-18 14:06 [PATCH] net: eepro100: validate various address values P J P 2021-02-18 14:18 ` no-reply 2021-02-18 14:41 ` Peter Maydell 2021-02-18 16:10 ` Stefan Weil 2021-02-19 1:54 ` Alexander Bulekov 2021-02-19 2:06 ` Li Qiang 2021-02-19 2:14 ` Alexander Bulekov 2021-02-19 4:43 ` Li Qiang 2021-02-20 3:05 ` Alexander Bulekov 2021-02-19 6:11 ` P J P 2021-02-19 8:08 ` Stefan Weil 2021-02-19 8:26 ` Stefan Weil 2021-02-19 9:26 ` P J P 2021-02-19 9:52 ` Stefan Weil
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.