Re: [Qemu-devel] [PATCH] net: ne2000: check ring buffer control registers

[P J P <ppandit@redhat.com>]

   Hello Jason,

+-- On Tue, 23 Feb 2016, Jason Wang wrote --+
| I mean with your patch, driver will only be allowed to set EN0_STOPPG
| before EN0_STARTPG. So if a driver want to set STARTPG first, the check
| 
| +            if (v < NE2000_PMEM_END && v < s->stop) {
| 
| will prevent the driver from working correctly since s->stop is zero here.

  Before drivers could start using NIC, it'll be initialised from its ROM, 
right? Which would set the PSTART & PSTOP registers to the default values. 
With '-net nic,model=ne2k_pci,vlan=0' I see,

    s->start = 19456, s->stop = 32768

| >   I think any attempts to define the ring buffer limits should reset 
| > 'boundary' and 'curpag' registers to s->start(STARTPG). I wonder if a 
| > driver should be allowed to fiddle with the ring buffers location inside 
| > contorller's memory. It does not seem right.
| 
| Well, I think we could not assume the behavior of a driver, especially
| consider it may be malicious.

  Yes; That's why it'll help to keep drivers from fiddling with the ring 
buffer dimensions. IIUC, there is an upper limit to where PSTOP could 
point[1],

  "In 8 bit mode the PSTOP register should not exceed to 0x60,
    in 16 bit mode the PSTOP register should not exceed to 0x80"

[1] http://www.ethernut.de/pdf/8019asds.pdf

Kernel drivers too seem to have it fixed
  -> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/8390/ne.c#n398
  -> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/8390/ne2k-pci.c#n342

| >   Check if (s->start == s->stop) at each receive call?
| Or in ne2000_buffer_full()?

  ne2000_buffer_full() too assumes that 's->stop > s->start'

    ...
    avail = (s->stop - s->start) - (index - boundary);

Is there a case wherein drivers need to adjust ring buffer pointers? If not, I 
think it's better to convert EN0_STARTPG:, EN0_STOPPG:, EN0_BOUNDARY: and 
EN1_CURPAG: cases into no-ops.

--
 - P J P
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F