[PATCH] lan78xx: fix ip header misalignment

[PATCH] lan78xx: fix ip header misalignment

[Yuriy M. Kaminskiy]

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

lan78xx.c:rx_submit() allocates space for frame-to-be-received with 
netdev_alloc_skb_ip_align(), which misalign start of buffer by 2 bytes 
in expectation that frame will start from 14-byte ethernet header, then 
ip header; if start of buffer misaligned by 2 bytes, ip header will be 
16-byte aligned.

Unfortunately, usb frame that is sent by lan78xx starts with another 
10-byte header (lan78xx_rx(): rx_cmd_a/rx_cmd_b/rx_cmd_c), *then* 
follows ethernet header, and *then* ip header (which ends up being 
misaligned).

This issue was observed on arm platform (where misaligned 32-bit word 
access triggers exception and leaves traces in /proc/cpu/alignment, see
https://github.com/raspberrypi/linux/issues/2599 ; for me, about any 
ipv6 traffic that hits machine - `ping -I eth0 ip6-allnodes`, tcp/udp 
packets, etc triggered increase in this counter, with 
ip6_datagram_recv_common_ctl, icmpv6_echo_reply, etc as culprit).

If we just allocate skb data without any misalignment tricks, ip header 
will end up and at offset 24 (8-byte aligned).

Patch attached; runtime-tested with raspbian fork of stable/4.14.y 
[4.14.92] on Raspberry pi 3B+ (it is slightly different from mainline, 
but patch should not have any conflicts, all affected code is pretty same).

P.S. I'm not subscribed, please CC me on reply.

[-- Attachment #2: 0001-lan78xx-fix-ip-header-misalignment.patch --]
[-- Type: text/x-patch, Size: 991 bytes --]

From 2bd6b0a11e222be2df97da948924c71bf13d7192 Mon Sep 17 00:00:00 2001
From: "Yuriy M. Kaminskiy" <yumkam@gmail.com>
Date: Mon, 21 Jan 2019 02:51:24 +0300
Subject: [PATCH] lan78xx: fix ip header misalignment

As lan78xx prepends 10-byte header before ether_hdr, skb->data
misalignment trick by netdev_alloc_skb_ip_align() made things
worse (ip_hdr becomes always misaligned).

See https://github.com/raspberrypi/linux/issues/2599
---
 drivers/net/usb/lan78xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 9b782cdf8..d64b0d3b8 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -3143,7 +3143,7 @@ static int rx_submit(struct lan78xx_net *dev, struct urb *urb, gfp_t flags)
 	size_t size = dev->rx_urb_size;
 	int ret = 0;
 
-	skb = netdev_alloc_skb_ip_align(dev->net, size);
+	skb = netdev_alloc_skb(dev->net, size);
 	if (!skb) {
 		usb_free_urb(urb);
 		return -ENOMEM;
-- 
2.11.0

RE: [PATCH] lan78xx: fix ip header misalignment

[RaghuramChary.Jallipalli]

Hi Yuiry,
Thanks for the patch.
Changes look good to me.

Thanks,
-Raghu

> -----Original Message-----
> From: Yuriy M. Kaminskiy <yumkam@gmail.com>
> Sent: Monday, January 21, 2019 6:18 PM
> To: netdev@vger.kernel.org
> Cc: Woojung Huh - C21699 <Woojung.Huh@microchip.com>;
> UNGLinuxDriver <UNGLinuxDriver@microchip.com>
> Subject: [PATCH] lan78xx: fix ip header misalignment
> 
> lan78xx.c:rx_submit() allocates space for frame-to-be-received with
> netdev_alloc_skb_ip_align(), which misalign start of buffer by 2 bytes in
> expectation that frame will start from 14-byte ethernet header, then ip
> header; if start of buffer misaligned by 2 bytes, ip header will be 16-byte
> aligned.
> 
> Unfortunately, usb frame that is sent by lan78xx starts with another 10-byte
> header (lan78xx_rx(): rx_cmd_a/rx_cmd_b/rx_cmd_c), *then* follows
> ethernet header, and *then* ip header (which ends up being misaligned).
> 
> This issue was observed on arm platform (where misaligned 32-bit word
> access triggers exception and leaves traces in /proc/cpu/alignment, see
> https://github.com/raspberrypi/linux/issues/2599 ; for me, about any
> ipv6 traffic that hits machine - `ping -I eth0 ip6-allnodes`, tcp/udp packets, etc
> triggered increase in this counter, with ip6_datagram_recv_common_ctl,
> icmpv6_echo_reply, etc as culprit).
> 
> If we just allocate skb data without any misalignment tricks, ip header will
> end up and at offset 24 (8-byte aligned).
> 
> Patch attached; runtime-tested with raspbian fork of stable/4.14.y [4.14.92]
> on Raspberry pi 3B+ (it is slightly different from mainline, but patch should
> not have any conflicts, all affected code is pretty same).
> 
> P.S. I'm not subscribed, please CC me on reply.

Re: [PATCH] lan78xx: fix ip header misalignment

[Yuriy M. Kaminskiy]

[-- Attachment #1: Type: text/plain, Size: 2989 bytes --]

On 21.01.2019 14:47, Yuriy M. Kaminskiy wrote:
> lan78xx.c:rx_submit() allocates space for frame-to-be-received with 
> netdev_alloc_skb_ip_align(), which misalign start of buffer by 2 bytes 
> in expectation that frame will start from 14-byte ethernet header, then 
> ip header; if start of buffer misaligned by 2 bytes, ip header will be 
> 16-byte aligned.
> 
> Unfortunately, usb frame that sent by lan78xx starts with another 
> 10-byte header (lan78xx_rx(): rx_cmd_a/rx_cmd_b/rx_cmd_c), *then* 
> follows ethernet header, and *then* ip header (which ends up being 
> misaligned).
> 
> This issue was noticed on arm platform (where misaligned 32-bit word 
> access triggers exception and leave traces in /proc/cpu/alignment, see
> https://github.com/raspberrypi/linux/issues/2599 ; for me, about any 
> ipv6 traffic that hits machine - `ping -I eth0 ip6-allnodes`, tcp/udp 
> packets, etc triggered increase in this counter, with 
> ip6_datagram_recv_common_ctl, icmpv6_echo_reply, etc as culprit).
> 
> If we just allocate skb data without any misalignment tricks, ip header 
> will end up and at offset 24 (8-byte aligned).
> 
> Patch attached; runtime-tested with raspbian fork of stable/4.14.y 
> [4.14.92] on Raspberry pi 3B+

I quickly reviewed other usbnet drivers for similar problems, it looks like:

1) ax88179_178a.c likely has similar problem (multiple packets in usb 
frame with 2-byte packet header, padded at end to 8-byte boundary; if 
first packet alignment will be fixed, it will fix all of them) and 
contains several potential read buffer overflows with misbehaving or 
malicious device.

2) cdc_eem.c likely has similar problem (usb frame contains several 
packets with 2-byte header), see eem_rx_fixup()

3) cx82310_eth.c  - ditto, see cx82310_rx_fixup()

