| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 4 May 2009 17:51:37 +0200
From: Omar Laazimani <omar.oberthur@...il.com>
To: David Brownell <david-b@...bell.net>
Cc: David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH] : CDC EEM driver patch to be applied to 2.6.30 kernel
Thanks for your quick feedbacks.
I have tested your patch with our device and it's working well.
I have also added the TX side support for ZLP (see patch herein).
Please note that I can't test this issue as our device doesn't support it yet.
By the way, Just for curiosity, I have two questions about your patch
(see bellow) :
Thanks,
Omar
> /*
> - * EEM command packet:
> + * EEM (link) command packet:
> * b0..10: bmEEMCmdParam
> * b11..13: bmEEMCmd
> - * b14: bmReserved (0 by default)
> - * b15: 1
> + * b14: bmReserved (must be 0)
> + * b15: 1 (EEM command)
> */
> + if (header & BIT(14)) {
> + devdbg(dev, "reserved command %04x\n", header);
> + continue;
> + }
>
> - skb2 = skb_clone(skb, GFP_ATOMIC);
> - if (unlikely(!skb2))
> - goto Continue;
> + bmEEMCmd = (header >> 11) & 0x7;
> + switch (bmEEMCmd) {
>
> - bmEEMCmd = (BIT(11) & header)
> - | (BIT(12) & header)
> - | (BIT(13) & header);
> + /* Responding to echo requests is mandatory. */
> + case 0: /* Echo command */
> + len = header & 0x7FF;
> +
> + /* bogus command? */
> + if (skb->len < len)
> + return 0;
> +
> + skb2 = skb_clone(skb, GFP_ATOMIC);
> + if (unlikely(!skb2))
> + goto next;
> + skb_trim(skb2, len);
> + put_unaligned_le16(BIT(15) | (1 << 11) | len,
> + skb_push(skb2, 2));
why did you use 1 << 11 instead of BIT(11) ?
> + eem_linkcmd(dev, skb2);
> + break;
>
> + /*
> + * The bmCRC helps to denote when the CRC field in
> + * the Ethernet frame contains a calculated CRC:
> * bmCRC = 1 : CRC is calculated
> * bmCRC = 0 : CRC = 0xDEADBEEF
> */
> - if (header & BIT(14)) {
> - u32 crc2;
> - crc2 = crc32_le(~0, skb2->data, len);
> - crc2 = ~crc2;
> - if (unlikely(crc != crc2)) {
> - dev->stats.rx_errors++;
> - goto Continue;
> - }
> - } else {
> - if (unlikely(crc != 0xdeadbeef)) {
> - dev->stats.rx_errors++;
> - goto Continue;
> - }
> - }
> + if (header & BIT(14))
> + crc2 = ~crc32_le(~0, skb2->data, len);
> + else
> + crc2 = 0xdeadbeef;
>
> - usbnet_skb_return(dev, skb2);
> + if (is_last)
> + return crc == crc2;
Why do you prefer returning 0 and not incrementing
"dev->stats.rx_errors" instead of returning 1 (in all the cases) and
incrementing "dev->stats.rx_errors" in the error cases?
> +
> + if (unlikely(crc != crc2)) {
> + dev->stats.rx_errors++;
> + } else
> + usbnet_skb_return(dev, skb2);
============== CUT HERE
fixed :
- Zero length EEM packet support:
* Handle on TX side
--- cdc_eem.c 2009-05-04 16:59:43.000000000 +0200
+++ cdc_eem_v5.c 2009-05-04 17:07:20.000000000 +0200
@@ -31,7 +31,6 @@
#include <linux/usb/cdc.h>
#include <linux/usb/usbnet.h>
-
/*
* This driver is an implementation of the CDC "Ethernet Emulation
* Model" (EEM) specification, which encapsulates Ethernet frames
@@ -122,11 +121,14 @@
struct sk_buff *skb2 = NULL;
u16 len = skb->len;
u32 crc = 0;
+ int padlen = 0;
- /* FIXME when ((len + EEM_HEAD + ETH_FCS_LEN) % dev->maxpacket)
+ /* When ((len + EEM_HEAD + ETH_FCS_LEN) % dev->maxpacket)
* is zero, stick two bytes of zero length EEM packet on the end
* (so the framework won't add invalid single byte padding).
*/
+ if (!((len + EEM_HEAD + ETH_FCS_LEN) % dev->maxpacket))
+ padlen += 2;
if (!skb_cloned(skb)) {
int headroom = skb_headroom(skb);
@@ -145,7 +147,7 @@
}
}
- skb2 = skb_copy_expand(skb, EEM_HEAD, ETH_FCS_LEN, flags);
+ skb2 = skb_copy_expand(skb, EEM_HEAD, ETH_FCS_LEN + padlen, flags);
if (!skb2)
return NULL;
@@ -167,6 +169,10 @@
len = skb->len;
put_unaligned_le16(BIT(14) | len, skb_push(skb, 2));
+ /* Add zero length EEM packet if needed */
+ if (padlen)
+ *skb_put(skb, 2) = (u16) 0;
+
return skb;
}
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists