Alexander Bulekov <alx...@bu.edu> 于2021年2月19日周五 上午9:56写道: > > On 210218 1441, Peter Maydell wrote: > > On Thu, 18 Feb 2021 at 14:13, P J P <ppan...@redhat.com> wrote: > > > > > > From: Prasad J Pandit <p...@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-sys...@rub.de> > > > Signed-off-by: Prasad J Pandit <p...@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 > > >