4) dm9601.c - weird 3-byte header, affected, would need 7 bytes 
prepended to compensate, see dm9601_rx_fixup()

5) kalmia.c - likely affected, 6-byte header; no padding/alignment 
between packets, so 2nd and following packet may end up misaligned anyway.

6) lg-vl600.c - unsure; multi-packet frames, first packet probably fine, 
but no padding/alignment between packets, so 2nd and following packets 
may end up misaligned anyway.

7) net1080.c - something weird, probably affected.

8) pegasus.c - (not usbnet based) not sure, but `pegasus->chip == 
0x8513` seems affected (2-byte header prepended to ethernet frame), see 
read_bulk_callback()

9) rndis_host.c - unpredictable, packet offset is driven by device?

10) sierra_net.c - ditto

11) smsc75xx.c - 10-byte packet header, affected, padded to 4-bytes 
alignment at end;

12) smsc95xx.c [fixed in master]

13) sr9700.c - weird 3-byte packet header, multiply packets; no 
alignment between packets, so 2nd and following packet may end up 
misaligned anyway.

Given I have no access to hw, I won't try to compose patches for them 
(except for not-even-compile-tested patch to ax88179_178a.c as PoC).

P.S. I'm not subscribed, please CC me on reply.

[-- Attachment #2: ax88179_178a.patch --]
[-- Type: text/x-patch, Size: 1924 bytes --]

diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index 0f69b77e8..85e48000f 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -1234,6 +1234,9 @@ static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
 	struct ax88179_data *ax179_data = (struct ax88179_data *)dev->data;
 	struct ethtool_eee eee_data;
 
+	/* XXX is this a right place? */
+	set_bit(EVENT_NO_IP_ALIGN, &dev->flags);
+
 	usbnet_get_endpoints(dev, intf);
 
 	tmp16 = (u16 *)buf;
@@ -1383,6 +1386,10 @@ static int ax88179_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
 
 	pkt_cnt = (u16)rx_hdr;
 	hdr_off = (u16)(rx_hdr >> 16);
+	if (hdr_off + sizeof(u32)*pkt_cnt > skb->len) {
+		/* XXX misbehaving or malicious device, report? */
+		return 0;
+	}
 	pkt_hdr = (u32 *)(skb->data + hdr_off);
 
 	while (pkt_cnt--) {
@@ -1390,11 +1397,19 @@ static int ax88179_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
 
 		le32_to_cpus(pkt_hdr);
 		pkt_len = (*pkt_hdr >> 16) & 0x1fff;
+		if (pkt_len + 2 > skb->len) {
+			/* XXX misbehaving or malicious device, report? */
+			return 0;
+		}
 
 		/* Check CRC or runt packet */
 		if ((*pkt_hdr & AX_RXHDR_CRC_ERR) ||
 		    (*pkt_hdr & AX_RXHDR_DROP_ERR)) {
-			skb_pull(skb, (pkt_len + 7) & 0xFFF8);
+			/* XXX actual packet size is (2+pkt_len), but we advance by pkt_len here? */
+			if (skb_pull(skb, (pkt_len + 7) & 0xFFF8) == NULL) {
+				/* misbehaving or malicious device, report? */
+				return 0;
+			}
 			pkt_hdr++;
 			continue;
 		}
@@ -1421,7 +1436,11 @@ static int ax88179_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
 			return 0;
 		}
 
-		skb_pull(skb, (pkt_len + 7) & 0xFFF8);
+		/* XXX packet size is always (2+pkt_len), but we advance by pkt_len here? */
+		if (skb_pull(skb, (pkt_len + 7) & 0xFFF8) == NULL) {
+			/* misbehaving or malicious device, report? */
+			return 0;
+		}
 		pkt_hdr++;
 	}
 	return 1;