kern/47788: Incorrect packet length transmitted for odd-lengths with smc91cxx driver

[webpages%sprow.co.uk@localhost]

>Number:         47788
>Category:       kern
>Synopsis:       Incorrect packet length transmitted for odd-lengths with 
>smc91cxx driver
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Apr 29 21:55:01 +0000 2013
>Originator:     Robert Sprowson
>Release:        N/A
>Organization:
N/A
>Environment:
N/A
>Description:
The padding logic in src/sys/dev/ic/smc91cxx.c is incorrectly padding by being 
confused about padding to get to ETHER_MIN_LEN and padding needed because the 
SMSC91Cxx family needs odd length packets handling specially.

By example, the following "ping -s" commands yield the following packet sizes 
when inspected with WireShark

  1400 -> 1400 (correct)
  1401 -> 1402 (1 byte too long)
  1402 -> 1402 (correct)
  1403 -> 1405 (2 bytes too long)

this pattern repeats every 4 bytes.

The patch below simplifies the copying routine to always stop at a word 
boundary, regardless of the state of SMC91CXX_NO_BYTE_WRITE, then builds any 
padding needed in variables which are then written using only word accesses 
(hence are compatible with both byte-write capable and non-byte-write capable 
targets).

Tested by inspection in WireShark with "ping -s" of sizes 8; 9; 10; 11 (to 
check the padding of < ETHER_MIN_LEN) and 1400 to 1407 inclusive.

Please note the patch is a diff assuming the patch in PR/47765 is applied first.
>How-To-Repeat:
ping -s 1401 my.host.address
ping -s 1403 my.host.address

or any other odd number.
>Fix:
--- smc91cxx_new.c      Thu Apr 25 06:49:32 2013
+++ smc91cxx_len.c      Mon Apr 29 21:30:58 2013
@@ -652,13 +652,12 @@
         */
        for (len = 0; m != NULL; m = m->m_next)
                len += m->m_len;
-       pad = (len & 1);
 
        /*
         * We drop packets that are too large.  Perhaps we should
         * truncate them instead?
         */
-       if ((len + pad) > (ETHER_MAX_LEN - ETHER_CRC_LEN)) {
+       if (len > (ETHER_MAX_LEN - ETHER_CRC_LEN)) {
                printf("%s: large packet discarded\n", 
device_xname(sc->sc_dev));
                ifp->if_oerrors++;
                IFQ_DEQUEUE(&ifp->if_snd, m);
@@ -666,6 +665,7 @@
                goto readcheck;
        }
 
+       pad = 0;
 #ifdef SMC91CXX_SW_PAD
        /*
         * Not using hardware padding; pad to ETHER_MIN_LEN.
@@ -745,24 +745,25 @@
        IFQ_DEQUEUE(&ifp->if_snd, m);
 
        /*
-        * Push the packet out to the card.
+        * Push the packet out to the card.  The copying function only does 
whole
+        * words and returns the straggling byte (if any).
         */
        oddbyte = smc91cxx_copy_tx_frame(sc, m);
 
 #ifdef SMC91CXX_SW_PAD
-#ifdef SMC91CXX_NO_BYTE_WRITE
 #if BYTE_ORDER == LITTLE_ENDIAN
        if (pad > 1 && (pad & 1)) {
                bus_space_write_2(bst, bsh, DATA_REG_W, oddbyte << 0);
                oddbyte = 0;
+               pad -= 1;
        }
 #else
        if (pad > 1 && (pad & 1)) {
                bus_space_write_2(bst, bsh, DATA_REG_W, oddbyte << 8);
                oddbyte = 0;
+               pad -= 1;
        }
 #endif
-#endif
 
        /*
         * Push out padding.
@@ -773,22 +774,17 @@
        }
 #endif
 
-#ifdef SMC91CXX_NO_BYTE_WRITE
        /*
         * Push out control byte and unused packet byte.  The control byte
-        * is 0, meaning the packet is even lengthed and no special
+        * denotes whether this is an odd or even length packet, and that no 
special
         * CRC handling is necessary.
         */
 #if BYTE_ORDER == LITTLE_ENDIAN
        bus_space_write_2(bst, bsh, DATA_REG_W,
-           oddbyte | (pad ? (CTLB_ODD << 8) : 0));
+           oddbyte | ((length & 1) ? (CTLB_ODD << 8) : 0));
 #else
        bus_space_write_2(bst, bsh, DATA_REG_W,
-           (oddbyte << 8) | (pad ? CTLB_ODD : 0));
-#endif
-#else
-       if (pad)
-               bus_space_write_1(bst, bsh, DATA_REG_B, 0);
+           (oddbyte << 8) | ((length & 1) ? CTLB_ODD : 0));
 #endif
 
        /*
@@ -892,10 +888,7 @@
                        panic("smc91cxx_copy_tx_frame: p != lim");
 #endif
        }
-#ifndef SMC91CXX_NO_BYTE_WRITE
-       if (leftover)
-               bus_space_write_1(bst, bsh, DATA_REG_B, dbuf);
-#endif
+
        return dbuf;
 }