| 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: Tue, 28 Jun 2022 10:53:49 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Jose Alonso <joalonsof@...il.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH] net: usb: ax88179_178a: corrects packet receiving
On Thu, 2022-06-23 at 11:06 -0300, Jose Alonso wrote:
> This patch corrects the packet receiving in ax88179_rx_fixup.
>
> - problem observed:
> ifconfig shows always a lot of 'RX Errors' while packets
> are received normally.
>
> This occurs because ax88179_rx_fixup does not recognize properly
> the usb urb received.
> The packets are normally processed and at the end, the code exits
> with 'return 0', generating RX Errors.
> (with pkt_cnt==-2 and ptk_hdr over the field rx_hdr trying to identify
> another packet there)
>
> This is a usb urb received by "tcpdump -i usbmon2 -X":
> 0x0000: eeee f8e3 3b19 87a0 94de 80e3 daac 0800
> ^ packet 1 start (pkt_len = 0x05ec)
> ^^^^ IP alignment pseudo header
> ^ ethernet frame start
> last byte of packet vv
> padding (8-bytes aligned) vvvv vvvv
> 0x05e0: c92d d444 1420 8a69 83dd 272f e82b 9811
> 0x05f0: eeee f8e3 3b19 87a0 94de 80e3 daac 0800
> ... ^ packet 2
> 0x0be0: eeee f8e3 3b19 87a0 94de 80e3 daac 0800
> ...
> 0x1130: 9d41 9171 8a38 0ec5 eeee f8e3 3b19 87a0
> ...
> 0x1720: 8cfc 15ff 5e4c e85c eeee f8e3 3b19 87a0
> ...
> 0x1d10: ecfa 2a3a 19ab c78c eeee f8e3 3b19 87a0
> ...
> 0x2070: eeee f8e3 3b19 87a0 94de 80e3 daac 0800
> ... ^ packet 7
> 0x2120: 7c88 4ca5 5c57 7dcc 0d34 7577 f778 7e0a
> 0x2130: f032 e093 7489 0740 3008 ec05 0000 0080
> ====1==== ====2====
> hdr_off ^
> pkt_len = 0x05ec ^^^^
> AX_RXHDR_*=0x00830 ^^^^ ^
> pkt_len = 0 ^^^^
> AX_RXHDR_DROP_ERR=0x80000000 ^^^^ ^
> 0x2140: 3008 ec05 0000 0080 3008 5805 0000 0080
> 0x2150: 3008 ec05 0000 0080 3008 ec05 0000 0080
> 0x2160: 3008 5803 0000 0080 3008 c800 0000 0080
> ===11==== ===12==== ===13==== ===14====
> 0x2170: 0000 0000 0e00 3821
> ^^^^ ^^^^ rx_hdr
> ^^^^ pkt_cnt=14
> ^^^^ hdr_off=0x2138
> ^^^^ ^^^^ padding
>
> The dump shows that pkt_cnt is the number of entries in the
> per-packet metadata. It is "2 * packet count".
> Each packet have two entries. The first have a valid
> value (pkt_len and AX_RXHDR_*) and the second have a
> dummy-header 0x80000000 (pkt_len=0 with AX_RXHDR_DROP_ERR).
> Why exists dummy-header for each packet?!?
> My guess is that this was done probably to align the
> entry for each packet to 64-bits and maintain compatibility
> with old firmware.
> There is also a padding (0x00000000) before the rx_hdr to
> align the end of rx_hdr to 64-bit.
> Note that packets have a alignment of 64-bits (8-bytes).
>
> This patch assumes that the dummy-header and the last
> padding are optional. So it preserves semantics and
> recognizes the same valid packets as the current code.
>
> This patch was made using only the dump file information and
> tested with only one device:
> 0b95:1790 ASIX Electronics Corp. AX88179 Gigabit Ethernet
>
> Signed-off-by: Jose Alonso <joalonsof@...il.com>
>
> ---
>
> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index 4704ed6f00ef..02bd113c5045 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -1472,6 +1472,42 @@ static int ax88179_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
> * are bundled into this buffer and where we can find an array of
> * per-packet metadata (which contains elements encoded into u16).
> */
> +
> + /* SKB contents for current firmware:
> + * <packet 1> <padding>
> + * ...
> + * <packet N> <padding>
> + * <per-packet metadata entry 1> <dummy header>
> + * ...
> + * <per-packet metadata entry N> <dummy header>
> + * <padding2> <rx_hdr>
> + *
> + * where:
> + * <packet N> contains pkt_len bytes:
> + * 2 bytes of IP alignment pseudo header
> + * packet received
> + * <per-packet metadata entry N> contains 4 bytes:
> + * pkt_len and fields AX_RXHDR_*
> + * <padding> 0-7 bytes to terminate at
> + * 8 bytes boundary (64-bit).
> + * <padding2> 4 bytes to make rx_hdr terminate at
> + * 8 bytes boundary (64-bit)
> + * <dummy-header> contains 4 bytes:
> + * pkt_len=0 and AX_RXHDR_DROP_ERR
> + * <rx-hdr> contains 4 bytes:
> + * pkt_cnt and hdr_off (offset of
> + * <per-packet metadata entry 1>)
> + *
> + * pkt_cnt is number of entries in the per-packet metadata.
> + * In current firmware there is 2 entries per packet.
> + * The first points to the packet and the
> + * second is a dummy header.
> + * This was done probably to align fields in 64-bit and
> + * maintain compatibility with old firmware.
> + * This code assumes that <dummy header> and <padding2> are
> + * optional.
> + */
> +
> if (skb->len < 4)
> return 0;
> skb_trim(skb, skb->len - 4);
> @@ -1485,51 +1521,66 @@ static int ax88179_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
> /* Make sure that the bounds of the metadata array are inside the SKB
> * (and in front of the counter at the end).
> */
> - if (pkt_cnt * 2 + hdr_off > skb->len)
> + if (pkt_cnt * 4 + hdr_off > skb->len)
> return 0;
> pkt_hdr = (u32 *)(skb->data + hdr_off);
>
> /* Packets must not overlap the metadata array */
> skb_trim(skb, hdr_off);
>
> - for (; ; pkt_cnt--, pkt_hdr++) {
> + for (; pkt_cnt > 0; pkt_cnt--, pkt_hdr++) {
> u16 pkt_len;
> + u16 pkt_len_plus_padd;
Very minor nit: please respect the reverse xmas tree order in variable
declaration - in this case:
u16 pkt_len_plus_padd;
u16 pkt_len;
Other then that, LGTM! Feel free to include my acked-by tag on re-spin.
I understand this is targeting -net, if so please include the correct
tag into the patch subj. Additionally adding a Fixes tag is advised, if
your intention is push this patch to stable trees.
Thanks!
Paolo
Powered by blists - more mailing lists