NET: Fix possible corruption in bpqether driver
authorRalf Baechle <ralf@linux-mips.org>
Thu, 3 Sep 2009 06:09:29 +0000 (23:09 -0700)
committerDavid S. Miller <davem@davemloft.net>
Thu, 3 Sep 2009 06:45:58 +0000 (23:45 -0700)
The bpq ether driver is modifying the data art of the skb by first
dropping the KISS byte (a command byte for the radio) then prepending the
length + 4 of the remaining AX.25 packet to be transmitted as a little
endian 16-bit number.  If the high byte of the length has a different
value than the dropped KISS byte users of clones of the skb may observe
this as corruption.  This was observed with by running listen(8) -a which
uses a packet socket which clones transmit packets.  The corruption will
then typically be displayed for as a KISS "TX Delay" command for AX.25
packets in the range of 252..508 bytes or any other KISS command for
yet larger packets.

Fixed by using skb_cow to create a private copy should the skb be cloned.
Using skb_cow also allows us to cleanup the old logic to ensure sufficient
headroom in the skb.

While at it, replace a return of 0 from bpq_xmit with the proper constant
NETDEV_TX_OK which is now being used everywhere else in this function.

Affected: all 2.2, 2.4 and 2.6 kernels.

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
Reported-by: Jann Traschewski <jann@gmx.de>
Signed-off-by: David S. Miller <davem@davemloft.net>
drivers/net/hamradio/bpqether.c

index 3c7cc7f458009365b6363a2f925164945501b1bc..fe893c91a01b904e24cd6027799a461fec8985bc 100644 (file)
@@ -249,7 +249,6 @@ drop:
  */
 static netdev_tx_t bpq_xmit(struct sk_buff *skb, struct net_device *dev)
 {
-       struct sk_buff *newskb;
        unsigned char *ptr;
        struct bpqdev *bpq;
        int size;
@@ -263,28 +262,23 @@ static netdev_tx_t bpq_xmit(struct sk_buff *skb, struct net_device *dev)
                return NETDEV_TX_OK;
        }
 
-       skb_pull(skb, 1);
+       skb_pull(skb, 1);                       /* Drop KISS byte */
        size = skb->len;
 
        /*
-        * The AX.25 code leaves enough room for the ethernet header, but
-        * sendto() does not.
+        * We're about to mess with the skb which may still shared with the
+        * generic networking code so unshare and ensure it's got enough
+        * space for the BPQ headers.
         */
-       if (skb_headroom(skb) < AX25_BPQ_HEADER_LEN) {  /* Ough! */
-               if ((newskb = skb_realloc_headroom(skb, AX25_BPQ_HEADER_LEN)) == NULL) {
-                       printk(KERN_WARNING "bpqether: out of memory\n");
-                       kfree_skb(skb);
-                       return NETDEV_TX_OK;
-               }
-
-               if (skb->sk != NULL)
-                       skb_set_owner_w(newskb, skb->sk);
-
+       if (skb_cow(skb, AX25_BPQ_HEADER_LEN)) {
+               if (net_ratelimit())
+                       pr_err("bpqether: out of memory\n");
                kfree_skb(skb);
-               skb = newskb;
+
+               return NETDEV_TX_OK;
        }
 
-       ptr = skb_push(skb, 2);
+       ptr = skb_push(skb, 2);                 /* Make space for length */
 
        *ptr++ = (size + 5) % 256;
        *ptr++ = (size + 5) / 256;