Re: [PATCH] net: eepro100: validate various address values

[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.


+-- 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