[PATCH v3 0/2] pegasus: correct buffer & packet sizes

[PATCH v3 0/2] pegasus: correct buffer & packet sizes

[Petko Manolov]

As noticed by Lincoln Ramsay <a1291762@gmail.com> some old (usb 1.1) Pegasus
based devices may actually return more bytes than the specified in the datasheet
amount.  That would not be a problem if the allocated space for the SKB was
equal to the parameter passed to usb_fill_bulk_urb().  Some poor bugger (i
really hope it was not me, but 'git blame' is useless in this case, so anyway)
decided to add '+ 8' to the buffer length parameter.  Sometimes the usb transfer
overflows and corrupts the socket structure, leading to kernel panic.

The above doesn't seem to happen for newer (Pegasus2 based) devices which did
help this bug to hide for so long.

The new default is to not include the CRC at the end of each received package.  
So far CRC has been ignored which makes no sense to do it in a first place.

The patch is against v4.6-rc5 and was tested on ADM8515 device by transferring
multiple gigabytes of data over a couple of days without any complaints from the
kernel.  Please apply it to whatever net tree you deem fit.

Changes since v1:

 - split the patch in two parts;
 - corrected the subject lines;

Changes since v2:

 - do not append CRC by default (based on a discussion with Johannes Berg);

Petko Manolov (2):
  pegasus: fixes URB buffer allocation size;
  pegasus: fixes reported packet length

 drivers/net/usb/pegasus.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.8.0.rc3

[PATCH v3 1/2] pegasus: fixes URB buffer allocation size;

[Petko Manolov]

usb_fill_bulk_urb() receives buffer length parameter 8 bytes larger
than what's allocated by alloc_skb(); This seems to be a problem with
older (pegasus usb-1.1) devices, which may silently return more data
than the maximal packet length.

Reported-by: Lincoln Ramsay <a1291762@gmail.com>
Signed-off-by: Petko Manolov <petkan@mip-labs.com>
---
 drivers/net/usb/pegasus.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index f840802..f919e20 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -528,7 +528,7 @@ static void read_bulk_callback(struct urb *urb)
 goon:
 	usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
 			  usb_rcvbulkpipe(pegasus->usb, 1),
-			  pegasus->rx_skb->data, PEGASUS_MTU + 8,
+			  pegasus->rx_skb->data, PEGASUS_MTU,
 			  read_bulk_callback, pegasus);
 	rx_status = usb_submit_urb(pegasus->rx_urb, GFP_ATOMIC);
 	if (rx_status == -ENODEV)
@@ -569,7 +569,7 @@ static void rx_fixup(unsigned long data)
 	}
 	usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
 			  usb_rcvbulkpipe(pegasus->usb, 1),
-			  pegasus->rx_skb->data, PEGASUS_MTU + 8,
+			  pegasus->rx_skb->data, PEGASUS_MTU,
 			  read_bulk_callback, pegasus);
 try_again:
 	status = usb_submit_urb(pegasus->rx_urb, GFP_ATOMIC);
@@ -823,7 +823,7 @@ static int pegasus_open(struct net_device *net)
 
 	usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
 			  usb_rcvbulkpipe(pegasus->usb, 1),
-			  pegasus->rx_skb->data, PEGASUS_MTU + 8,
+			  pegasus->rx_skb->data, PEGASUS_MTU,
 			  read_bulk_callback, pegasus);
 	if ((res = usb_submit_urb(pegasus->rx_urb, GFP_KERNEL))) {
 		if (res == -ENODEV)
-- 
2.8.0.rc3

[PATCH v3 2/2] pegasus: fixes reported packet length

[Petko Manolov]

The default Pegasus setup was to append the status and CRC at the end of each
received packet.  The status bits are used to update various stats, but CRC has
been ignored.  The new default is to not append CRC at the end of RX packets.

Signed-off-by: Petko Manolov <petkan@mip-labs.com>
---
 drivers/net/usb/pegasus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index f919e20..82129ee 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -411,7 +411,7 @@ static int enable_net_traffic(struct net_device *dev, struct usb_device *usb)
 	int ret;
 
 	read_mii_word(pegasus, pegasus->phy, MII_LPA, &linkpart);
-	data[0] = 0xc9;
+	data[0] = 0xc8; /* TX & RX enable, append status, no CRC */
 	data[1] = 0;
 	if (linkpart & (ADVERTISE_100FULL | ADVERTISE_10FULL))
 		data[1] |= 0x20;	/* set full duplex */
@@ -497,7 +497,7 @@ static void read_bulk_callback(struct urb *urb)
 		pkt_len = buf[count - 3] << 8;
 		pkt_len += buf[count - 4];
 		pkt_len &= 0xfff;
-		pkt_len -= 8;
+		pkt_len -= 4;
 	}
 
 	/*
-- 
2.8.0.rc3

Re: [PATCH v3 0/2] pegasus: correct buffer & packet sizes

[David Miller]

From: Petko Manolov <petkan@mip-labs.com>
Date: Wed, 27 Apr 2016 14:24:48 +0300

> As noticed by Lincoln Ramsay <a1291762@gmail.com> some old (usb 1.1) Pegasus
> based devices may actually return more bytes than the specified in the datasheet
> amount.  That would not be a problem if the allocated space for the SKB was
> equal to the parameter passed to usb_fill_bulk_urb().  Some poor bugger (i
> really hope it was not me, but 'git blame' is useless in this case, so anyway)
> decided to add '+ 8' to the buffer length parameter.  Sometimes the usb transfer
> overflows and corrupts the socket structure, leading to kernel panic.
> 
> The above doesn't seem to happen for newer (Pegasus2 based) devices which did
> help this bug to hide for so long.
> 
> The new default is to not include the CRC at the end of each received package.  
> So far CRC has been ignored which makes no sense to do it in a first place.
> 
> The patch is against v4.6-rc5 and was tested on ADM8515 device by transferring
> multiple gigabytes of data over a couple of days without any complaints from the
> kernel.  Please apply it to whatever net tree you deem fit.
> 
> Changes since v1:
> 
>  - split the patch in two parts;
>  - corrected the subject lines;
> 
> Changes since v2:
> 
>  - do not append CRC by default (based on a discussion with Johannes Berg);

Series applied, thanks.