more virtio-net csum hardening

virtio_net_hdr_to_skb() refuses packets whose checksum metadata it cannot make sense of, but the vector raw transport ignored its return value: rejected packets went on into the stack with their metadata unvalidated.

Propagate the failure.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 arch/um/drivers/vector_transports.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

arch/um/drivers/vector_transports.c +0 -0

index e1fb2a76fdff..8b7548458663 100644

Quoted 18 lines · Michael S. Tsirkin

virtio_net_hdr_to_skb() refuses packets whose checksum metadata it cannot make sense of, but the vector raw transport ignored its return value: rejected packets went on into the stack with their metadata unvalidated.

Propagate the failure.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 arch/um/drivers/vector_transports.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

arch/um/drivers/vector_transports.c +0 -0

index e1fb2a76fdff..8b7548458663 100644

arch/um/drivers/vector_transports.c static int raw_verify_header( · line 217

Quoted 9 lines

arch/um/drivers/vector_transports.c +3 -1

@@ -210,7 +210,9 @@ static int raw_verify_header(
 		return 1;
 
 	skb_set_network_header(skb, ETH_HLEN);
-	virtio_net_hdr_to_skb(skb, vheader, virtio_legacy_is_little_endian());
+	if (virtio_net_hdr_to_skb(skb, vheader,
+			virtio_legacy_is_little_endian()) < 0)
+		return -1;
 	return 0;
 

Seems it'd be simpler to just

	return virtio_net_hdr_to_skb(...);

As far as I can tell, the function only returns -EINVAL or 0 (never a positive value), and the caller only cares about negative (error) or positive (no checksum needed), so that'd be equivalent?

johannes

Quoted 33 lines · Johannes Berg

Quoted 18 lines · Michael S. Tsirkin

virtio_net_hdr_to_skb() refuses packets whose checksum metadata it cannot make sense of, but the vector raw transport ignored its return value: rejected packets went on into the stack with their metadata unvalidated.

Propagate the failure.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 arch/um/drivers/vector_transports.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

arch/um/drivers/vector_transports.c +0 -0

index e1fb2a76fdff..8b7548458663 100644

arch/um/drivers/vector_transports.c static int raw_verify_header( · line 217

Quoted 9 lines

arch/um/drivers/vector_transports.c +3 -1

@@ -210,7 +210,9 @@ static int raw_verify_header(
 		return 1;
 
 	skb_set_network_header(skb, ETH_HLEN);
-	virtio_net_hdr_to_skb(skb, vheader, virtio_legacy_is_little_endian());
+	if (virtio_net_hdr_to_skb(skb, vheader,
+			virtio_legacy_is_little_endian()) < 0)
+		return -1;
 	return 0;
 

Seems it'd be simpler to just

	return virtio_net_hdr_to_skb(...);

Quoted 5 lines

As far as I can tell, the function only returns -EINVAL or 0 (never a positive value), and the caller only cares about negative (error) or positive (no checksum needed), so that'd be equivalent?

johannes

if raw_verify_header can safely return any errno - i did not want to audit it.

Quoted 38 lines · Michael S. Tsirkin

Quoted 37 lines · Johannes Berg

Quoted 18 lines · Michael S. Tsirkin

virtio_net_hdr_to_skb() refuses packets whose checksum metadata it cannot make sense of, but the vector raw transport ignored its return value: rejected packets went on into the stack with their metadata unvalidated.

Propagate the failure.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 arch/um/drivers/vector_transports.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

arch/um/drivers/vector_transports.c +0 -0

index e1fb2a76fdff..8b7548458663 100644

arch/um/drivers/vector_transports.c static int raw_verify_header( · line 217

Quoted 9 lines

arch/um/drivers/vector_transports.c +3 -1

@@ -210,7 +210,9 @@ static int raw_verify_header(
 		return 1;
 
 	skb_set_network_header(skb, ETH_HLEN);
-	virtio_net_hdr_to_skb(skb, vheader, virtio_legacy_is_little_endian());
+	if (virtio_net_hdr_to_skb(skb, vheader,
+			virtio_legacy_is_little_endian()) < 0)
+		return -1;
 	return 0;
 

Seems it'd be simpler to just

	return virtio_net_hdr_to_skb(...);

As far as I can tell, the function only returns -EINVAL or 0 (never a positive value), and the caller only cares about negative (error) or positive (no checksum needed), so that'd be equivalent?

Quoted 4 lines

Quoted 1 line

johannes

if raw_verify_header can safely return any errno - i did not want to audit it.

You already used an LLM and still didn't want to? ;-)

I checked briefly (by hand) and it seems fine, it's assigned to the function pointer, which only has two callers on the _kern.c side.

johannes

Quoted 34 lines · Michael S. Tsirkin

Quoted 33 lines · Johannes Berg

Quoted 18 lines · Michael S. Tsirkin

virtio_net_hdr_to_skb() refuses packets whose checksum metadata it cannot make sense of, but the vector raw transport ignored its return value: rejected packets went on into the stack with their metadata unvalidated.

Propagate the failure.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 arch/um/drivers/vector_transports.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

arch/um/drivers/vector_transports.c +0 -0

index e1fb2a76fdff..8b7548458663 100644

arch/um/drivers/vector_transports.c static int raw_verify_header( · line 217

Quoted 9 lines

arch/um/drivers/vector_transports.c +3 -1

@@ -210,7 +210,9 @@ static int raw_verify_header(
 		return 1;
 
 	skb_set_network_header(skb, ETH_HLEN);
-	virtio_net_hdr_to_skb(skb, vheader, virtio_legacy_is_little_endian());
+	if (virtio_net_hdr_to_skb(skb, vheader,
+			virtio_legacy_is_little_endian()) < 0)
+		return -1;
 	return 0;
 

Seems it'd be simpler to just

	return virtio_net_hdr_to_skb(...);

Quoted 8 lines

Quoted 5 lines

As far as I can tell, the function only returns -EINVAL or 0 (never a positive value), and the caller only cares about negative (error) or positive (no checksum needed), so that'd be equivalent?

johannes

if raw_verify_header can safely return any errno - i did not want to audit it.

You already used an LLM and still didn't want to? ;-)

I checked briefly (by hand) and it seems fine, it's assigned to the function pointer, which only has two callers on the _kern.c side.

johannes

Quoted 21 lines

Quoted 12 lines

Quoted 12 lines

arch/um/drivers/vector_transports.c static int raw_verify_header( · line 216

Quoted 7 lines

arch/um/drivers/vector_transports.c +3 -1

-	virtio_net_hdr_to_skb(skb, vheader, virtio_legacy_is_little_endian());
+	if (virtio_net_hdr_to_skb(skb, vheader,
+			virtio_legacy_is_little_endian()) < 0)
+		return -1;
 	return 0;

Seems it'd be simpler to just

	return virtio_net_hdr_to_skb(...);

Quoted 8 lines

Quoted 5 lines

As far as I can tell, the function only returns -EINVAL or 0 (never a positive value), and the caller only cares about negative (error) or positive (no checksum needed), so that'd be equivalent?

johannes

if raw_verify_header can safely return any errno - i did not want to audit it.

Quoted 6 lines

You already used an LLM and still didn't want to? ;-)

I checked briefly (by hand) and it seems fine, it's assigned to the function pointer, which only has two callers on the _kern.c side.

johannes

                               header_check = vp->verify_header(
                                        mmsg_vector->msg_hdr.msg_iov->iov_base,
                                        skb,
                                        vp
                                );
                                if (header_check < 0) {
                                /* Overlay header failed to verify - discard.
                                 * We can actually keep this skb and reuse it,
                                 * but that will make the prep logic too
                                 * complex.
                                 */
                                        dev_kfree_skb_irq(skb);
                                        vp->estats.rx_encaps_errors++;
                                        (*skbuff_vector) = NULL;
                                        mmsg_vector++;
                                        skbuff_vector++;
                                        continue;
                                }
                                if (header_check > 0) {
                                        vp->estats.rx_csum_offload_good++;
                                        skb->ip_summed = CHECKSUM_UNNECESSARY;
                                }

so any negative is fine, but if we return 1 then it will force to CHECKSUM_UNNECESSARY which I am not sure is what was intended?

Quoted 54 lines · Michael S. Tsirkin

Quoted 21 lines

Quoted 12 lines

Quoted 12 lines

arch/um/drivers/vector_transports.c static int raw_verify_header( · line 216

Quoted 7 lines

arch/um/drivers/vector_transports.c +3 -1

-	virtio_net_hdr_to_skb(skb, vheader, virtio_legacy_is_little_endian());
+	if (virtio_net_hdr_to_skb(skb, vheader,
+			virtio_legacy_is_little_endian()) < 0)
+		return -1;
 	return 0;

Seems it'd be simpler to just

	return virtio_net_hdr_to_skb(...);

Quoted 8 lines

Quoted 5 lines

As far as I can tell, the function only returns -EINVAL or 0 (never a positive value), and the caller only cares about negative (error) or positive (no checksum needed), so that'd be equivalent?

johannes

if raw_verify_header can safely return any errno - i did not want to audit it.

Quoted 6 lines

You already used an LLM and still didn't want to? ;-)

I checked briefly (by hand) and it seems fine, it's assigned to the function pointer, which only has two callers on the _kern.c side.

johannes

                               header_check = vp->verify_header(
                                        mmsg_vector->msg_hdr.msg_iov->iov_base,
                                        skb,
                                        vp
                                );
                                if (header_check < 0) {
                                /* Overlay header failed to verify - discard.
                                 * We can actually keep this skb and reuse it,
                                 * but that will make the prep logic too
                                 * complex.
                                 */
                                        dev_kfree_skb_irq(skb);
                                        vp->estats.rx_encaps_errors++;
                                        (*skbuff_vector) = NULL;
                                        mmsg_vector++;
                                        skbuff_vector++;
                                        continue;
                                }
                                if (header_check > 0) {
                                        vp->estats.rx_csum_offload_good++;
                                        skb->ip_summed = CHECKSUM_UNNECESSARY;
                                }

Quoted 3 lines

so any negative is fine, but if we return 1 then it will force to CHECKSUM_UNNECESSARY which I am not sure is what was intended?

Right, but virtio_net_hdr_to_skb() cannot return 1, only 0 on success and -ERROR on failure?

johannes

Quoted 3 lines

Quoted 1 line

Right, but virtio_net_hdr_to_skb() cannot return 1, only 0 on success and -ERROR on failure?

After this patchset, it can, to signal slow path software checksum. I'm changing callers first so that changing the function itself does not break bisect. I should mention this in the commit log.

Quoted 4 lines · Michael S. Tsirkin

Quoted 3 lines

Quoted 1 line

Right, but virtio_net_hdr_to_skb() cannot return 1, only 0 on success and -ERROR on failure?

Quoted 3 lines

After this patchset, it can, to signal slow path software checksum. I'm changing callers first so that changing the function itself does not break bisect. I should mention this in the commit log.

Oh, OK, that I clearly missed, but also was only CC'ed on patches 2 and 3 :)

johannes

virtio_net_hdr_to_skb() refuses packets whose checksum metadata it cannot make sense of, but the vector raw transport ignored its return value: rejected packets went on into the stack with their metadata unvalidated.

Propagate the failure.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 arch/um/drivers/vector_transports.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

arch/um/drivers/vector_transports.c +0 -0

index e1fb2a76fdff..8b7548458663 100644

Quoted 18 lines · Michael S. Tsirkin

virtio_net_hdr_to_skb() refuses packets whose checksum metadata it cannot make sense of, but the vector raw transport ignored its return value: rejected packets went on into the stack with their metadata unvalidated.

Propagate the failure.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 arch/um/drivers/vector_transports.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

arch/um/drivers/vector_transports.c +0 -0

index e1fb2a76fdff..8b7548458663 100644

arch/um/drivers/vector_transports.c static int raw_verify_header( · line 215

Quoted 7 lines

arch/um/drivers/vector_transports.c +3 -1

@@ -210,7 +210,9 @@ static int raw_verify_header(
 		return 1;
 
 	skb_set_network_header(skb, ETH_HLEN);
-	virtio_net_hdr_to_skb(skb, vheader, virtio_legacy_is_little_endian());
+	if (virtio_net_hdr_to_skb(skb, vheader,
+			virtio_legacy_is_little_endian()) < 0)
+		return -1;

Should this return 1, like the above?

Not sure whether the verify_header return value is well defined beyond just boolean.

As reported first by Paulos Yibelo <[email protected]> (after which others, credited below, found more ways the bug can affect users) a virtio net header can place CHECKSUM_PARTIAL at any in-packet offset. If that state is retained, later checksum completion can modify the packet after protocol processing has made decisions from it.

Let's fix that: let's validate csum_start and csum_offset and only keep CHECKSUM_PARTIAL when csum_start and csum_offset match a parsed TCP or UDP header, while handling VLAN headers, IPv4 options and IPv6 extension headers. Complete other non-GSO checksums immediately, before any protocol processing runs.

We can't easily "complete the checksum" for GSO we don't recognize - GSO assumes knowledge of the protocol. We could segment ignoring the checksum then do whatever csum_start and csum_offset say, but that seems like an overkill for something that shouldn't happen.

There's some risk here:
- non-gso packets which can't be parsed get checksum calculated in software, adding performance overhead
- gso packets which can't be parsed get dropped

To mitigate the risks, the following patch will add a sysctl knob to bypass the resolution. This is reasonable if the setup is such that there is no protocol processing at all.

UDP tunnel headers get the same handling on the inner header.

Packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are unaffected.

Assisted-by LLM

Reported-by Paulos Yibelo <[email protected]>

Reported-by Chris Arges <[email protected]>

Tested-by Chris Arges <[email protected]>

Fixes f43798c27684 ("tun: Allow GSO using virtio_net_hdr")

Suggested-by Willem de Bruijn <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 include/linux/virtio_net.h | 251 +++++++++++++++++++++++++++++++++++--
 1 file changed, 241 insertions(+), 10 deletions(-)

include/linux/virtio_net.h +4 -0

index d6466f96cdd0..357115ff0ec2 100644
@@ -5,7 +5,11 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/netdevice.h>
 #include <linux/udp.h>
+#include <net/gso.h>
+#include <net/ip.h>
+#include <net/ipv6.h>
 #include <net/tcp.h>
 #include <uapi/linux/tcp.h>
 #include <uapi/linux/virtio_net.h>

@@ -48,6 +52,171 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
 	return 0;
 }

include/linux/virtio_net.h +16 -0

+static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb,
+						  unsigned int nhoff,
+						  unsigned int *thoff)
+{
+	struct iphdr *iph;
+
+	if (!pskb_may_pull(skb, nhoff + sizeof(struct iphdr)))
+		return NULL;
+
+	iph = (struct iphdr *)(skb->data + nhoff);
+	if (iph->version != 4 ||
+	    iph->ihl < sizeof(struct iphdr) / sizeof(u32))
+		return NULL;
+
+	*thoff = nhoff + iph->ihl * sizeof(u32);
+	return iph;

include/linux/virtio_net.h +51 -3

+		*proto = skb->protocol;
+	}
+	return true;
+}
+
 
 
 /* This function must be called after virtio_net_hdr_from_skb(). */

@@ -282,6 +470,35 @@ static inline unsigned int virtio_l3min(bool is_ipv6)
 	return is_ipv6 ? sizeof(struct ipv6hdr) : sizeof(struct iphdr);
 }
 
+/* Do the tunnel offsets match the outer headers in the frame?
+ * The L3 header the tunnel claims must be the one the frame
+ * carries, with UDP following it at @outer_th.
+ */
+static inline bool
+virtio_net_tnl_offsets_ok(struct sk_buff *skb, bool outer_isv6,
+			  unsigned int outer_th)
+{
+	int nhoff;
+	__be16 proto;
+	unsigned int thoff;
+	int ip_proto;
+
+	if (!virtio_net_l3_start(skb, &nhoff, &proto))
+		return false;
+	if (outer_isv6 != (proto == htons(ETH_P_IPV6)))
+		return false;
+	if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
+		return false;
+
+	return ip_proto == IPPROTO_UDP && outer_th == thoff;
+}
+
+/*
+ * Return value:
+ * 0 - success
+ * >0 - checksum was completed in software (slow path)
+ * <0 - failure
+ */
 static inline int
 virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 			  const struct virtio_net_hdr_v1_hash_tunnel *vhdr,

@@ -293,6 +510,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	unsigned int inner_nh, outer_th, inner_th;
 	unsigned int inner_l3min, outer_l3min;
 	u8 gso_inner_type, gso_tunnel_type;
+	__be16 inner_proto;
 	bool outer_isv6, inner_isv6;
 	int ret;
 

@@ -332,10 +550,13 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	    inner_th < inner_nh + inner_l3min)
 		return -EINVAL;
 
+	if (!virtio_net_tnl_offsets_ok(skb, outer_isv6, outer_th))
+		return -EINVAL;
+
 	/* Let the basic parsing deal with plain GSO features. */
 	ret = __virtio_net_hdr_to_skb(skb, hdr, true,
 				      hdr->gso_type & ~gso_tunnel_type);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	/* In case of USO, the inner protocol is still unknown and

@@ -353,7 +574,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	}
 
 	skb_set_inner_protocol(skb, inner_isv6 ? htons(ETH_P_IPV6) :
-						 htons(ETH_P_IP));
+			    htons(ETH_P_IP));
 	if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) {
 		if (!tnl_csum_negotiated)
 			return -EINVAL;

@@ -368,7 +589,17 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	skb->inner_mac_header = inner_nh + skb_headroom(skb);
 	skb->transport_header = outer_th + skb_headroom(skb);
 	skb->encapsulation = 1;
-	return 0;
+
+	/*
+	 * We know VIRTIO_NET_HDR_F_NEEDS_CSUM was set so __virtio_net_hdr_to_skb
+	 * set CHECKSUM_PARTIAL.
+	 */
+	inner_proto = inner_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP);
+	if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
+						 inner_proto))
+		return 0;
+
+	return virtio_net_resolve_csum_partial_mismatch(skb);
 }
 
 /* Checksum-related fields validation for the driver */

Signature · 1 line

MST

Quoted 60 lines · Michael S. Tsirkin

As reported first by Paulos Yibelo <[email protected]> (after which others, credited below, found more ways the bug can affect users) a virtio net header can place CHECKSUM_PARTIAL at any in-packet offset. If that state is retained, later checksum completion can modify the packet after protocol processing has made decisions from it.

Let's fix that: let's validate csum_start and csum_offset and only keep CHECKSUM_PARTIAL when csum_start and csum_offset match a parsed TCP or UDP header, while handling VLAN headers, IPv4 options and IPv6 extension headers. Complete other non-GSO checksums immediately, before any protocol processing runs.

We can't easily "complete the checksum" for GSO we don't recognize - GSO assumes knowledge of the protocol. We could segment ignoring the checksum then do whatever csum_start and csum_offset say, but that seems like an overkill for something that shouldn't happen.

There's some risk here:
- non-gso packets which can't be parsed get checksum calculated in software, adding performance overhead
- gso packets which can't be parsed get dropped

To mitigate the risks, the following patch will add a sysctl knob to bypass the resolution. This is reasonable if the setup is such that there is no protocol processing at all.

UDP tunnel headers get the same handling on the inner header.

Packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are unaffected.

Assisted-by LLM

Reported-by Paulos Yibelo <[email protected]>

Reported-by Chris Arges <[email protected]>

Tested-by Chris Arges <[email protected]>

Fixes f43798c27684 ("tun: Allow GSO using virtio_net_hdr")

Suggested-by Willem de Bruijn <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 include/linux/virtio_net.h | 251 +++++++++++++++++++++++++++++++++++--
 1 file changed, 241 insertions(+), 10 deletions(-)

include/linux/virtio_net.h +4 -0

index d6466f96cdd0..357115ff0ec2 100644
@@ -5,7 +5,11 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/netdevice.h>
 #include <linux/udp.h>
+#include <net/gso.h>
+#include <net/ip.h>
+#include <net/ipv6.h>
 #include <net/tcp.h>
 #include <uapi/linux/tcp.h>
 #include <uapi/linux/virtio_net.h>

@@ -48,6 +52,171 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
 	return 0;
 }

include/linux/virtio_net.h static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, · line 63

Quoted 10 lines

include/linux/virtio_net.h +9 -0

 
+/* TODO: nothing virtio specific here, move to ip.h */
+
+/*
+ * Pull and minimally validate the IPv4 header at @nhoff.
+ * Returns the header and stores the transport header offset in
+ * @thoff, or NULL if truncated or malformed.  Only the fixed header
+ * is guaranteed linear; callers that need the options or the
+ * transport header must pull further.
+ */

nit: comment basically describes the code (what), which is quite self explanatory. I'd drop. Or else consider converting to kdoc fmt.

Quoted 16 lines

include/linux/virtio_net.h +16 -0

+static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb,
+						  unsigned int nhoff,
+						  unsigned int *thoff)
+{
+	struct iphdr *iph;
+
+	if (!pskb_may_pull(skb, nhoff + sizeof(struct iphdr)))
+		return NULL;
+
+	iph = (struct iphdr *)(skb->data + nhoff);
+	if (iph->version != 4 ||
+	    iph->ihl < sizeof(struct iphdr) / sizeof(u32))
+		return NULL;
+
+	*thoff = nhoff + iph->ihl * sizeof(u32);
+	return iph;

include/linux/virtio_net.h static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, · line 89

Quoted 10 lines

include/linux/virtio_net.h +10 -0

+}
+
+/* Where does the L3 header start, and what protocol is it?
+ * Handles Ethernet and VLAN headers; other devices use the
+ * network header offset the caller set up.
+ */
+static inline bool
+virtio_net_l3_start(struct sk_buff *skb, int *nhoff, __be16 *proto)
+{
+	if (skb->dev->type == ARPHRD_ETHER) {

Only since recent commit 44378d02c5aa ("net: always dissect GSO packets in __virtio_net_hdr_to_skb()") can skb->dev be relied on to be non-zero.

We'll have to be careful that this does not get backported to stable without that.

Perhaps an explicit precondition test and error return (and DEBUG_NET_WARN_ON_ONCE) if a NULL skb->dev is encountered.

OTOH, it results in a hard crash so will be discoverd fast without that too.

include/linux/virtio_net.h static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, · line 93

Quoted 4 lines

include/linux/virtio_net.h +4 -0

+		if (!pskb_may_pull(skb, ETH_HLEN))
+			return false;
+		*nhoff = ETH_HLEN;
+		*proto = eth_hdr(skb)->h_proto;

When called from virtio_net_hdr_tnl_to_skb, according to my LLM mac header may not be set yet. Have not verified yet.

skb_reset_mac_header(skb) is called inside __virtio_net_hdr_to_skb, which is then not called yet.

include/linux/virtio_net.h static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, · line 100

Quoted 7 lines

include/linux/virtio_net.h +7 -0

+		if (eth_type_vlan(*proto)) {
+			*proto = __vlan_get_protocol(skb, *proto, nhoff);
+			if (!*proto)
+				return false;
+		}
+	} else {
+		*nhoff = skb_network_offset(skb);

The same is true for skb->network_header. Since an uninitialized/zero value there causes more subtle issues, here an explicit check may be more valuable (even) than for skb->dev.

Quoted 98 lines

include/linux/virtio_net.h +51 -3

+		*proto = skb->protocol;
+	}
+	return true;
+}
+
 
 
 /* This function must be called after virtio_net_hdr_from_skb(). */

@@ -282,6 +470,35 @@ static inline unsigned int virtio_l3min(bool is_ipv6)
 	return is_ipv6 ? sizeof(struct ipv6hdr) : sizeof(struct iphdr);
 }
 
