netdev - RE: [RFC PATCH] net: usb: ax88179_178a: fix packet alignment padding

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 12 Jun 2020 12:10:47 +0800
From:   <louis@...x.com.tw>
To:     "'ASIX_Allan [Office]'" <allan@...x.com.tw>,
        "'Jeremy Kerr'" <jk@...abs.org>,
        "'Freddy Xin'" <freddy@...x.com.tw>
Cc:     "'Peter Fink'" <pfink@...ist-es.de>, <netdev@...r.kernel.org>,
        <linux-usb@...r.kernel.org>
Subject: RE: [RFC PATCH] net: usb: ax88179_178a: fix packet alignment padding

Hi Jeremy,

Thanks for the correction.
Indeed, the hardware adds two bytes dummy data at beginning of Ethernet packet to make IP header aligned. 
The original patch made by Freddy contains the length of dummy header. 
Thanks for your concerning.
Please let us know if you have any other question.

Thank you,
Louis.

---
Best regards,
Allan Chou
ASIX Electronics Corporation
TEL: 886-3-5799500 ext.228
FAX: 886-3-5799558
E-mail: allan@...x.com.tw 
https://www.asix.com.tw/ 



-----Original Message-----
From: Jeremy Kerr <jk@...abs.org> 
Sent: Tuesday, June 2, 2020 11:18 AM
To: Freddy Xin <freddy@...x.com.tw>; Allan Chou <allan@...x.com.tw>
Cc: Peter Fink <pfink@...ist-es.de>; netdev@...r.kernel.org; linux-usb@...r.kernel.org
Subject: Re: [RFC PATCH] net: usb: ax88179_178a: fix packet alignment padding

Hi Freddy and Allan,

Just following up on the RFC patch below: Can you confirm whether the packet len (in the hardware-provided packet RX metadata) includes the two-byte padding field? Is this the same for all ax88179 devices?

Cheers,


Jeremy

> Using a AX88179 device (0b95:1790), I see two bytes of appended data 
> on every RX packet. For example, this 48-byte ping, using 0xff as a 
> payload byte:
> 
>   04:20:22.528472 IP 192.168.1.1 > 192.168.1.2: ICMP echo request, id 2447, seq 1, length 64
> 	0x0000:  000a cd35 ea50 000a cd35 ea4f 0800 4500
> 	0x0010:  0054 c116 4000 4001 f63e c0a8 0101 c0a8
> 	0x0020:  0102 0800 b633 098f 0001 87ea cd5e 0000
> 	0x0030:  0000 dcf2 0600 0000 0000 ffff ffff ffff
> 	0x0040:  ffff ffff ffff ffff ffff ffff ffff ffff
> 	0x0050:  ffff ffff ffff ffff ffff ffff ffff ffff
> 	0x0060:  ffff 961f
> 
> Those last two bytes - 96 1f - aren't part of the original packet.
> 
> In the ax88179 RX path, the usbnet rx_fixup function trims a 2-byte 
> 'alignment pseudo header' from the start of the packet, and sets the 
> length from a per-packet field populated by hardware. It looks like 
> that length field *includes* the 2-byte header; the current driver 
> assumes that it's excluded.
> 
> This change trims the 2-byte alignment header after we've set the 
> packet length, so the resulting packet length is correct. While we're 
> moving the comment around, this also fixes the spelling of 'pseudo'.
> 
> Signed-off-by: Jeremy Kerr <jk@...abs.org>
> 
> ---
> RFC: I don't have access to docs for this hardware, so this is all 
> based on observed behaviour of the reported packet length.
> ---
>  drivers/net/usb/ax88179_178a.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/usb/ax88179_178a.c 
> b/drivers/net/usb/ax88179_178a.c index 93044cf1417a..1fe4cc28d154 
> 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -1414,10 +1414,10 @@ static int ax88179_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>  		}
>  
>  		if (pkt_cnt == 0) {
> -			/* Skip IP alignment psudo header */
> -			skb_pull(skb, 2);
>  			skb->len = pkt_len;
> -			skb_set_tail_pointer(skb, pkt_len);
> +			/* Skip IP alignment pseudo header */
> +			skb_pull(skb, 2);
> +			skb_set_tail_pointer(skb, skb->len);
>  			skb->truesize = pkt_len + sizeof(struct sk_buff);
>  			ax88179_rx_checksum(skb, pkt_hdr);
>  			return 1;
> @@ -1426,8 +1426,9 @@ static int ax88179_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>  		ax_skb = skb_clone(skb, GFP_ATOMIC);
>  		if (ax_skb) {
>  			ax_skb->len = pkt_len;
> -			ax_skb->data = skb->data + 2;
> -			skb_set_tail_pointer(ax_skb, pkt_len);
> +			/* Skip IP alignment pseudo header */
> +			skb_pull(ax_skb, 2);
> +			skb_set_tail_pointer(ax_skb, ax_skb->len);
>  			ax_skb->truesize = pkt_len + sizeof(struct sk_buff);
>  			ax88179_rx_checksum(ax_skb, pkt_hdr);
>  			usbnet_skb_return(dev, ax_skb);
>