[PATCH 00/10] net: fix problems handling trailing bytes

[PATCH 00/10] net: fix problems handling trailing bytes

[Ahmad Fatoum]

This started by pinging a Raspberry Pi 3b running barebox sitting behind
a router employing conntrack. The router discarded the ping responses
due to the wrong ICMP checksum and these issues do not pop up normally
because the ping command itself doesn't bother to verify the checksum.

This unearthed issues in two drivers as well as the barebox network
stack itself.

See the commit messages for details.

Ahmad Fatoum (10):
  net: free packets with net_free_packet
  net: ip: don't blindly trust driver supplied frame size
  net: icmp: don't blindly trust driver supplied frame size
  net: icmp: properly set IP TTL and fragement fields
  net: icmp: don't overrun buffer on send
  net: cpsw: report correct frame size to network stack
  net: usb: smsc95xx: don't opencode get/put_aligned_le32
  net: usb: smsc95xx: don't blindly trust hardware size
  net: usb: smsc95xx: fix handling of multiple packets per urb
  net: usb: smsc95xx: disable HW checksumming in driver

 drivers/net/cpsw.c         |  7 +++-
 drivers/net/usb/smsc95xx.c | 68 ++++++++++++++++----------------------
 net/net.c                  | 64 +++++++++++++++++++++++++++++------
 3 files changed, 88 insertions(+), 51 deletions(-)

-- 
2.39.2

[PATCH 01/10] net: free packets with net_free_packet

[Ahmad Fatoum]

While ultimately the same (net_free_packet == dma_free == free), this
will not necessarily be always the case, so let's use the dedicated
net_free_packet function to free the packets.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 net/net.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/net.c b/net/net.c
index c06e88fe532d..2625fb8604fb 100644
--- a/net/net.c
+++ b/net/net.c
@@ -465,7 +465,7 @@ static struct net_connection *net_new(struct eth_device *edev, IPaddr_t dest,
 
 	return con;
 out:
-	free(con->packet);
+	net_free_packet(con->packet);
 	free(con);
 	return ERR_PTR(ret);
 }
@@ -510,7 +510,7 @@ struct net_connection *net_icmp_new(IPaddr_t dest, rx_handler_f *handler,
 void net_unregister(struct net_connection *con)
 {
 	list_del(&con->list);
-	free(con->packet);
+	net_free_packet(con->packet);
 	free(con);
 }
 
@@ -564,7 +564,7 @@ static int net_answer_arp(struct eth_device *edev, unsigned char *pkt, int len)
 		return 0;
 	memcpy(packet, pkt, ETHER_HDR_SIZE + ARP_HDR_SIZE);
 	ret = eth_send(edev, packet, ETHER_HDR_SIZE + ARP_HDR_SIZE);
-	free(packet);
+	net_free_packet(packet);
 
 	return ret;
 }
@@ -677,7 +677,7 @@ static int ping_reply(struct eth_device *edev, unsigned char *pkt, int len)
 
 	ret = eth_send(edev, packet, ETHER_HDR_SIZE + len);
 
-	free(packet);
+	net_free_packet(packet);
 
 	return ret;
 }
-- 
2.39.2

[PATCH 02/10] net: ip: don't blindly trust driver supplied frame size

[Ahmad Fatoum]

We currently assume that the size reported by the network driver
reflects the number of bytes actually transmitted over the wire, but
this is apparently not always the case, at least for the barebox cpsw
and smc95xx drivers. These don't handle hardware checksum offloading
correctly and thus pass extraneous checksum bytes inserted by the
hardware to the network stack as if these were part of the transmitted
frame.

These drivers will be fixed in follow-up commits, but on the off-chance
more drivers are affected, let's use the size reported in the IP header
when doing IP packet processing.