+/* Do the tunnel offsets match the outer headers in the frame?
+ * The L3 header the tunnel claims must be the one the frame
+ * carries, with UDP following it at @outer_th.
+ */
+static inline bool
+virtio_net_tnl_offsets_ok(struct sk_buff *skb, bool outer_isv6,
+			  unsigned int outer_th)
+{
+	int nhoff;
+	__be16 proto;
+	unsigned int thoff;
+	int ip_proto;
+
+	if (!virtio_net_l3_start(skb, &nhoff, &proto))
+		return false;
+	if (outer_isv6 != (proto == htons(ETH_P_IPV6)))
+		return false;
+	if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
+		return false;
+
+	return ip_proto == IPPROTO_UDP && outer_th == thoff;
+}
+
+/*
+ * Return value:
+ * 0 - success
+ * >0 - checksum was completed in software (slow path)
+ * <0 - failure
+ */
 static inline int
 virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 			  const struct virtio_net_hdr_v1_hash_tunnel *vhdr,

@@ -293,6 +510,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	unsigned int inner_nh, outer_th, inner_th;
 	unsigned int inner_l3min, outer_l3min;
 	u8 gso_inner_type, gso_tunnel_type;
+	__be16 inner_proto;
 	bool outer_isv6, inner_isv6;
 	int ret;
 

