[PATCH v2 0/2] pegasus: correct buffer sizes

[PATCH v2 0/2] pegasus: correct buffer 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.

Nearly all Pegasus devices may append the RX status to the end of the received 
packet.  It is the default setup for the driver.  The actual ethernet packet is 
4 bytes shorter.  Why and when 'pkt_len -= 4' became 'pkt_len -= 8' is again 
hidden in the mists of time.  There might have been a good reason to do so, but 
multiple reads of the datasheet did not point me to any.

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.

Changes since v1:

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

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

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

-- 
2.8.0.rc3

[PATCH v2 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 v2 2/2] pegasus: fixes reported packet length

[Petko Manolov]

According to ADM851x docs the ethernet packet is appended with 4
bytes, not 8.  Fixing this to report (hopefully) the right amount
of data.

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

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index f919e20..780c217 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -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 v2 0/2] pegasus: correct buffer sizes

[Johannes Berg]

On Wed, 2016-04-27 at 10:35 +0300, Petko Manolov wrote:
> 
> Nearly all Pegasus devices may append the RX status to the end of the
> received  packet.  It is the default setup for the driver.  The
> actual ethernet packet is  4 bytes shorter.  Why and when 'pkt_len -=
> 4' became 'pkt_len -= 8' is again hidden in the mists of time.  There
> might have been a good reason to do so, but multiple reads of the
> datasheet did not point me to any.
> 

Wild guess: (some) devices pass the FCS, perhaps depending on the
configuration?

johannes

Re: [PATCH v2 0/2] pegasus: correct buffer sizes

[Johannes Berg]

On Wed, 2016-04-27 at 12:33 +0300, Petko Manolov wrote:
> 
> Your guess turned out to be not so wild.  ;)  All Pegasus devices are
> configured  (by the driver) to append CRC at the end of each RX
> packet.  However, the driver reports packet length that does not
> include it.  

Interesting, then my guess was wrong though, since the length is
reported without it, or am I misunderstanding this?

> I doubt the appended CRC is being silently verified by the upper
> layer, bit i might be wrong of course.

It's even "outside" the skb as you describe it, so it can't even be
touched, no?

> Perhaps it is best if instruct the device to not include the CRC as
> it seems ignored anyway.

Yeah, there's no point in passing it over the bus.

johannes

RE: [PATCH v2 0/2] pegasus: correct buffer sizes

[David Laight]

From: Johannes Berg
> Sent: 27 April 2016 10:44
> On Wed, 2016-04-27 at 12:33 +0300, Petko Manolov wrote:
> >
> > Your guess turned out to be not so wild.;)All Pegasus devices are
> > configured (by the driver) to append CRC at the end of each RX
> > packet.However, the driver reports packet length that does not
> > include it.
> 
> Interesting, then my guess was wrong though, since the length is
> reported without it, or am I misunderstanding this?
...

It is even possible that the crc is written into the rx buffer even
though the length the hardware reports excludes it.

	David

Re: [PATCH v2 0/2] pegasus: correct buffer sizes

[Johannes Berg]

On Wed, 2016-04-27 at 16:26 +0000, David Laight wrote:
> From: Johannes Berg
> > 
> > Sent: 27 April 2016 10:44
> > On Wed, 2016-04-27 at 12:33 +0300, Petko Manolov wrote:
> > > 
> > > 
> > > Your guess turned out to be not so wild.;)All Pegasus devices are
> > > configured (by the driver) to append CRC at the end of each RX
> > > packet.However, the driver reports packet length that does not
> > > include it.
> > Interesting, then my guess was wrong though, since the length is
> > reported without it, or am I misunderstanding this?
> ...
> 
> It is even possible that the crc is written into the rx buffer even
> though the length the hardware reports excludes it.
> 

Right. I think a proper test would be to construct some kind of
ethernet-only frames, and check that they come out properly on the
other side. IP always has its own length field, so may be happy with
trailing garbage reported to the network stack, but doing raw ethernet
will let you check that the frame length is correct.

johannes