netdev - [PATCH net] virtio: fix GSO with frames unaligned to size

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-next>] [day] [month] [year] [list]

Message-ID: <20240723223109.2196886-1-kuba@kernel.org>
Date: Tue, 23 Jul 2024 15:31:09 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: davem@...emloft.net
Cc: netdev@...r.kernel.org,
	edumazet@...gle.com,
	pabeni@...hat.com,
	Jakub Kicinski <kuba@...nel.org>,
	willemdebruijn.kernel@...il.com,
	mst@...hat.com,
	jasowang@...hat.com,
	xuanzhuo@...ux.alibaba.com,
	eperezma@...hat.com,
	shuah@...nel.org,
	arefev@...mel.ru,
	virtualization@...ts.linux.dev,
	linux-kselftest@...r.kernel.org
Subject: [PATCH net] virtio: fix GSO with frames unaligned to size

The commit under fixes added a questionable check to
virtio_net_hdr_to_skb(). I'm guessing the check was supposed
to protect from csum offset being outside of a segment
(especially if len is not multiple of segment size).

The condition can't be right, tho, as it breaks previously
working sending of GSO frames with only one segment
(i.e. when gso_size <= len we silently ignore the GSO
request and send a single non-GSO frame).

Fix the logic and move it to the GSO part.

This has been caught by net/tap and net/psock_send.sh tests.

Fixes: e269d79c7d35 ("net: missing check virtio")
Signed-off-by: Jakub Kicinski <kuba@...nel.org>
---
CC: willemdebruijn.kernel@...il.com
CC: mst@...hat.com
CC: jasowang@...hat.com
CC: xuanzhuo@...ux.alibaba.com
CC: eperezma@...hat.com
CC: shuah@...nel.org
CC: arefev@...mel.ru
CC: virtualization@...ts.linux.dev
CC: linux-kselftest@...r.kernel.org
---
 include/linux/virtio_net.h        | 27 ++++++++++++++++-----------
 tools/testing/selftests/net/tap.c | 30 ++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index d1d7825318c3..c54b7aa42921 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -52,11 +52,11 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
 					bool little_endian)
 {
 	unsigned int nh_min_len = sizeof(struct iphdr);
+	unsigned int csum_needed = 0;
 	unsigned int gso_type = 0;
 	unsigned int thlen = 0;
 	unsigned int p_off = 0;
 	unsigned int ip_proto;
-	u64 ret, remainder, gso_size;
 
 	if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
 		switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
@@ -99,16 +99,6 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
 		u32 off = __virtio16_to_cpu(little_endian, hdr->csum_offset);
 		u32 needed = start + max_t(u32, thlen, off + sizeof(__sum16));
 
-		if (hdr->gso_size) {
-			gso_size = __virtio16_to_cpu(little_endian, hdr->gso_size);
-			ret = div64_u64_rem(skb->len, gso_size, &remainder);
-			if (!(ret && (hdr->gso_size > needed) &&
-						((remainder > needed) || (remainder == 0)))) {
-				return -EINVAL;
-			}
-			skb_shinfo(skb)->tx_flags |= SKBFL_SHARED_FRAG;
-		}
-
 		if (!pskb_may_pull(skb, needed))
 			return -EINVAL;
 
@@ -119,6 +109,8 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
 		p_off = nh_min_len + thlen;
 		if (!pskb_may_pull(skb, p_off))
 			return -EINVAL;
+
+		csum_needed = needed;
 	} else {
 		/* gso packets without NEEDS_CSUM do not set transport_offset.
 		 * probe and drop if does not match one of the above types.
@@ -188,6 +180,19 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
 		if (gso_size == GSO_BY_FRAGS)
 			return -EINVAL;
 
+		if (csum_needed) {
+			unsigned int p_rem, p_size;
+
+			p_size = gso_size;
+			p_rem = (skb->len - nh_off) % gso_size;
+			if (p_rem)
+				p_size = p_rem;
+
+			/* Make sure csum still within packet after GSO */
+			if (p_size + nh_off < csum_needed)
+				return -EINVAL;
+		}
+
 		/* Too small packets are not really GSO ones. */
 		if (skb->len - nh_off > gso_size) {
 			shinfo->gso_size = gso_size;
diff --git a/tools/testing/selftests/net/tap.c b/tools/testing/selftests/net/tap.c
index 247c3b3ac1c9..8527d51449cf 100644
--- a/tools/testing/selftests/net/tap.c
+++ b/tools/testing/selftests/net/tap.c
@@ -418,6 +418,36 @@ TEST_F(tap, test_packet_valid_udp_csum)
 	ASSERT_EQ(ret, off);
 }
 
+TEST_F(tap, test_packet_invalid_udp_gso_csum)
+{
+	uint8_t pkt[TEST_PACKET_SZ];
+	uint16_t payload;
+	size_t off;
+	int ret;
+	int i;
+
+	payload = ETH_DATA_LEN - sizeof(struct iphdr) - sizeof(struct udphdr);
+
+	memset(pkt, 0, sizeof(pkt));
+	off = build_test_packet_valid_udp_gso(pkt, payload);
+
+	for (i = -16; i < 16; i++) {
+		ret = write(self->fd, pkt, off + i);
+
+		if (i <= 0 ||
+		    i > __builtin_offsetof(struct udphdr, check) + 1) {
+			EXPECT_EQ(ret, off + i)
+				TH_LOG("mismatch with offset: %d (%zd)",
+				       i, off + i);
+		} else {
+			EXPECT_EQ(ret, -1)
+				TH_LOG("mismatch with offset: %d (%zd)",
+				       i, off + i);
+			EXPECT_EQ(errno, 22);
+		}
+	}
+}
+
 TEST_F(tap, test_packet_crash_tap_invalid_eth_proto)
 {
 	uint8_t pkt[TEST_PACKET_SZ];
-- 
2.45.2