@@ -332,10 +550,13 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	    inner_th < inner_nh + inner_l3min)
 		return -EINVAL;
 
+	if (!virtio_net_tnl_offsets_ok(skb, outer_isv6, outer_th))
+		return -EINVAL;
+
 	/* Let the basic parsing deal with plain GSO features. */
 	ret = __virtio_net_hdr_to_skb(skb, hdr, true,
 				      hdr->gso_type & ~gso_tunnel_type);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	/* In case of USO, the inner protocol is still unknown and

@@ -353,7 +574,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	}
 
 	skb_set_inner_protocol(skb, inner_isv6 ? htons(ETH_P_IPV6) :
-						 htons(ETH_P_IP));
+			    htons(ETH_P_IP));
 	if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) {
 		if (!tnl_csum_negotiated)
 			return -EINVAL;

@@ -368,7 +589,17 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	skb->inner_mac_header = inner_nh + skb_headroom(skb);
 	skb->transport_header = outer_th + skb_headroom(skb);
 	skb->encapsulation = 1;
-	return 0;
+
+	/*
+	 * We know VIRTIO_NET_HDR_F_NEEDS_CSUM was set so __virtio_net_hdr_to_skb
+	 * set CHECKSUM_PARTIAL.
+	 */
+	inner_proto = inner_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP);
+	if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
+						 inner_proto))
+		return 0;
+
+	return virtio_net_resolve_csum_partial_mismatch(skb);
 }
 
 /* Checksum-related fields validation for the driver */

Signature · 1 line

MST

Quoted 124 lines · Willem de Bruijn

Quoted 60 lines · Michael S. Tsirkin

As reported first by Paulos Yibelo <[email protected]> (after which others, credited below, found more ways the bug can affect users) a virtio net header can place CHECKSUM_PARTIAL at any in-packet offset. If that state is retained, later checksum completion can modify the packet after protocol processing has made decisions from it.

Let's fix that: let's validate csum_start and csum_offset and only keep CHECKSUM_PARTIAL when csum_start and csum_offset match a parsed TCP or UDP header, while handling VLAN headers, IPv4 options and IPv6 extension headers. Complete other non-GSO checksums immediately, before any protocol processing runs.

We can't easily "complete the checksum" for GSO we don't recognize - GSO assumes knowledge of the protocol. We could segment ignoring the checksum then do whatever csum_start and csum_offset say, but that seems like an overkill for something that shouldn't happen.

There's some risk here:
- non-gso packets which can't be parsed get checksum calculated in software, adding performance overhead
- gso packets which can't be parsed get dropped

To mitigate the risks, the following patch will add a sysctl knob to bypass the resolution. This is reasonable if the setup is such that there is no protocol processing at all.

UDP tunnel headers get the same handling on the inner header.

Packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are unaffected.

Assisted-by LLM

Reported-by Paulos Yibelo <[email protected]>

Reported-by Chris Arges <[email protected]>

Tested-by Chris Arges <[email protected]>

Fixes f43798c27684 ("tun: Allow GSO using virtio_net_hdr")

Suggested-by Willem de Bruijn <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 include/linux/virtio_net.h | 251 +++++++++++++++++++++++++++++++++++--
 1 file changed, 241 insertions(+), 10 deletions(-)

include/linux/virtio_net.h +4 -0

index d6466f96cdd0..357115ff0ec2 100644
@@ -5,7 +5,11 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/netdevice.h>
 #include <linux/udp.h>
+#include <net/gso.h>
+#include <net/ip.h>
+#include <net/ipv6.h>
 #include <net/tcp.h>
 #include <uapi/linux/tcp.h>
 #include <uapi/linux/virtio_net.h>

@@ -48,6 +52,171 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
 	return 0;
 }

include/linux/virtio_net.h static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, · line 63

Quoted 10 lines

include/linux/virtio_net.h +9 -0

 
+/* TODO: nothing virtio specific here, move to ip.h */
+
+/*
+ * Pull and minimally validate the IPv4 header at @nhoff.
+ * Returns the header and stores the transport header offset in
+ * @thoff, or NULL if truncated or malformed.  Only the fixed header
+ * is guaranteed linear; callers that need the options or the
+ * transport header must pull further.
+ */

nit: comment basically describes the code (what), which is quite self explanatory. I'd drop. Or else consider converting to kdoc fmt.

Quoted 16 lines

include/linux/virtio_net.h +16 -0

+static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb,
+						  unsigned int nhoff,
+						  unsigned int *thoff)
+{
+	struct iphdr *iph;
+
+	if (!pskb_may_pull(skb, nhoff + sizeof(struct iphdr)))
+		return NULL;
+
+	iph = (struct iphdr *)(skb->data + nhoff);
+	if (iph->version != 4 ||
+	    iph->ihl < sizeof(struct iphdr) / sizeof(u32))
+		return NULL;
+
+	*thoff = nhoff + iph->ihl * sizeof(u32);
+	return iph;

include/linux/virtio_net.h static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, · line 89

Quoted 10 lines

include/linux/virtio_net.h +10 -0

+}
+
+/* Where does the L3 header start, and what protocol is it?
+ * Handles Ethernet and VLAN headers; other devices use the
+ * network header offset the caller set up.
+ */
+static inline bool
+virtio_net_l3_start(struct sk_buff *skb, int *nhoff, __be16 *proto)
+{
+	if (skb->dev->type == ARPHRD_ETHER) {

Only since recent commit 44378d02c5aa ("net: always dissect GSO packets in __virtio_net_hdr_to_skb()") can skb->dev be relied on to be non-zero.

We'll have to be careful that this does not get backported to stable without that.

Perhaps an explicit precondition test and error return (and DEBUG_NET_WARN_ON_ONCE) if a NULL skb->dev is encountered.

OTOH, it results in a hard crash so will be discoverd fast without that too.

include/linux/virtio_net.h static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, · line 93

Quoted 4 lines

include/linux/virtio_net.h +4 -0

+		if (!pskb_may_pull(skb, ETH_HLEN))
+			return false;
+		*nhoff = ETH_HLEN;
+		*proto = eth_hdr(skb)->h_proto;

When called from virtio_net_hdr_tnl_to_skb, according to my LLM mac header may not be set yet. Have not verified yet.

Quoted 3 lines

skb_reset_mac_header(skb) is called inside __virtio_net_hdr_to_skb, which is then not called yet.

I am able to confirm this in my testing. I have KASAN enabled, and it triggered when testing leading me to this change.

Adding skb_reset_mac_header(skb); before calling virtio_net_tnl_offsets_ok mitigates this issue.

--chris

Quoted 113 lines

include/linux/virtio_net.h static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, · line 100

Quoted 8 lines

include/linux/virtio_net.h +7 -0

+		if (eth_type_vlan(*proto)) {
+			*proto = __vlan_get_protocol(skb, *proto, nhoff);
+			if (!*proto)
+				return false;
+		}
+	} else {
+		*nhoff = skb_network_offset(skb);

The same is true for skb->network_header. Since an uninitialized/zero value there causes more subtle issues, here an explicit check may be more valuable (even) than for skb->dev.

Quoted 98 lines

include/linux/virtio_net.h +51 -3

+		*proto = skb->protocol;
+	}
+	return true;
+}
+
 
 
 /* This function must be called after virtio_net_hdr_from_skb(). */

@@ -282,6 +470,35 @@ static inline unsigned int virtio_l3min(bool is_ipv6)
 	return is_ipv6 ? sizeof(struct ipv6hdr) : sizeof(struct iphdr);
 }
 
+/* Do the tunnel offsets match the outer headers in the frame?
+ * The L3 header the tunnel claims must be the one the frame
+ * carries, with UDP following it at @outer_th.
+ */
+static inline bool
+virtio_net_tnl_offsets_ok(struct sk_buff *skb, bool outer_isv6,
+			  unsigned int outer_th)
+{
+	int nhoff;
+	__be16 proto;
+	unsigned int thoff;
+	int ip_proto;
+
+	if (!virtio_net_l3_start(skb, &nhoff, &proto))
+		return false;
+	if (outer_isv6 != (proto == htons(ETH_P_IPV6)))
+		return false;
+	if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
+		return false;
+
+	return ip_proto == IPPROTO_UDP && outer_th == thoff;
+}
+
+/*
+ * Return value:
+ * 0 - success
+ * >0 - checksum was completed in software (slow path)
+ * <0 - failure
+ */
 static inline int
 virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 			  const struct virtio_net_hdr_v1_hash_tunnel *vhdr,

@@ -293,6 +510,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	unsigned int inner_nh, outer_th, inner_th;
 	unsigned int inner_l3min, outer_l3min;
 	u8 gso_inner_type, gso_tunnel_type;
+	__be16 inner_proto;
 	bool outer_isv6, inner_isv6;
 	int ret;
 

@@ -332,10 +550,13 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	    inner_th < inner_nh + inner_l3min)
 		return -EINVAL;
 
+	if (!virtio_net_tnl_offsets_ok(skb, outer_isv6, outer_th))
+		return -EINVAL;
+
 	/* Let the basic parsing deal with plain GSO features. */
 	ret = __virtio_net_hdr_to_skb(skb, hdr, true,
 				      hdr->gso_type & ~gso_tunnel_type);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	/* In case of USO, the inner protocol is still unknown and

@@ -353,7 +574,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	}
 
 	skb_set_inner_protocol(skb, inner_isv6 ? htons(ETH_P_IPV6) :
-						 htons(ETH_P_IP));
+			    htons(ETH_P_IP));
 	if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) {
 		if (!tnl_csum_negotiated)
 			return -EINVAL;

@@ -368,7 +589,17 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	skb->inner_mac_header = inner_nh + skb_headroom(skb);
 	skb->transport_header = outer_th + skb_headroom(skb);
 	skb->encapsulation = 1;
-	return 0;
+
+	/*
+	 * We know VIRTIO_NET_HDR_F_NEEDS_CSUM was set so __virtio_net_hdr_to_skb
+	 * set CHECKSUM_PARTIAL.
+	 */
+	inner_proto = inner_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP);
+	if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
+						 inner_proto))
+		return 0;
+
+	return virtio_net_resolve_csum_partial_mismatch(skb);
 }
 
 /* Checksum-related fields validation for the driver */

Signature · 1 line

MST

Quoted 60 lines · Michael S. Tsirkin

As reported first by Paulos Yibelo <[email protected]> (after which others, credited below, found more ways the bug can affect users) a virtio net header can place CHECKSUM_PARTIAL at any in-packet offset. If that state is retained, later checksum completion can modify the packet after protocol processing has made decisions from it.

