| Message ID | 20210218140629.373646-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: eepro100: validate various address values | expand |
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
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
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
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 >
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 > > >
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 > > > > >
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 > > > > > > >
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
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
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)
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
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
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 > > > > > > > > >
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);