As the old size is needed to dump the packet in net_bad_packet(), this
is moved inside the new function.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 net/net.c | 43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/net/net.c b/net/net.c
index 2625fb8604fb..56a599d0bc61 100644
--- a/net/net.c
+++ b/net/net.c
@@ -645,6 +645,43 @@ static int net_handle_udp(unsigned char *pkt, int len)
 	return -EINVAL;
 }
 
+static struct iphdr *ip_verify_size(unsigned char *pkt, int *total_len_nic)
+{
+	struct iphdr *ip = (struct iphdr *)(pkt + ETHER_HDR_SIZE);
+	int total_len_pkt;
+
+	total_len_pkt = ntohs(ip->tot_len) + ETHER_HDR_SIZE;
+
+	/* Only trust the packet's size if it's within bounds */
+	if (*total_len_nic < sizeof(struct ethernet) + sizeof(struct iphdr) ||
+	    *total_len_nic < total_len_pkt) {
+		net_bad_packet(pkt, *total_len_nic);
+		return NULL;
+	}
+
+#ifdef DEBUG
+	/* Hitting this warning means we have trailing bytes after the IP
+	 * payload that are not needed for padding.
+	 *
+	 * This may be an indication that the NIC driver is doing funny
+	 * offloading stuff it shouldn't, but can also mean that some sender
+	 * in the network likes to waste bit time for nought.
+	 *
+	 * We can't differentiate between the two, so we just print the
+	 * warning when DEBUG is defined, so developers may investigate the
+	 * reason without annoying users about something that might not even
+	 * be barebox's fault.
+	 */
+	if (WARN_ON_ONCE(*total_len_nic > total_len_pkt &&
+			 *total_len_nic > 64)) {
+		net_bad_packet(pkt, *total_len_nic);
+	}
+#endif
+
+	*total_len_nic = total_len_pkt;
+	return ip;
+}
+
 static int ping_reply(struct eth_device *edev, unsigned char *pkt, int len)
 {
 	struct ethernet *et = (struct ethernet *)pkt;
@@ -709,10 +746,10 @@ static int net_handle_ip(struct eth_device *edev, unsigned char *pkt, int len)
 
 	pr_debug("%s\n", __func__);
 
-	if (len < sizeof(struct ethernet) + sizeof(struct iphdr) ||
-		len < ETHER_HDR_SIZE + ntohs(ip->tot_len)) {
+	ip = ip_verify_size(pkt, &len);
+	if (!ip) {
 		pr_debug("%s: bad len\n", __func__);
-		goto bad;
+		return 0;
 	}
 
 	if ((ip->hl_v & 0xf0) != 0x40)
-- 
2.39.2

[PATCH 03/10] net: icmp: don't blindly trust driver supplied frame size

[Ahmad Fatoum]

The barebox network stack calls ping_reply() to handle ICMP echo
requests. The function copies destination addresses to source addresses,
fixes up some fields, recalculates checksums and then sends back the
ICMP echo response.

There are two checksums involved: The IP checksum over the IP header and
the ICMP header spanning the ICMP payload, which extends to the end of
packet.

The number of bytes that the ICMP checksum should be computed for was
being calculated as total_size_reported_by_nic - iphdr_size - ethhdr_size.

This is wrong as it assumes that the IP payload extends until the end of
packet. There are multiple reasons this may not be the case:

  - The packet is smaller than the minimum and is thus padded.
  - The sender adds trailing bytes after the IP payload for no reason.
  - The network driver reports a wrong size, because it doesn't
    correctly handle extra bytes added by hardware checksum offloading.

The last one affects the barebox cpsw and smsc95xx drivers, which will
be fixed up in follow-up commits.

The proper solution is to use the same length for the payload and
for its checksum. Do this by using the new ip_verify_size(), which will
take care to fix up the size discrepancy.

This issue was found by trying to ping a Raspberry Pi 3b running barebox
sitting behind a router employing conntrack, which apparently discarded
the ping responses due to the wrong ICMP checksum. These issues did not
occur without such a router in-beween, because the ping command itself
doesn't bother to verify the checksum.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
Cc: Jan Lübbe <jlu@pengutronix.de>
Cc: Björn Lässig <bla@pengutronix.de>
---
 net/net.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/net.c b/net/net.c
index 56a599d0bc61..6745085635dc 100644
--- a/net/net.c
+++ b/net/net.c
@@ -686,7 +686,7 @@ static int ping_reply(struct eth_device *edev, unsigned char *pkt, int len)
 {
 	struct ethernet *et = (struct ethernet *)pkt;
 	struct icmphdr *icmp;
-	struct iphdr *ip = (struct iphdr *)(pkt + ETHER_HDR_SIZE);
+	struct iphdr *ip;
 	unsigned char *packet;
 	int ret;
 
@@ -696,6 +696,10 @@ static int ping_reply(struct eth_device *edev, unsigned char *pkt, int len)
 
 	icmp = net_eth_to_icmphdr(pkt);
 
+	ip = ip_verify_size(pkt, &len);
+	if (!ip)
+		return -EILSEQ;
+
 	icmp->type = ICMP_ECHO_REPLY;
 	icmp->checksum = 0;
 	icmp->checksum = ~net_checksum((unsigned char *)icmp,
-- 
2.39.2

[PATCH 04/10] net: icmp: properly set IP TTL and fragement fields

[Ahmad Fatoum]

We shouldn't keep using the TTL value of the ICMP echo request,
as we are sending a fresh packet, therefore restore it to the maximum
value.

While at it, also fix the frag_off field: A fragment offset of 0 on its
own doesn't mean that there's no fragmentation, but that this is the
first fragment. Writing 0x4000 there sets the "Don't fragment" bit,
which we are already setting for all other IP communication and should
be setting here as well.

Suggested-by: Jan Lübbe <j.luebbe@pengutronix.de>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 net/net.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/net.c b/net/net.c
index 6745085635dc..754a764d2a49 100644
--- a/net/net.c
+++ b/net/net.c
@@ -705,7 +705,8 @@ static int ping_reply(struct eth_device *edev, unsigned char *pkt, int len)
 	icmp->checksum = ~net_checksum((unsigned char *)icmp,
 				       len - sizeof(struct iphdr) - ETHER_HDR_SIZE);
 	ip->check = 0;
-	ip->frag_off = 0;
+	ip->frag_off = htons(0x4000);
+	ip->ttl = 255;
 	net_copy_ip((void *)&ip->daddr, &ip->saddr);
 	net_copy_ip((void *)&ip->saddr, &edev->ipaddr);
 	ip->check = ~net_checksum((unsigned char *)ip, sizeof(struct iphdr));
-- 
2.39.2

[PATCH 05/10] net: icmp: don't overrun buffer on send

[Ahmad Fatoum]

len is the size of the ICMP echo request and the ICMP echo response will
have the exact same size, but the code erroneously, copied ETHER_HDR_SIZE
bytes worth of extra data beyond the buffer and sent that out.

Fix this by using the correct length.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 net/net.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/net.c b/net/net.c
index 754a764d2a49..a9d130635417 100644
--- a/net/net.c
+++ b/net/net.c
@@ -715,9 +715,9 @@ static int ping_reply(struct eth_device *edev, unsigned char *pkt, int len)
 	if (!packet)
 		return 0;
 
-	memcpy(packet, pkt, ETHER_HDR_SIZE + len);
+	memcpy(packet, pkt, len);
 
-	ret = eth_send(edev, packet, ETHER_HDR_SIZE + len);
+	ret = eth_send(edev, packet, len);
 
 	net_free_packet(packet);
 
-- 
2.39.2

[PATCH 06/10] net: cpsw: report correct frame size to network stack

[Ahmad Fatoum]

If the NIC reports that there is a computed CRC appended to the frame
content, we need to subtract its size from the total length before
passing that further down the network stack.

This fixes a WARN_ON_ONCE reported by the network stack when built
with #define DEBUG, because the expected size as indicated by the IP
header is not the same as what's reported by the driver.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/net/cpsw.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/cpsw.c b/drivers/net/cpsw.c
index 31ca61a230e9..3aafebaad49d 100644
--- a/drivers/net/cpsw.c
+++ b/drivers/net/cpsw.c
@@ -51,10 +51,12 @@
 #define CPDMA_DESC_EOP		BIT(30)
 #define CPDMA_DESC_OWNER	BIT(29)
 #define CPDMA_DESC_EOQ		BIT(28)
+#define CPDMA_DESC_PASS_CRC	BIT(26)
 #define CPDMA_DESC_TO_PORT_EN	BIT(20)
 #define CPDMA_FROM_TO_PORT_SHIFT	16
 #define CPDMA_RX_SOURCE_PORT(__status__)	\
 	(((__status__) >> CPDMA_FROM_TO_PORT_SHIFT) & 0x7)
+#define CPDMA_DESC_CRC_LEN	4
 
 #define SLIVER_SIZE		0x40
 
@@ -865,8 +867,11 @@ static int cpdma_process(struct cpsw_slave *slave, struct cpdma_chan *chan,
 
 	status = readl(&desc->hw_mode);
 
-	if (len)
+	if (len) {
 		*len = status & 0x7ff;
+		if (status & CPDMA_DESC_PASS_CRC)
+			*len -= CPDMA_DESC_CRC_LEN;
+	}
 
 	if (dma)
 		*dma = readl(&desc->hw_buffer);
-- 
2.39.2

[PATCH 07/10] net: usb: smsc95xx: don't opencode get/put_aligned_le32

[Ahmad Fatoum]

This introduces no functional change, but makes code easier to read and
aligns us some more with the current Linux state of the driver.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/net/usb/smsc95xx.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index b6f81cfab825..ab0059fcdb0f 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -7,6 +7,7 @@
 #include <net.h>
 #include <linux/usb/usb.h>
 #include <linux/usb/usbnet.h>
+#include <asm/unaligned.h>
 #include <malloc.h>
 #include <asm/byteorder.h>
 #include <errno.h>
@@ -757,8 +758,7 @@ static int smsc95xx_rx_fixup(struct usbnet *dev, void *buf, int len)
 		unsigned char *packet;
 		u16 size;
 
-		memcpy(&header, buf, sizeof(header));
-		le32_to_cpus(&header);
+		header = get_unaligned_le32(buf);
 		buf += 4 + NET_IP_ALIGN;
 		len -= 4 + NET_IP_ALIGN;
 		packet = buf;
@@ -816,13 +816,11 @@ static int smsc95xx_tx_fixup(struct usbnet *dev,
 {
 	u32 tx_cmd_a, tx_cmd_b;
 
-	tx_cmd_a = (u32)(len) | TX_CMD_A_FIRST_SEG_ | TX_CMD_A_LAST_SEG_;
-	cpu_to_le32s(&tx_cmd_a);
-	memcpy(nbuf, &tx_cmd_a, 4);
+	tx_cmd_b = (u32)len;
+	tx_cmd_a = tx_cmd_b | TX_CMD_A_FIRST_SEG_ | TX_CMD_A_LAST_SEG_;
 
-	tx_cmd_b = (u32)(len);
-	cpu_to_le32s(&tx_cmd_b);
-	memcpy(nbuf + 4, &tx_cmd_b, 4);
+	put_unaligned_le32(tx_cmd_a, nbuf);
+	put_unaligned_le32(tx_cmd_b, nbuf + 4);
 
 	memcpy(nbuf + 8, buf, len);
 
-- 
2.39.2

[PATCH 08/10] net: usb: smsc95xx: don't blindly trust hardware size

[Ahmad Fatoum]

This is a port of Linux commit ff821092cf02a70c2bccd2d19269f01e29aa52cf:

| Author:     Szymon Heidrich <szymon.heidrich@gmail.com>
| AuthorDate: Thu Mar 16 11:19:54 2023 +0100
|
| net: usb: smsc95xx: Limit packet length to skb->len
|
| Packet length retrieved from descriptor may be larger than
| the actual socket buffer length. In such case the cloned
| skb passed up the network stack will leak kernel memory
| contents.
|
| Fixes: 2f7ca802bdae ("net: Add SMSC LAN9500 USB2.0 10/100 ethernet adapter driver")
| Signed-off-by: Szymon Heidrich <szymon.heidrich@gmail.com>
| Reviewed-by: Jakub Kicinski <kuba@kernel.org>
| Link: https://lore.kernel.org/r/20230316101954.75836-1-szymon.heidrich@gmail.com
| Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/net/usb/smsc95xx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index ab0059fcdb0f..1587128368ad 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -767,6 +767,12 @@ static int smsc95xx_rx_fixup(struct usbnet *dev, void *buf, int len)
 		size = (u16)((header & RX_STS_FL_) >> 16);
 		align_count = (4 - ((size + NET_IP_ALIGN) % 4)) % 4;
 
+		if (unlikely(size > len)) {
+			netif_dbg(dev, rx_err, dev->net,
+				  "size err header=0x%08x\n", header);
+			return 0;
+		}
+
 		if (header & RX_STS_ES_) {
 			netif_dbg(dev, rx_err, dev->net,
 				  "Error header=0x%08x\n", header);
-- 
2.39.2

[PATCH 09/10] net: usb: smsc95xx: fix handling of multiple packets per urb

[Ahmad Fatoum]

len is the actual length of the USB bulk transfer, while size is the
length of the current packet, which may be different if we have multiple
packets per transfer.

We don't seem to run into this in barebox, perhaps because of our MTU,
but let's fix it anyway.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/net/usb/smsc95xx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 1587128368ad..291e3c2f80f7 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -790,14 +790,17 @@ static int smsc95xx_rx_fixup(struct usbnet *dev, void *buf, int len)
 				return 1;
 			}
 
-			net_receive(&dev->edev, packet, len - 4);
+			net_receive(&dev->edev, packet, size - 4);
 		}
 
 		len -= size;
+		buf += size;
 
 		/* padding bytes before the next frame starts */
-		if (len)
+		if (len) {
 			len -= align_count;
+			buf += size;
+		}
 	}
 
 	if (len < 0) {
-- 
2.39.2

[PATCH 10/10] net: usb: smsc95xx: disable HW checksumming in driver

[Ahmad Fatoum]

We currently enable hardware checksumming, which means the NIC will add
two bytes of checksum after each packet with the computed checksum. We
don't use those two bytes however and will recompute the checksum
anyway.

Because we also don't adjust the size reported to the network stack to
be 2 bytes shorter, we trigger the newly introduced WARN_ON_ONCE when
DEBUG is #define'd that checks this condition, so let's just disable
the checksumming altogether and remove left-over dead code associated
with it.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/net/usb/smsc95xx.c | 41 +++++++++++---------------------------
 1 file changed, 12 insertions(+), 29 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 291e3c2f80f7..a90041731a66 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -24,8 +24,6 @@
 #define MAX_SINGLE_PACKET_SIZE		(2048)
 #define LAN95XX_EEPROM_MAGIC		(0x9500)
 #define EEPROM_MAC_OFFSET		(0x01)
-#define DEFAULT_TX_CSUM_ENABLE		(1)
-#define DEFAULT_RX_CSUM_ENABLE		(1)
 #define SMSC95XX_INTERNAL_PHY_ID	(1)
 #define SMSC95XX_TX_OVERHEAD		(8)
 #define SMSC95XX_TX_OVERHEAD_CSUM	(12)
@@ -46,8 +44,6 @@
 
 struct smsc95xx_priv {
 	u32 mac_cr;
-	int use_tx_csum;
-	int use_rx_csum;
 };
 
 static int turbo_mode = 0;
@@ -320,10 +316,9 @@ static void smsc95xx_set_multicast(struct usbnet *dev)
 	smsc95xx_write_reg(dev, MAC_CR, pdata->mac_cr);
 }
 
-/* Enable or disable Tx & Rx checksum offload engines */
-static int smsc95xx_set_csums(struct usbnet *dev)
+/* Disable Tx & Rx IP checksum offload engines */
+static int smsc95xx_disable_csums(struct usbnet *dev)
 {
-	struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
 	u32 read_buf;
 	int ret = smsc95xx_read_reg(dev, COE_CR, &read_buf);
 	if (ret < 0) {
@@ -331,15 +326,8 @@ static int smsc95xx_set_csums(struct usbnet *dev)
 		return ret;
 	}
 
-	if (pdata->use_tx_csum)
-		read_buf |= Tx_COE_EN_;
-	else
-		read_buf &= ~Tx_COE_EN_;
-
-	if (pdata->use_rx_csum)
-		read_buf |= Rx_COE_EN_;
-	else
-		read_buf &= ~Rx_COE_EN_;
+	read_buf &= ~Tx_COE_EN_;
+	read_buf &= ~Rx_COE_EN_;
 
 	ret = smsc95xx_write_reg(dev, COE_CR, read_buf);
 	if (ret < 0) {
@@ -670,7 +658,13 @@ static int smsc95xx_reset(struct usbnet *dev)
 		return ret;
 	}
 
-	ret = smsc95xx_set_csums(dev);
+	/*
+	 * barebox network stack doesn't care for hardware checksum offloading,
+	 * so this enabling them doesn't help and indeed introduces breakage:
+	 * The driver will be unaware of the two byte COE trailer and thus packet
+	 * sizes reported will be 2 bytes more than what was actually transmitted.
+	 */
+	ret = smsc95xx_disable_csums(dev);
 	if (ret < 0) {
 		netdev_warn(dev->net, "Failed to set csum offload: %d\n", ret);
 		return ret;
@@ -724,9 +718,6 @@ static int smsc95xx_bind(struct usbnet *dev)
 		return -ENOMEM;
 	}
 
-	pdata->use_tx_csum = DEFAULT_TX_CSUM_ENABLE;
-	pdata->use_rx_csum = DEFAULT_RX_CSUM_ENABLE;
-
 	/* Init all registers */
 	ret = smsc95xx_reset(dev);
 
@@ -810,15 +801,7 @@ static int smsc95xx_rx_fixup(struct usbnet *dev, void *buf, int len)
 
 	return 1;
 }
-#if 0
-static u32 smsc95xx_calc_csum_preamble(struct sk_buff *skb)
-{
-	int len = skb->data - skb->head;
-	u16 high_16 = (u16)(skb->csum_offset + skb->csum_start - len);
-	u16 low_16 = (u16)(skb->csum_start - len);
-	return (high_16 << 16) | low_16;
-}
-#endif
+
 static int smsc95xx_tx_fixup(struct usbnet *dev,
                                 void *buf, int len,
                                 void *nbuf, int *nlen)
-- 
2.39.2

Re: [PATCH 00/10] net: fix problems handling trailing bytes

[Ahmad Fatoum]

On 04.04.24 20:39, Ahmad Fatoum wrote:
> This started by pinging a Raspberry Pi 3b running barebox sitting behind
> a router employing conntrack. The router discarded the ping responses
> due to the wrong ICMP checksum and these issues do not pop up normally
> because the ping command itself doesn't bother to verify the checksum.

Fix for iputils ping(1) here: https://github.com/iputils/iputils/pull/534

> 
> This unearthed issues in two drivers as well as the barebox network
> stack itself.
> 
> See the commit messages for details.
> 
> Ahmad Fatoum (10):
>   net: free packets with net_free_packet
>   net: ip: don't blindly trust driver supplied frame size
>   net: icmp: don't blindly trust driver supplied frame size
>   net: icmp: properly set IP TTL and fragement fields
>   net: icmp: don't overrun buffer on send
>   net: cpsw: report correct frame size to network stack
>   net: usb: smsc95xx: don't opencode get/put_aligned_le32
>   net: usb: smsc95xx: don't blindly trust hardware size
>   net: usb: smsc95xx: fix handling of multiple packets per urb
>   net: usb: smsc95xx: disable HW checksumming in driver
> 
>  drivers/net/cpsw.c         |  7 +++-
>  drivers/net/usb/smsc95xx.c | 68 ++++++++++++++++----------------------
>  net/net.c                  | 64 +++++++++++++++++++++++++++++------
>  3 files changed, 88 insertions(+), 51 deletions(-)
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] fixup! net: usb: smsc95xx: fix handling of multiple packets per urb

[Ahmad Fatoum]

We need to advance the buffer with the same value that is subtracted
from the length (skb_pull).

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/net/usb/smsc95xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 291e3c2f80f7..2e085c435dbe 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -799,7 +799,7 @@ static int smsc95xx_rx_fixup(struct usbnet *dev, void *buf, int len)
 		/* padding bytes before the next frame starts */
 		if (len) {
 			len -= align_count;
-			buf += size;
+			buf += align_count;
 		}
 	}
 
-- 
2.39.2

Re: [PATCH 00/10] net: fix problems handling trailing bytes

[Sascha Hauer]

On Thu, 04 Apr 2024 20:39:51 +0200, Ahmad Fatoum wrote:
> This started by pinging a Raspberry Pi 3b running barebox sitting behind
> a router employing conntrack. The router discarded the ping responses
> due to the wrong ICMP checksum and these issues do not pop up normally
> because the ping command itself doesn't bother to verify the checksum.
> 
> This unearthed issues in two drivers as well as the barebox network
> stack itself.
> 
> [...]

Applied, thanks!

[01/10] net: free packets with net_free_packet
        https://git.pengutronix.de/cgit/barebox/commit/?id=006fd6e86707 (link may not be stable)
[02/10] net: ip: don't blindly trust driver supplied frame size
        https://git.pengutronix.de/cgit/barebox/commit/?id=1b2559919d13 (link may not be stable)
[03/10] net: icmp: don't blindly trust driver supplied frame size
        https://git.pengutronix.de/cgit/barebox/commit/?id=d1316e6745d0 (link may not be stable)
[04/10] net: icmp: properly set IP TTL and fragement fields
        https://git.pengutronix.de/cgit/barebox/commit/?id=e2f7ec4625fa (link may not be stable)
[05/10] net: icmp: don't overrun buffer on send
        https://git.pengutronix.de/cgit/barebox/commit/?id=ad753cf0f5ce (link may not be stable)
[06/10] net: cpsw: report correct frame size to network stack
        https://git.pengutronix.de/cgit/barebox/commit/?id=20ece44e01aa (link may not be stable)
[07/10] net: usb: smsc95xx: don't opencode get/put_aligned_le32
        https://git.pengutronix.de/cgit/barebox/commit/?id=209cca7cef6f (link may not be stable)
[08/10] net: usb: smsc95xx: don't blindly trust hardware size
        https://git.pengutronix.de/cgit/barebox/commit/?id=9a868a27e334 (link may not be stable)
[09/10] net: usb: smsc95xx: fix handling of multiple packets per urb
        https://git.pengutronix.de/cgit/barebox/commit/?id=1ef8e95ec855 (link may not be stable)
[10/10] net: usb: smsc95xx: disable HW checksumming in driver
        https://git.pengutronix.de/cgit/barebox/commit/?id=b83c88237593 (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>