Let's fix that: let's validate csum_start and csum_offset and only keep CHECKSUM_PARTIAL when csum_start and csum_offset match a parsed TCP or UDP header, while handling VLAN headers, IPv4 options and IPv6 extension headers. Complete other non-GSO checksums immediately, before any protocol processing runs.

We can't easily "complete the checksum" for GSO we don't recognize - GSO assumes knowledge of the protocol. We could segment ignoring the checksum then do whatever csum_start and csum_offset say, but that seems like an overkill for something that shouldn't happen.

There's some risk here:
- non-gso packets which can't be parsed get checksum calculated in software, adding performance overhead
- gso packets which can't be parsed get dropped

To mitigate the risks, the following patch will add a sysctl knob to bypass the resolution. This is reasonable if the setup is such that there is no protocol processing at all.

UDP tunnel headers get the same handling on the inner header.

Packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are unaffected.

Assisted-by LLM

Reported-by Paulos Yibelo <[email protected]>

Reported-by Chris Arges <[email protected]>

Tested-by Chris Arges <[email protected]>

Fixes f43798c27684 ("tun: Allow GSO using virtio_net_hdr")

Suggested-by Willem de Bruijn <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 include/linux/virtio_net.h | 251 +++++++++++++++++++++++++++++++++++--
 1 file changed, 241 insertions(+), 10 deletions(-)

include/linux/virtio_net.h +4 -0

index d6466f96cdd0..357115ff0ec2 100644
@@ -5,7 +5,11 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/netdevice.h>
 #include <linux/udp.h>
+#include <net/gso.h>
+#include <net/ip.h>
+#include <net/ipv6.h>
 #include <net/tcp.h>
 #include <uapi/linux/tcp.h>
 #include <uapi/linux/virtio_net.h>

@@ -48,6 +52,171 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
 	return 0;
 }

include/linux/virtio_net.h static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, · line 63

Quoted 10 lines

include/linux/virtio_net.h +9 -0

 
+/* TODO: nothing virtio specific here, move to ip.h */
+
+/*
+ * Pull and minimally validate the IPv4 header at @nhoff.
+ * Returns the header and stores the transport header offset in
+ * @thoff, or NULL if truncated or malformed.  Only the fixed header
+ * is guaranteed linear; callers that need the options or the
+ * transport header must pull further.
+ */

nit: comment basically describes the code (what), which is quite self explanatory. I'd drop. Or else consider converting to kdoc fmt.

Quoted 22 lines

include/linux/virtio_net.h +22 -0

+static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb,
+						  unsigned int nhoff,
+						  unsigned int *thoff)
+{
+	struct iphdr *iph;
+
+	if (!pskb_may_pull(skb, nhoff + sizeof(struct iphdr)))
+		return NULL;
+
+	iph = (struct iphdr *)(skb->data + nhoff);
+	if (iph->version != 4 ||
+	    iph->ihl < sizeof(struct iphdr) / sizeof(u32))
+		return NULL;
+
+	*thoff = nhoff + iph->ihl * sizeof(u32);
+	return iph;
+}
+
+/* Where does the L3 header start, and what protocol is it?
+ * Handles Ethernet and VLAN headers; other devices use the
+ * network header offset the caller set up.
+ */

include/linux/virtio_net.h static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, · line 89

Quoted 4 lines

include/linux/virtio_net.h +4 -0

+static inline bool
+virtio_net_l3_start(struct sk_buff *skb, int *nhoff, __be16 *proto)
+{
+	if (skb->dev->type == ARPHRD_ETHER) {

Only since recent commit 44378d02c5aa ("net: always dissect GSO packets in __virtio_net_hdr_to_skb()") can skb->dev be relied on to be non-zero.

We'll have to be careful that this does not get backported to stable without that.

Perhaps an explicit precondition test and error return (and DEBUG_NET_WARN_ON_ONCE) if a NULL skb->dev is encountered.

OTOH, it results in a hard crash so will be discoverd fast without that too.

include/linux/virtio_net.h static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, · line 93

Quoted 4 lines

include/linux/virtio_net.h +4 -0

+		if (!pskb_may_pull(skb, ETH_HLEN))
+			return false;
+		*nhoff = ETH_HLEN;
+		*proto = eth_hdr(skb)->h_proto;

When called from virtio_net_hdr_tnl_to_skb, according to my LLM mac header may not be set yet. Have not verified yet.

skb_reset_mac_header(skb) is called inside __virtio_net_hdr_to_skb, which is then not called yet.

include/linux/virtio_net.h static inline int virtio_net_hdr_set_proto(struct sk_buff *skb, · line 100

Quoted 7 lines

include/linux/virtio_net.h +7 -0

+		if (eth_type_vlan(*proto)) {
+			*proto = __vlan_get_protocol(skb, *proto, nhoff);
+			if (!*proto)
+				return false;
+		}
+	} else {
+		*nhoff = skb_network_offset(skb);

The same is true for skb->network_header. Since an uninitialized/zero value there causes more subtle issues, here an explicit check may be more valuable (even) than for skb->dev.

Quoted 98 lines

include/linux/virtio_net.h +51 -3

+		*proto = skb->protocol;
+	}
+	return true;
+}
+
 
 
 /* This function must be called after virtio_net_hdr_from_skb(). */

@@ -282,6 +470,35 @@ static inline unsigned int virtio_l3min(bool is_ipv6)
 	return is_ipv6 ? sizeof(struct ipv6hdr) : sizeof(struct iphdr);
 }
 
+/* Do the tunnel offsets match the outer headers in the frame?
+ * The L3 header the tunnel claims must be the one the frame
+ * carries, with UDP following it at @outer_th.
+ */
+static inline bool
+virtio_net_tnl_offsets_ok(struct sk_buff *skb, bool outer_isv6,
+			  unsigned int outer_th)
+{
+	int nhoff;
+	__be16 proto;
+	unsigned int thoff;
+	int ip_proto;
+
+	if (!virtio_net_l3_start(skb, &nhoff, &proto))
+		return false;
+	if (outer_isv6 != (proto == htons(ETH_P_IPV6)))
+		return false;
+	if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
+		return false;
+
+	return ip_proto == IPPROTO_UDP && outer_th == thoff;
+}
+
+/*
+ * Return value:
+ * 0 - success
+ * >0 - checksum was completed in software (slow path)
+ * <0 - failure
+ */
 static inline int
 virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 			  const struct virtio_net_hdr_v1_hash_tunnel *vhdr,

@@ -293,6 +510,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	unsigned int inner_nh, outer_th, inner_th;
 	unsigned int inner_l3min, outer_l3min;
 	u8 gso_inner_type, gso_tunnel_type;
+	__be16 inner_proto;
 	bool outer_isv6, inner_isv6;
 	int ret;
 

@@ -332,10 +550,13 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	    inner_th < inner_nh + inner_l3min)
 		return -EINVAL;
 
+	if (!virtio_net_tnl_offsets_ok(skb, outer_isv6, outer_th))
+		return -EINVAL;
+
 	/* Let the basic parsing deal with plain GSO features. */
 	ret = __virtio_net_hdr_to_skb(skb, hdr, true,
 				      hdr->gso_type & ~gso_tunnel_type);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	/* In case of USO, the inner protocol is still unknown and

@@ -353,7 +574,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	}
 
 	skb_set_inner_protocol(skb, inner_isv6 ? htons(ETH_P_IPV6) :
-						 htons(ETH_P_IP));
+			    htons(ETH_P_IP));
 	if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) {
 		if (!tnl_csum_negotiated)
 			return -EINVAL;

@@ -368,7 +589,17 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	skb->inner_mac_header = inner_nh + skb_headroom(skb);
 	skb->transport_header = outer_th + skb_headroom(skb);
 	skb->encapsulation = 1;
-	return 0;
+
+	/*
+	 * We know VIRTIO_NET_HDR_F_NEEDS_CSUM was set so __virtio_net_hdr_to_skb
+	 * set CHECKSUM_PARTIAL.
+	 */
+	inner_proto = inner_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP);
+	if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
+						 inner_proto))
+		return 0;
+
+	return virtio_net_resolve_csum_partial_mismatch(skb);
 }
 
 /* Checksum-related fields validation for the driver */

Signature · 1 line

MST

As reported first by Paulos Yibelo <[email protected]> (after which others, credited below, found more ways the bug can affect users) a virtio net header can place CHECKSUM_PARTIAL at any in-packet offset. If that state is retained, later checksum completion can modify the packet after protocol processing has made decisions from it.

Let's fix that: let's validate csum_start and csum_offset and only keep CHECKSUM_PARTIAL when csum_start and csum_offset match a parsed TCP or UDP header, while handling VLAN headers, IPv4 options and IPv6 extension headers. Complete other non-GSO checksums immediately, before any protocol processing runs.

We can't easily "complete the checksum" for GSO we don't recognize - GSO assumes knowledge of the protocol. We could segment ignoring the checksum then do whatever csum_start and csum_offset say, but that seems like an overkill for something that shouldn't happen.

There's some risk here:
- non-gso packets which can't be parsed get checksum calculated in software, adding performance overhead
- gso packets which can't be parsed get dropped

To mitigate the risks, the following patch will add a sysctl knob to bypass the resolution. This is reasonable if the setup is such that there is no protocol processing at all.

UDP tunnel headers get the same handling on the inner header.

Packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are unaffected.

Assisted-by LLM

Reported-by Paulos Yibelo <[email protected]>

Reported-by Chris Arges <[email protected]>

Tested-by Chris Arges <[email protected]>

Fixes f43798c27684 ("tun: Allow GSO using virtio_net_hdr")

Suggested-by Willem de Bruijn <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 include/linux/virtio_net.h | 251 +++++++++++++++++++++++++++++++++++--
 1 file changed, 241 insertions(+), 10 deletions(-)

include/linux/virtio_net.h +4 -0

index d6466f96cdd0..357115ff0ec2 100644
@@ -5,7 +5,11 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/netdevice.h>
 #include <linux/udp.h>
+#include <net/gso.h>
+#include <net/ip.h>
+#include <net/ipv6.h>
 #include <net/tcp.h>
 #include <uapi/linux/tcp.h>
 #include <uapi/linux/virtio_net.h>

static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,

include/linux/virtio_net.h +0 -2

 					  const struct virtio_net_hdr *hdr,
 					  bool little_endian, u8 hdr_gso_type)

@@ -158,8 +327,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
 		case SKB_GSO_UDP_L4:
 			if (!(hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM))
 				return -EINVAL;
-			if (skb->csum_offset != offsetof(struct udphdr, check))
-				return -EINVAL;
 			if (skb->len - p_off > gso_size * UDP_MAX_SEGMENTS)
 				return -EINVAL;
 			if (gso_type != SKB_GSO_UDP_L4)

As reported first by Paulos Yibelo <[email protected]> (after which others, credited below, found more ways the bug can affect users) a virtio net header can place CHECKSUM_PARTIAL at any in-packet offset. If that state is retained, later checksum completion can modify the packet after protocol processing has made decisions from it.

Let's fix that: let's validate csum_start and csum_offset and only keep CHECKSUM_PARTIAL when csum_start and csum_offset match a parsed TCP or UDP header, while handling VLAN headers, IPv4 options and IPv6 extension headers. Complete other non-GSO checksums immediately, before any protocol processing runs.

We can't easily "complete the checksum" for GSO we don't recognize - GSO assumes knowledge of the protocol. We could segment ignoring the checksum then do whatever csum_start and csum_offset say, but that seems like an overkill for something that shouldn't happen.

There's some risk here:
- non-gso packets which can't be parsed get checksum calculated in software, adding performance overhead
- gso packets which can't be parsed get dropped

As reported first by Paulos Yibelo <[email protected]> (after which others, credited below, found more ways the bug can affect users) a virtio net header can place CHECKSUM_PARTIAL at any in-packet offset. If that state is retained, later checksum completion can modify the packet after protocol processing has made decisions from it.

Let's fix that: let's validate csum_start and csum_offset and only keep CHECKSUM_PARTIAL when csum_start and csum_offset match a parsed TCP or UDP header, while handling VLAN headers, IPv4 options and IPv6 extension headers. Complete other non-GSO checksums immediately, before any protocol processing runs.

We can't easily "complete the checksum" for GSO we don't recognize - GSO assumes knowledge of the protocol. We could segment ignoring the checksum then do whatever csum_start and csum_offset say, but that seems like an overkill for something that shouldn't happen.

There's some risk here:
- non-gso packets which can't be parsed get checksum calculated in software, adding performance overhead
- gso packets which can't be parsed get dropped

To mitigate the risks, the following patch will add a sysctl knob to bypass the resolution. This is reasonable if the setup is such that there is no protocol processing at all.

UDP tunnel headers get the same handling on the inner header.

Packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are unaffected.

Assisted-by LLM

Reported-by Paulos Yibelo <[email protected]>

Reported-by Chris Arges <[email protected]>

Tested-by Chris Arges <[email protected]>

Fixes f43798c27684 ("tun: Allow GSO using virtio_net_hdr")

Suggested-by Willem de Bruijn <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 include/linux/virtio_net.h | 251 +++++++++++++++++++++++++++++++++++--
 1 file changed, 241 insertions(+), 10 deletions(-)

include/linux/virtio_net.h +233 -10

index d6466f96cdd0..357115ff0ec2 100644
@@ -5,7 +5,11 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/netdevice.h>
 #include <linux/udp.h>
+#include <net/gso.h>
+#include <net/ip.h>
+#include <net/ipv6.h>
 #include <net/tcp.h>
 #include <uapi/linux/tcp.h>
 #include <uapi/linux/virtio_net.h>

@@ -48,6 +52,171 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
 	return 0;
 }
 
+/* TODO: nothing virtio specific here, move to ip.h */
+
+/*
+ * Pull and minimally validate the IPv4 header at @nhoff.
+ * Returns the header and stores the transport header offset in
+ * @thoff, or NULL if truncated or malformed.  Only the fixed header
+ * is guaranteed linear; callers that need the options or the
+ * transport header must pull further.
+ */
+static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb,
+						  unsigned int nhoff,
+						  unsigned int *thoff)
+{
+	struct iphdr *iph;
+
+	if (!pskb_may_pull(skb, nhoff + sizeof(struct iphdr)))
+		return NULL;
+
+	iph = (struct iphdr *)(skb->data + nhoff);
+	if (iph->version != 4 ||
+	    iph->ihl < sizeof(struct iphdr) / sizeof(u32))
+		return NULL;
+
+	*thoff = nhoff + iph->ihl * sizeof(u32);
+	return iph;
+}
+
+/* Where does the L3 header start, and what protocol is it?
+ * Handles Ethernet and VLAN headers; other devices use the
+ * network header offset the caller set up.
+ */
+static inline bool
+virtio_net_l3_start(struct sk_buff *skb, int *nhoff, __be16 *proto)
+{
+	if (skb->dev->type == ARPHRD_ETHER) {
+		if (!pskb_may_pull(skb, ETH_HLEN))
+			return false;
+		*nhoff = ETH_HLEN;
+		*proto = eth_hdr(skb)->h_proto;
+		if (eth_type_vlan(*proto)) {
+			*proto = __vlan_get_protocol(skb, *proto, nhoff);
+			if (!*proto)
+				return false;
+		}
+	} else {
+		*nhoff = skb_network_offset(skb);
+		*proto = skb->protocol;
+	}
+	return true;
+}
+
+/* Walk the L3 header at @nhoff: where does its transport header
+ * start, and which protocol is it?  Returns false if the header
+ * is truncated, malformed or fragmented.
+ */
+static inline bool
+virtio_net_l4_start(struct sk_buff *skb, unsigned int nhoff, __be16 proto,
+		    unsigned int *thoff, int *ip_proto)
+{
+	if (proto == htons(ETH_P_IP)) {
+		const struct iphdr *iph = virtio_net_ip4_parse(skb, nhoff, thoff);
+
+		if (!iph || ip_is_fragment(iph))
+			return false;
+		*ip_proto = iph->protocol;
+		return true;
+	}
+
+	if (proto == htons(ETH_P_IPV6)) {
+		const struct ipv6hdr *iph;
+		int flags = 0;
+
+		if (!pskb_may_pull(skb, nhoff + sizeof(*iph)))
+			return false;
+		iph = (const void *)(skb->data + nhoff);
+		if (iph->version != 6)
+			return false;
+		*thoff = nhoff + sizeof(*iph);
+		*ip_proto = iph->nexthdr;
+		if (*ip_proto != IPPROTO_TCP && *ip_proto != IPPROTO_UDP) {
+			*thoff = nhoff;
+			*ip_proto = ipv6_find_hdr(skb, thoff, -1, NULL,
+						  &flags);
+			if (*ip_proto < 0 || flags & IP6_FH_F_FRAG)
+				return false;
+		}
+		return true;
+	}
+
+	return false;
+}
+
+/* Does checksum metadata match a known TCP or UDP header? */
+static inline bool
+virtio_net_csum_partial_ok_at_offset(struct sk_buff *skb, u8 hdr_gso_type,
+				     unsigned int nhoff, __be16 proto)
+{
+	unsigned int thoff;
+	int ip_proto;
+
+	if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
+		return false;
+
+	if (skb_checksum_start_offset(skb) != thoff)
+		return false;
+
+	switch (ip_proto) {
+	case IPPROTO_TCP:
+		if (skb->csum_offset != offsetof(struct tcphdr, check) ||
+		    !pskb_may_pull(skb, thoff + sizeof(struct tcphdr)))
+			return false;
+		break;
+	case IPPROTO_UDP:
+		if (skb->csum_offset != offsetof(struct udphdr, check) ||
+		    !pskb_may_pull(skb, thoff + sizeof(struct udphdr)))
+			return false;
+		break;
+	default:
+		return false;
+	}
+
+	switch (hdr_gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
+	case VIRTIO_NET_HDR_GSO_NONE:
+		return true;
+	case VIRTIO_NET_HDR_GSO_TCPV4:
+		return proto == htons(ETH_P_IP) && ip_proto == IPPROTO_TCP;
+	case VIRTIO_NET_HDR_GSO_TCPV6:
+		return proto == htons(ETH_P_IPV6) && ip_proto == IPPROTO_TCP;
+	case VIRTIO_NET_HDR_GSO_UDP:
+	case VIRTIO_NET_HDR_GSO_UDP_L4:
+		return ip_proto == IPPROTO_UDP;
+	default:
+		return false;
+	}
+}
+
+static inline bool
+virtio_net_csum_partial_ok(struct sk_buff *skb, u8 hdr_gso_type)
+{
+	int nhoff;
+	__be16 proto;
+
+	if (!virtio_net_l3_start(skb, &nhoff, &proto))
+		return false;
+
+	return virtio_net_csum_partial_ok_at_offset(skb, hdr_gso_type, nhoff,
+						   proto);
+}
+
+/*
+ * Handle a packet whose checksum metadata does not match the packet:
+ * GSO packets are dropped, as segmentation needs one checksum per
+ * segment; others are completed in software.
+ *
+ * Return 1 if the checksum was completed in software, else a negative
+ * errno.
+ */
+static inline int virtio_net_resolve_csum_partial_mismatch(struct sk_buff *skb)
+{
+	if (skb_is_gso(skb))
+		return -EINVAL;
+
+	return skb_checksum_help(skb) ?: 1;
+}
+
 static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
 					  const struct virtio_net_hdr *hdr,
 					  bool little_endian, u8 hdr_gso_type)

@@ -158,8 +327,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
 		case SKB_GSO_UDP_L4:
 			if (!(hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM))
 				return -EINVAL;
-			if (skb->csum_offset != offsetof(struct udphdr, check))
-				return -EINVAL;
 			if (skb->len - p_off > gso_size * UDP_MAX_SEGMENTS)
 				return -EINVAL;
 			if (gso_type != SKB_GSO_UDP_L4)

@@ -167,10 +334,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
 			break;
 		case SKB_GSO_TCPV4:
 		case SKB_GSO_TCPV6:
-			if (skb->ip_summed == CHECKSUM_PARTIAL &&
-			    skb->csum_offset != offsetof(struct tcphdr, check))
-				return -EINVAL;
-
 			BUILD_BUG_ON(TCP_MIN_GSO_SIZE * GSO_MAX_SEGS < GSO_MAX_SIZE);
 			gso_size = max(gso_size, TCP_MIN_GSO_SIZE);
 			break;

@@ -194,11 +357,36 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
 	return 0;
 }
 
+/*
+ * Return value:
+ * 0 - success
+ * >0 - checksum was completed in software (slow path)
+ * <0 - failure
+ */
 static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
 					const struct virtio_net_hdr *hdr,
 					bool little_endian)
 {
-	return __virtio_net_hdr_to_skb(skb, hdr, little_endian, hdr->gso_type);
+	typeof(skb->transport_header) old_transport = skb->transport_header;
+	int ret = __virtio_net_hdr_to_skb(skb, hdr, little_endian,
+					  hdr->gso_type);
+
+	if (ret < 0)
+		return ret;
+
+	if (skb->ip_summed != CHECKSUM_PARTIAL ||
+	    virtio_net_csum_partial_ok(skb, hdr->gso_type))
+		return 0;
+
+	/*
+	 * Here we do not trust the checksum metadata.
+	 * __virtio_net_hdr_to_skb set the transport header to csum_start
+	 * from virtio hdr, restore it to a known good value.
+	 */
+	skb->transport_header = old_transport;
+	ret = virtio_net_resolve_csum_partial_mismatch(skb);
+
+	return ret;
 }
 
 /* This function must be called after virtio_net_hdr_from_skb(). */

@@ -282,6 +470,35 @@ static inline unsigned int virtio_l3min(bool is_ipv6)
 	return is_ipv6 ? sizeof(struct ipv6hdr) : sizeof(struct iphdr);
 }
 
+/* Do the tunnel offsets match the outer headers in the frame?
+ * The L3 header the tunnel claims must be the one the frame
+ * carries, with UDP following it at @outer_th.
+ */
+static inline bool
+virtio_net_tnl_offsets_ok(struct sk_buff *skb, bool outer_isv6,
+			  unsigned int outer_th)
+{
+	int nhoff;
+	__be16 proto;
+	unsigned int thoff;
+	int ip_proto;
+
+	if (!virtio_net_l3_start(skb, &nhoff, &proto))
+		return false;
+	if (outer_isv6 != (proto == htons(ETH_P_IPV6)))
+		return false;
+	if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
+		return false;
+
+	return ip_proto == IPPROTO_UDP && outer_th == thoff;
+}
+
+/*
+ * Return value:
+ * 0 - success
+ * >0 - checksum was completed in software (slow path)
+ * <0 - failure
+ */
 static inline int
 virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 			  const struct virtio_net_hdr_v1_hash_tunnel *vhdr,

@@ -293,6 +510,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	unsigned int inner_nh, outer_th, inner_th;
 	unsigned int inner_l3min, outer_l3min;
 	u8 gso_inner_type, gso_tunnel_type;
+	__be16 inner_proto;
 	bool outer_isv6, inner_isv6;
 	int ret;
 

@@ -332,10 +550,13 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	    inner_th < inner_nh + inner_l3min)
 		return -EINVAL;
 
+	if (!virtio_net_tnl_offsets_ok(skb, outer_isv6, outer_th))
+		return -EINVAL;
+
 	/* Let the basic parsing deal with plain GSO features. */
 	ret = __virtio_net_hdr_to_skb(skb, hdr, true,
 				      hdr->gso_type & ~gso_tunnel_type);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	/* In case of USO, the inner protocol is still unknown and

@@ -353,7 +574,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	}
 
 	skb_set_inner_protocol(skb, inner_isv6 ? htons(ETH_P_IPV6) :
-						 htons(ETH_P_IP));
+			    htons(ETH_P_IP));
 	if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) {
 		if (!tnl_csum_negotiated)
 			return -EINVAL;

@@ -368,7 +589,17 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	skb->inner_mac_header = inner_nh + skb_headroom(skb);
 	skb->transport_header = outer_th + skb_headroom(skb);
 	skb->encapsulation = 1;
-	return 0;
+
+	/*
+	 * We know VIRTIO_NET_HDR_F_NEEDS_CSUM was set so __virtio_net_hdr_to_skb

include/linux/virtio_net.h +0 -0

 /* Checksum-related fields validation for the driver */

Signature · 1 line

MST

The ingress validation as implemented by the previous patch is stricter than what it replaced: GSO
packets whose checksum metadata matches no transport header are dropped, and non-GSO ones get their checksum completed in place. Such packets used to pass with the metadata unvalidated, and with no overhead.

I judge that this should not affect any deployments, but there is still some risk that it does. In case a deployment depends on them anyway, provide a per-netns switch to get the old behavior back:

echo 0 > /proc/sys/net/core/virtio_net_hdr_csum_check

The default is validating.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 Documentation/admin-guide/sysctl/net.rst | 15 +++++++
 include/linux/virtio_net.h               | 23 ++++++++++
 include/net/netns/core.h                 |  1 +
 net/core/dev.c                           |  6 +++
 net/core/net_namespace.c                 |  1 +
 net/core/sysctl_net_core.c               | 54 ++++++++++++++++++++++++
 6 files changed, 100 insertions(+)

Documentation/admin-guide/sysctl/net.rst +0 -0

index e586e17fc7a5..7f230929c37e 100644

Documentation/admin-guide/sysctl/net.rst +13 -0

+
+When processing packets with a virtio net header (through tun, tap, a
+packet socket or a virtio network device) that request checksum offload
+(VIRTIO_NET_HDR_F_NEEDS_CSUM), validate the packet: complete the checksum
+in software, or drop the packet where the header does not match it.
+
+When disabled, the header is trusted not to corrupt the packet, and the
+checksum is offloaded to hardware (or not verified if it never reaches
+hardware). This is only safe if no protocol is processing the packets.
+
+Default: 1 (on)
+
+
 wmem_default
 ------------
 

include/linux/virtio_net.h +1 -0

index 357115ff0ec2..aa2f8ac4657c 100644
@@ -5,6 +5,7 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/jump_label.h>
 #include <linux/netdevice.h>
 #include <linux/udp.h>
 #include <net/gso.h>

include/linux/virtio_net.h +59 -0

+	if (virtio_net_hdr_csum_check_disabled(skb))
+		return true;
+
 	if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
 		return false;
 

@@ -483,6 +503,9 @@ virtio_net_tnl_offsets_ok(struct sk_buff *skb, bool outer_isv6,
 	unsigned int thoff;
 	int ip_proto;
 
+	if (virtio_net_hdr_csum_check_disabled(skb))
+		return true;
+
 	if (!virtio_net_l3_start(skb, &nhoff, &proto))
 		return false;
 	if (outer_isv6 != (proto == htons(ETH_P_IPV6)))
 
+/* Defined in net/core/dev.c, declared for the gate in
+ * include/linux/virtio_net.h.
+ */
+DECLARE_STATIC_KEY_FALSE(virtio_net_hdr_csum_check_disabled_key);
+
+/*
+ * Protects changes to virtio_net_hdr_csum_check_disabled_key and each
+ * net->core.sysctl_virtio_net_hdr_csum_check. Readers don't use a lock.
+ */
+static DEFINE_MUTEX(virtio_net_hdr_csum_check_mutex);
+
+static int virtio_net_hdr_csum_check_sysctl(const struct ctl_table *table,
+					    int write, void *buffer,
+					    size_t *lenp, loff_t *ppos)
+{
+	struct net *net = (struct net *)table->data;
+	/* checkpatch warns but this can't be const: tmp.data is set below */
+	struct ctl_table tmp = *table;
+	u8 old;
+	int ret;
+
+	/*
+	 * Make sure virtio_net_hdr_csum_check_disabled_key counts zero values of
+	 * net->core.sysctl_virtio_net_hdr_csum_check.
+	 */
+	mutex_lock(&virtio_net_hdr_csum_check_mutex);
+
+	old = net->core.sysctl_virtio_net_hdr_csum_check;
+	tmp.data = &net->core.sysctl_virtio_net_hdr_csum_check;
+	ret = proc_dou8vec_minmax(&tmp, write, buffer, lenp, ppos);
+	if (write && !ret && old != net->core.sysctl_virtio_net_hdr_csum_check) {
+		if (net->core.sysctl_virtio_net_hdr_csum_check)
+			static_branch_dec(&virtio_net_hdr_csum_check_disabled_key);
+		else
+			static_branch_inc(&virtio_net_hdr_csum_check_disabled_key);
+	}
+
+	mutex_unlock(&virtio_net_hdr_csum_check_mutex);
+
+	return ret;
+}
+
 static const struct ctl_table netns_core_table[] = {
 #if IS_ENABLED(CONFIG_RPS)
 	{

@@ -737,6 +780,15 @@ static const struct ctl_table netns_core_table[] = {
 		.extra1		= SYSCTL_ZERO,
 		.extra2		= SYSCTL_ONE
 	},
+	{
+		.procname	= "virtio_net_hdr_csum_check",
+		.data		= &init_net,
+		.maxlen		= sizeof(u8),
+		.mode		= 0644,
+		.proc_handler	= virtio_net_hdr_csum_check_sysctl,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE
+	},
 	/* sysctl_core_net_init() will set the values after this
 	 * to readonly in network namespaces
 	 */

@@ -841,6 +893,8 @@ static __net_exit void sysctl_core_net_exit(struct net *net)
 	tbl = net->core.sysctl_hdr->ctl_table_arg;
 	unregister_net_sysctl_table(net->core.sysctl_hdr);
 	BUG_ON(tbl == netns_core_table);
+	if (!net->core.sysctl_virtio_net_hdr_csum_check)
+		static_branch_dec(&virtio_net_hdr_csum_check_disabled_key);
 #if IS_ENABLED(CONFIG_RPS)
 	kfree(net->core.rps_default_mask);
 #endif

Signature · 1 line

MST

Quoted 34 lines · Michael S. Tsirkin

The ingress validation as implemented by the previous patch is stricter than what it replaced: GSO
packets whose checksum metadata matches no transport header are dropped, and non-GSO ones get their checksum completed in place. Such packets used to pass with the metadata unvalidated, and with no overhead.

I judge that this should not affect any deployments, but there is still some risk that it does. In case a deployment depends on them anyway, provide a per-netns switch to get the old behavior back:

echo 0 > /proc/sys/net/core/virtio_net_hdr_csum_check

The default is validating.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 Documentation/admin-guide/sysctl/net.rst | 15 +++++++
 include/linux/virtio_net.h               | 23 ++++++++++
 include/net/netns/core.h                 |  1 +
 net/core/dev.c                           |  6 +++
 net/core/net_namespace.c                 |  1 +
 net/core/sysctl_net_core.c               | 54 ++++++++++++++++++++++++
 6 files changed, 100 insertions(+)

Documentation/admin-guide/sysctl/net.rst +0 -0

index e586e17fc7a5..7f230929c37e 100644
@@ -246,6 +246,21 @@ processes are dropped unless socket option SOF_TIMESTAMPING_OPT_TSONLY is set.
 Default: 1 (on)
 
 

Documentation/admin-guide/sysctl/net.rst processes are dropped unless socket option SOF_TIMESTAMPING_OPT_TSONLY is set. · line 250

Quoted 2 lines

Documentation/admin-guide/sysctl/net.rst +2 -0

+virtio_net_hdr_csum_check
+-------------------------

Make more generic, virtio_net_hdr_validate?

In case we add more validation in the future (as we have in the past), without having to introduce multiple such knobs.

Quoted 29 lines

Documentation/admin-guide/sysctl/net.rst +13 -0

+
+When processing packets with a virtio net header (through tun, tap, a
+packet socket or a virtio network device) that request checksum offload
+(VIRTIO_NET_HDR_F_NEEDS_CSUM), validate the packet: complete the checksum
+in software, or drop the packet where the header does not match it.
+
+When disabled, the header is trusted not to corrupt the packet, and the
+checksum is offloaded to hardware (or not verified if it never reaches
+hardware). This is only safe if no protocol is processing the packets.
+
+Default: 1 (on)
+
+
 wmem_default
 ------------
 

include/linux/virtio_net.h +1 -0

index 357115ff0ec2..aa2f8ac4657c 100644
@@ -5,6 +5,7 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/jump_label.h>
 #include <linux/netdevice.h>
 #include <linux/udp.h>
 #include <net/gso.h>

@@ -78,6 +79,19 @@ static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb,

include/linux/virtio_net.h static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb, · line 84

Quoted 6 lines

include/linux/virtio_net.h +3 -0

 	*thoff = nhoff + iph->ihl * sizeof(u32);
 	return iph;
 }
+/* Defined in net/core/dev.c, enabled while any network namespace
+ * has net.core.virtio_net_hdr_csum_check set to 0.
+ */

Should this be a global sysctl, rather than per netns?

Even if per-netns, most likely do not want it writable by netns root with user namespaces.

That is the equivalent of ns_capable(CAP_NET_RAW), which is the cause of many crashes due to bad packets ingested by, e.g., a packet socket.

A global sysctl also allows using proc_do_static_key and avoids the custom logic in virtio_net_hdr_csum_check_sysctl.

Quoted 6 lines

include/linux/virtio_net.h +0 -0

 /* Where does the L3 header start, and what protocol is it?
  * Handles Ethernet and VLAN headers; other devices use the

@@ -152,6 +166,12 @@ virtio_net_csum_partial_ok_at_offset(struct sk_buff *skb, u8 hdr_gso_type,
 	unsigned int thoff;
 	int ip_proto;
 

include/linux/virtio_net.h virtio_net_csum_partial_ok_at_offset(struct sk_buff *skb, u8 hdr_gso_type, · line 171

Quoted 3 lines

include/linux/virtio_net.h +3 -0

+	/* Failsafe: a namespace opted out of the validation,
+	 * trust the checksum metadata as before.
+	 */

nit: as before, imho the code here is clear and comment unnecessary.

Quoted 90 lines

include/linux/virtio_net.h +59 -0

+	if (virtio_net_hdr_csum_check_disabled(skb))
+		return true;
+
 	if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
 		return false;
 

@@ -483,6 +503,9 @@ virtio_net_tnl_offsets_ok(struct sk_buff *skb, bool outer_isv6,
 	unsigned int thoff;
 	int ip_proto;
 
+	if (virtio_net_hdr_csum_check_disabled(skb))
+		return true;
+
 	if (!virtio_net_l3_start(skb, &nhoff, &proto))
 		return false;
 	if (outer_isv6 != (proto == htons(ETH_P_IPV6)))
 
+/* Defined in net/core/dev.c, declared for the gate in
+ * include/linux/virtio_net.h.
+ */
+DECLARE_STATIC_KEY_FALSE(virtio_net_hdr_csum_check_disabled_key);
+
+/*
+ * Protects changes to virtio_net_hdr_csum_check_disabled_key and each
+ * net->core.sysctl_virtio_net_hdr_csum_check. Readers don't use a lock.
+ */
+static DEFINE_MUTEX(virtio_net_hdr_csum_check_mutex);
+
+static int virtio_net_hdr_csum_check_sysctl(const struct ctl_table *table,
+					    int write, void *buffer,
+					    size_t *lenp, loff_t *ppos)
+{
+	struct net *net = (struct net *)table->data;
+	/* checkpatch warns but this can't be const: tmp.data is set below */
+	struct ctl_table tmp = *table;
+	u8 old;
+	int ret;
+
+	/*
+	 * Make sure virtio_net_hdr_csum_check_disabled_key counts zero values of
+	 * net->core.sysctl_virtio_net_hdr_csum_check.
+	 */
+	mutex_lock(&virtio_net_hdr_csum_check_mutex);
+
+	old = net->core.sysctl_virtio_net_hdr_csum_check;
+	tmp.data = &net->core.sysctl_virtio_net_hdr_csum_check;
+	ret = proc_dou8vec_minmax(&tmp, write, buffer, lenp, ppos);
+	if (write && !ret && old != net->core.sysctl_virtio_net_hdr_csum_check) {
+		if (net->core.sysctl_virtio_net_hdr_csum_check)
+			static_branch_dec(&virtio_net_hdr_csum_check_disabled_key);
+		else
+			static_branch_inc(&virtio_net_hdr_csum_check_disabled_key);
+	}
+
+	mutex_unlock(&virtio_net_hdr_csum_check_mutex);
+
+	return ret;
+}
+
 static const struct ctl_table netns_core_table[] = {
 #if IS_ENABLED(CONFIG_RPS)
 	{

@@ -737,6 +780,15 @@ static const struct ctl_table netns_core_table[] = {
 		.extra1		= SYSCTL_ZERO,
 		.extra2		= SYSCTL_ONE
 	},
+	{
+		.procname	= "virtio_net_hdr_csum_check",
+		.data		= &init_net,
+		.maxlen		= sizeof(u8),
+		.mode		= 0644,
+		.proc_handler	= virtio_net_hdr_csum_check_sysctl,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE
+	},
 	/* sysctl_core_net_init() will set the values after this
 	 * to readonly in network namespaces
 	 */

@@ -841,6 +893,8 @@ static __net_exit void sysctl_core_net_exit(struct net *net)
 	tbl = net->core.sysctl_hdr->ctl_table_arg;
 	unregister_net_sysctl_table(net->core.sysctl_hdr);
 	BUG_ON(tbl == netns_core_table);
+	if (!net->core.sysctl_virtio_net_hdr_csum_check)
+		static_branch_dec(&virtio_net_hdr_csum_check_disabled_key);
 #if IS_ENABLED(CONFIG_RPS)
 	kfree(net->core.rps_default_mask);
 #endif

Signature · 1 line

MST

Quoted 35 lines · Willem de Bruijn

Quoted 34 lines · Michael S. Tsirkin

The ingress validation as implemented by the previous patch is stricter than what it replaced: GSO
packets whose checksum metadata matches no transport header are dropped, and non-GSO ones get their checksum completed in place. Such packets used to pass with the metadata unvalidated, and with no overhead.

I judge that this should not affect any deployments, but there is still some risk that it does. In case a deployment depends on them anyway, provide a per-netns switch to get the old behavior back:

echo 0 > /proc/sys/net/core/virtio_net_hdr_csum_check

The default is validating.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 Documentation/admin-guide/sysctl/net.rst | 15 +++++++
 include/linux/virtio_net.h               | 23 ++++++++++
 include/net/netns/core.h                 |  1 +
 net/core/dev.c                           |  6 +++
 net/core/net_namespace.c                 |  1 +
 net/core/sysctl_net_core.c               | 54 ++++++++++++++++++++++++
 6 files changed, 100 insertions(+)

Documentation/admin-guide/sysctl/net.rst +0 -0

index e586e17fc7a5..7f230929c37e 100644
@@ -246,6 +246,21 @@ processes are dropped unless socket option SOF_TIMESTAMPING_OPT_TSONLY is set.
 Default: 1 (on)
 
 

Quoted 7 lines

Documentation/admin-guide/sysctl/net.rst processes are dropped unless socket option SOF_TIMESTAMPING_OPT_TSONLY is set. · line 250

Quoted 2 lines

Documentation/admin-guide/sysctl/net.rst +2 -0

+virtio_net_hdr_csum_check
+-------------------------

Make more generic, virtio_net_hdr_validate?

In case we add more validation in the future (as we have in the past), without having to introduce multiple such knobs.

But it's uapi, are you sure we'll be able to change the meaning of an existing knob, even if we keep the name vague?

Quoted 29 lines

Quoted 29 lines

Documentation/admin-guide/sysctl/net.rst +13 -0

+
+When processing packets with a virtio net header (through tun, tap, a
+packet socket or a virtio network device) that request checksum offload
+(VIRTIO_NET_HDR_F_NEEDS_CSUM), validate the packet: complete the checksum
+in software, or drop the packet where the header does not match it.
+
+When disabled, the header is trusted not to corrupt the packet, and the
+checksum is offloaded to hardware (or not verified if it never reaches
+hardware). This is only safe if no protocol is processing the packets.
+
+Default: 1 (on)
+
+
 wmem_default
 ------------
 

include/linux/virtio_net.h +1 -0

index 357115ff0ec2..aa2f8ac4657c 100644
@@ -5,6 +5,7 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/jump_label.h>
 #include <linux/netdevice.h>
 #include <linux/udp.h>
 #include <net/gso.h>

@@ -78,6 +79,19 @@ static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb,

Quoted 8 lines

include/linux/virtio_net.h static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb, · line 84

Quoted 6 lines

include/linux/virtio_net.h +3 -0

 	*thoff = nhoff + iph->ihl * sizeof(u32);
 	return iph;
 }
+/* Defined in net/core/dev.c, enabled while any network namespace
+ * has net.core.virtio_net_hdr_csum_check set to 0.
+ */

Should this be a global sysctl, rather than per netns?

For customers I work with, it would be much less useful.

Quoted 6 lines

Even if per-netns, most likely do not want it writable by netns root with user namespaces.

That is the equivalent of ns_capable(CAP_NET_RAW), which is the cause of many crashes due to bad packets ingested by, e.g., a packet socket.

Hmm, yes i get that. Two knobs? a. verify b. the per netns value ro? Both on by default?

Quoted 86 lines · Michael S. Tsirkin

Quoted 35 lines · Willem de Bruijn

Quoted 34 lines · Michael S. Tsirkin

The ingress validation as implemented by the previous patch is stricter than what it replaced: GSO
packets whose checksum metadata matches no transport header are dropped, and non-GSO ones get their checksum completed in place. Such packets used to pass with the metadata unvalidated, and with no overhead.

I judge that this should not affect any deployments, but there is still some risk that it does. In case a deployment depends on them anyway, provide a per-netns switch to get the old behavior back:

echo 0 > /proc/sys/net/core/virtio_net_hdr_csum_check

The default is validating.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 Documentation/admin-guide/sysctl/net.rst | 15 +++++++
 include/linux/virtio_net.h               | 23 ++++++++++
 include/net/netns/core.h                 |  1 +
 net/core/dev.c                           |  6 +++
 net/core/net_namespace.c                 |  1 +
 net/core/sysctl_net_core.c               | 54 ++++++++++++++++++++++++
 6 files changed, 100 insertions(+)

Documentation/admin-guide/sysctl/net.rst +0 -0

index e586e17fc7a5..7f230929c37e 100644
@@ -246,6 +246,21 @@ processes are dropped unless socket option SOF_TIMESTAMPING_OPT_TSONLY is set.
 Default: 1 (on)
 
 

Quoted 7 lines

Documentation/admin-guide/sysctl/net.rst processes are dropped unless socket option SOF_TIMESTAMPING_OPT_TSONLY is set. · line 250

Quoted 2 lines

Documentation/admin-guide/sysctl/net.rst +2 -0

+virtio_net_hdr_csum_check
+-------------------------

Make more generic, virtio_net_hdr_validate?

In case we add more validation in the future (as we have in the past), without having to introduce multiple such knobs.

But it's uapi, are you sure we'll be able to change the meaning of an existing knob, even if we keep the name vague?

Quoted 29 lines

Quoted 29 lines

Documentation/admin-guide/sysctl/net.rst +13 -0

+
+When processing packets with a virtio net header (through tun, tap, a
+packet socket or a virtio network device) that request checksum offload
+(VIRTIO_NET_HDR_F_NEEDS_CSUM), validate the packet: complete the checksum
+in software, or drop the packet where the header does not match it.
+
+When disabled, the header is trusted not to corrupt the packet, and the
+checksum is offloaded to hardware (or not verified if it never reaches
+hardware). This is only safe if no protocol is processing the packets.
+
+Default: 1 (on)
+
+
 wmem_default
 ------------
 

include/linux/virtio_net.h +1 -0

index 357115ff0ec2..aa2f8ac4657c 100644
@@ -5,6 +5,7 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/jump_label.h>
 #include <linux/netdevice.h>
 #include <linux/udp.h>
 #include <net/gso.h>

@@ -78,6 +79,19 @@ static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb,

Quoted 8 lines

include/linux/virtio_net.h static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb, · line 84

Quoted 6 lines

include/linux/virtio_net.h +3 -0

 	*thoff = nhoff + iph->ihl * sizeof(u32);
 	return iph;
 }
+/* Defined in net/core/dev.c, enabled while any network namespace
+ * has net.core.virtio_net_hdr_csum_check set to 0.
+ */

Should this be a global sysctl, rather than per netns?

Quoted 1 line

For customers I work with, it would be much less useful.

Third option: make it an integer rather than a boolean.

0: no validation
1: current level of validation
2 and beyond: if we have to extend it later

Not pretty, I'll admit. So fine to keep as is too if you prefer. But I do foresee more changes in this area, if history is a guide.

Quoted 7 lines

Quoted 6 lines

Even if per-netns, most likely do not want it writable by netns root with user namespaces.

That is the equivalent of ns_capable(CAP_NET_RAW), which is the cause of many crashes due to bad packets ingested by, e.g., a packet socket.

Quoted 2 lines

Hmm, yes i get that. Two knobs? a. verify b. the per netns value ro? Both on by default?

A single system wide field is simplest.

Is there a benefit to making this configurable on a per netns basis?

If so, that's fine too. As long as root-in-namespace cannot modify it, so readonly indeed.

Quoted 108 lines · Willem de Bruijn

Quoted 86 lines · Michael S. Tsirkin

Quoted 35 lines · Willem de Bruijn

Quoted 34 lines · Michael S. Tsirkin

The ingress validation as implemented by the previous patch is stricter than what it replaced: GSO
packets whose checksum metadata matches no transport header are dropped, and non-GSO ones get their checksum completed in place. Such packets used to pass with the metadata unvalidated, and with no overhead.

I judge that this should not affect any deployments, but there is still some risk that it does. In case a deployment depends on them anyway, provide a per-netns switch to get the old behavior back:

echo 0 > /proc/sys/net/core/virtio_net_hdr_csum_check

The default is validating.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 Documentation/admin-guide/sysctl/net.rst | 15 +++++++
 include/linux/virtio_net.h               | 23 ++++++++++
 include/net/netns/core.h                 |  1 +
 net/core/dev.c                           |  6 +++
 net/core/net_namespace.c                 |  1 +
 net/core/sysctl_net_core.c               | 54 ++++++++++++++++++++++++
 6 files changed, 100 insertions(+)

Documentation/admin-guide/sysctl/net.rst +0 -0

index e586e17fc7a5..7f230929c37e 100644
@@ -246,6 +246,21 @@ processes are dropped unless socket option SOF_TIMESTAMPING_OPT_TSONLY is set.
 Default: 1 (on)
 
 

Quoted 7 lines

Documentation/admin-guide/sysctl/net.rst processes are dropped unless socket option SOF_TIMESTAMPING_OPT_TSONLY is set. · line 250

Quoted 2 lines

Documentation/admin-guide/sysctl/net.rst +2 -0

+virtio_net_hdr_csum_check
+-------------------------

Make more generic, virtio_net_hdr_validate?

In case we add more validation in the future (as we have in the past), without having to introduce multiple such knobs.

But it's uapi, are you sure we'll be able to change the meaning of an existing knob, even if we keep the name vague?

Quoted 29 lines

Quoted 29 lines

Documentation/admin-guide/sysctl/net.rst +13 -0

+
+When processing packets with a virtio net header (through tun, tap, a
+packet socket or a virtio network device) that request checksum offload
+(VIRTIO_NET_HDR_F_NEEDS_CSUM), validate the packet: complete the checksum
+in software, or drop the packet where the header does not match it.
+
+When disabled, the header is trusted not to corrupt the packet, and the
+checksum is offloaded to hardware (or not verified if it never reaches
+hardware). This is only safe if no protocol is processing the packets.
+
+Default: 1 (on)
+
+
 wmem_default
 ------------
 

include/linux/virtio_net.h +1 -0

index 357115ff0ec2..aa2f8ac4657c 100644
@@ -5,6 +5,7 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/jump_label.h>
 #include <linux/netdevice.h>
 #include <linux/udp.h>
 #include <net/gso.h>

@@ -78,6 +79,19 @@ static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb,

Quoted 8 lines

include/linux/virtio_net.h static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb, · line 84

Quoted 6 lines

include/linux/virtio_net.h +3 -0

 	*thoff = nhoff + iph->ihl * sizeof(u32);
 	return iph;
 }
+/* Defined in net/core/dev.c, enabled while any network namespace
+ * has net.core.virtio_net_hdr_csum_check set to 0.
+ */

Should this be a global sysctl, rather than per netns?

Quoted 1 line

For customers I work with, it would be much less useful.

Third option: make it an integer rather than a boolean.

0: no validation
1: current level of validation
2 and beyond: if we have to extend it later

Not pretty, I'll admit. So fine to keep as is too if you prefer. But I do foresee more changes in this area, if history is a guide.

Quoted 9 lines

Quoted 6 lines

Even if per-netns, most likely do not want it writable by netns root with user namespaces.

That is the equivalent of ns_capable(CAP_NET_RAW), which is the cause of many crashes due to bad packets ingested by, e.g., a packet socket.

Hmm, yes i get that. Two knobs? a. verify b. the per netns value ro? Both on by default?

Quoted 3 lines

A single system wide field is simplest.

Is there a benefit to making this configurable on a per netns basis?

As I tried to explain: a specific container breaks, you decide to trust it, you do not need to trust all.

Quoted 2 lines

If so, that's fine too. As long as root-in-namespace cannot modify it, so readonly indeed.

Right. So I envision along the lines of:

net.core.virtio_net_hdr_validate_all - validate packets from any netns, netns root can not change it, only host root

net.core.virtio_net_hdr_validate - per netns

Quoted 109 lines · Michael S. Tsirkin

Quoted 108 lines · Willem de Bruijn

Quoted 86 lines · Michael S. Tsirkin

Quoted 35 lines · Willem de Bruijn

Quoted 34 lines · Michael S. Tsirkin

The ingress validation as implemented by the previous patch is stricter than what it replaced: GSO
packets whose checksum metadata matches no transport header are dropped, and non-GSO ones get their checksum completed in place. Such packets used to pass with the metadata unvalidated, and with no overhead.

I judge that this should not affect any deployments, but there is still some risk that it does. In case a deployment depends on them anyway, provide a per-netns switch to get the old behavior back:

echo 0 > /proc/sys/net/core/virtio_net_hdr_csum_check

The default is validating.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 Documentation/admin-guide/sysctl/net.rst | 15 +++++++
 include/linux/virtio_net.h               | 23 ++++++++++
 include/net/netns/core.h                 |  1 +
 net/core/dev.c                           |  6 +++
 net/core/net_namespace.c                 |  1 +
 net/core/sysctl_net_core.c               | 54 ++++++++++++++++++++++++
 6 files changed, 100 insertions(+)

Documentation/admin-guide/sysctl/net.rst +0 -0

index e586e17fc7a5..7f230929c37e 100644
@@ -246,6 +246,21 @@ processes are dropped unless socket option SOF_TIMESTAMPING_OPT_TSONLY is set.
 Default: 1 (on)
 
 

Quoted 7 lines

Documentation/admin-guide/sysctl/net.rst processes are dropped unless socket option SOF_TIMESTAMPING_OPT_TSONLY is set. · line 250

Quoted 2 lines

Documentation/admin-guide/sysctl/net.rst +2 -0

+virtio_net_hdr_csum_check
+-------------------------

Make more generic, virtio_net_hdr_validate?

In case we add more validation in the future (as we have in the past), without having to introduce multiple such knobs.

But it's uapi, are you sure we'll be able to change the meaning of an existing knob, even if we keep the name vague?

Quoted 29 lines

Quoted 29 lines

Documentation/admin-guide/sysctl/net.rst +13 -0

+
+When processing packets with a virtio net header (through tun, tap, a
+packet socket or a virtio network device) that request checksum offload
+(VIRTIO_NET_HDR_F_NEEDS_CSUM), validate the packet: complete the checksum
+in software, or drop the packet where the header does not match it.
+
+When disabled, the header is trusted not to corrupt the packet, and the
+checksum is offloaded to hardware (or not verified if it never reaches
+hardware). This is only safe if no protocol is processing the packets.
+
+Default: 1 (on)
+
+
 wmem_default
 ------------
 

include/linux/virtio_net.h +1 -0

index 357115ff0ec2..aa2f8ac4657c 100644
@@ -5,6 +5,7 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/jump_label.h>
 #include <linux/netdevice.h>
 #include <linux/udp.h>
 #include <net/gso.h>

@@ -78,6 +79,19 @@ static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb,

Quoted 8 lines

include/linux/virtio_net.h static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb, · line 84

Quoted 6 lines

include/linux/virtio_net.h +3 -0

 	*thoff = nhoff + iph->ihl * sizeof(u32);
 	return iph;
 }
+/* Defined in net/core/dev.c, enabled while any network namespace
+ * has net.core.virtio_net_hdr_csum_check set to 0.
+ */

Should this be a global sysctl, rather than per netns?

Quoted 1 line

For customers I work with, it would be much less useful.

Third option: make it an integer rather than a boolean.

0: no validation
1: current level of validation
2 and beyond: if we have to extend it later

Not pretty, I'll admit. So fine to keep as is too if you prefer. But I do foresee more changes in this area, if history is a guide.

Quoted 9 lines

Quoted 6 lines

Even if per-netns, most likely do not want it writable by netns root with user namespaces.

That is the equivalent of ns_capable(CAP_NET_RAW), which is the cause of many crashes due to bad packets ingested by, e.g., a packet socket.

Hmm, yes i get that. Two knobs? a. verify b. the per netns value ro? Both on by default?

Quoted 6 lines

Quoted 3 lines

A single system wide field is simplest.

Is there a benefit to making this configurable on a per netns basis?

As I tried to explain: a specific container breaks, you decide to trust it, you do not need to trust all.

Ok

Quoted 9 lines

Quoted 2 lines

If so, that's fine too. As long as root-in-namespace cannot modify it, so readonly indeed.

Right. So I envision along the lines of:

net.core.virtio_net_hdr_validate_all - validate packets from any netns, netns root can not change it, only host root

Quoted 1 line

net.core.virtio_net_hdr_validate - per netns

Then skip the global option, as it's just a convenience: the same can be achieved by iterating over all the per-netns settings.

Per netns validation, but only settable by root, not by root-in-namespace.

Quoted 44 lines

Quoted 24 lines

Quoted 24 lines

Quoted 4 lines

Quoted 1 line

Should this be a global sysctl, rather than per netns?

For customers I work with, it would be much less useful.

Third option: make it an integer rather than a boolean.

0: no validation
1: current level of validation
2 and beyond: if we have to extend it later

Not pretty, I'll admit. So fine to keep as is too if you prefer. But I do foresee more changes in this area, if history is a guide.

Quoted 9 lines

Quoted 6 lines

Even if per-netns, most likely do not want it writable by netns root with user namespaces.

That is the equivalent of ns_capable(CAP_NET_RAW), which is the cause of many crashes due to bad packets ingested by, e.g., a packet socket.

Hmm, yes i get that. Two knobs? a. verify b. the per netns value ro? Both on by default?

Quoted 6 lines

Quoted 3 lines

A single system wide field is simplest.

Is there a benefit to making this configurable on a per netns basis?

As I tried to explain: a specific container breaks, you decide to trust it, you do not need to trust all.

Ok

Quoted 10 lines

Quoted 2 lines

If so, that's fine too. As long as root-in-namespace cannot modify it, so readonly indeed.

Right. So I envision along the lines of:

net.core.virtio_net_hdr_validate_all - validate packets from any netns, netns root can not change it, only host root

net.core.virtio_net_hdr_validate - per netns

Quoted 4 lines

Then skip the global option, as it's just a convenience: the same can be achieved by iterating over all the per-netns settings.

Per netns validation, but only settable by root, not by root-in-namespace.

Hmm I don't know how to achieve that. Are there examples of such options?

Quoted 86 lines · Michael S. Tsirkin

Quoted 35 lines · Willem de Bruijn

Quoted 34 lines · Michael S. Tsirkin

The ingress validation as implemented by the previous patch is stricter than what it replaced: GSO
packets whose checksum metadata matches no transport header are dropped, and non-GSO ones get their checksum completed in place. Such packets used to pass with the metadata unvalidated, and with no overhead.

I judge that this should not affect any deployments, but there is still some risk that it does. In case a deployment depends on them anyway, provide a per-netns switch to get the old behavior back:

echo 0 > /proc/sys/net/core/virtio_net_hdr_csum_check

The default is validating.

Assisted-by LLM

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 Documentation/admin-guide/sysctl/net.rst | 15 +++++++
 include/linux/virtio_net.h               | 23 ++++++++++
 include/net/netns/core.h                 |  1 +
 net/core/dev.c                           |  6 +++
 net/core/net_namespace.c                 |  1 +
 net/core/sysctl_net_core.c               | 54 ++++++++++++++++++++++++
 6 files changed, 100 insertions(+)

Documentation/admin-guide/sysctl/net.rst +0 -0

index e586e17fc7a5..7f230929c37e 100644
@@ -246,6 +246,21 @@ processes are dropped unless socket option SOF_TIMESTAMPING_OPT_TSONLY is set.
 Default: 1 (on)
 
 

Quoted 7 lines

Documentation/admin-guide/sysctl/net.rst processes are dropped unless socket option SOF_TIMESTAMPING_OPT_TSONLY is set. · line 250

Quoted 2 lines

Documentation/admin-guide/sysctl/net.rst +2 -0

+virtio_net_hdr_csum_check
+-------------------------

Make more generic, virtio_net_hdr_validate?

In case we add more validation in the future (as we have in the past), without having to introduce multiple such knobs.

But it's uapi, are you sure we'll be able to change the meaning of an existing knob, even if we keep the name vague?

Quoted 37 lines

Quoted 29 lines

Documentation/admin-guide/sysctl/net.rst +13 -0

+
+When processing packets with a virtio net header (through tun, tap, a
+packet socket or a virtio network device) that request checksum offload
+(VIRTIO_NET_HDR_F_NEEDS_CSUM), validate the packet: complete the checksum
+in software, or drop the packet where the header does not match it.
+
+When disabled, the header is trusted not to corrupt the packet, and the
+checksum is offloaded to hardware (or not verified if it never reaches
+hardware). This is only safe if no protocol is processing the packets.
+
+Default: 1 (on)
+
+
 wmem_default
 ------------
 

include/linux/virtio_net.h +1 -0

index 357115ff0ec2..aa2f8ac4657c 100644
@@ -5,6 +5,7 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/jump_label.h>
 #include <linux/netdevice.h>
 #include <linux/udp.h>
 #include <net/gso.h>

include/linux/virtio_net.h static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb, · line 84

Quoted 6 lines

include/linux/virtio_net.h +3 -0

@@ -78,6 +79,19 @@ static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb,
 	*thoff = nhoff + iph->ihl * sizeof(u32);
 	return iph;
 }
+/* Defined in net/core/dev.c, enabled while any network namespace
+ * has net.core.virtio_net_hdr_csum_check set to 0.
+ */

Should this be a global sysctl, rather than per netns?