[PATCH net v5 0/7] more virtio-net csum hardening

[PATCH net v5 0/7] more virtio-net csum hardening

[Michael S. Tsirkin]

Okay this seems to be converging.... But sashiko did find some issues I
thought are best fixed. Specifically I thought of a better way to handle
short gso in a tunnel - just clear tunnel gso! As a side effect we now
fix up the checksum for these dodgy packets like we did before, instead
of a drop. Good. And more false positives that prompted me to write a
bit more comments. So here goes.

Apologies for reposting so frequently, I know netdev bot will be unhappy
with that.  But Paulos's report made a lot of publicity, there is a lot
of people looking into this, so I feel there's value in defending
against all this class of issues sooner rather than later.

The issues were mostly found by code inspection, but all reproduced by
me in a VM.

------

As reported first by Paulos Yibelo <habte.yibelo@gmail.com> (after which
others, credited below, found more ways the bug can affect users):

allowing untrusted hardware to specify csum start/offset for
CHECKSUM_PARTIAL was not a good idea, because, if it edits the packet
outside the actual checksum, and depending on the setup, the networking
stack could be looking at the packet both before and after the edit, and
seeing different results.

Previously, commit 6785011f8b16 ("ipv4: validate checksum_start before
completing checksum") validated all known instances where this
causes the most grief, but the real fix is to make sure
that either only actual checksum fields are ever edited,
or the checksum is completed before the net stack looks at it,
so it is always consistent.


This basically goes back to
what we did before f43798c27684 ("tun: Allow GSO using virtio_net_hdr"),
except in a narrow list of specified known good cases.


Summary of the patchset:

These are probabably net material (and seem to be too big for stable,
or I would recommend backporting there, too):

  net: account for ingress-resolved virtio net hdr state
	- accounting, not the fix itself, dependency of the fix
  um: vector: trim skbs before verifying the header
	- fixes the um receive path: without it the checksum would be
	  completed over the unreceived tail of the buffer
  virtio_net: parse tunnel csum_start little-endian
	- bugfix, a dependency of validating the header
  net: resolve uncommon partial checksums at ingress
	- fix itself
  net: add a sysctl to opt out of virtio net hdr checksum validation
	- reducing risk of the fix by adding a sysctl to disable it

These are cleanups, can be deferred to net-next, too:

  net: add ip4_parse()
  virtio_net: cleanup protocol calculation
	- cleanup resulting code duplication
	 (kept separate to simplify backporting)

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

Changes since v1:
	- reorder patches to fix bisectability, addressing
	  a comment by Björn Töpel.
	- code refactoring, different patch splitup,
	  a sysctl knob to disable as suggested by Willem de Bruijn.
	- fix a bunch of minor issues spotted by LLMs


Changes since v2:

virtio_net: parse tunnel csum_start little-endian
  - new patch. correctly parsing the header is a dependency
    of validating it, otherwise i do not know what to validate

net: account for ingress-resolved virtio net hdr state
  - make rx_csum_sw counter 64 bit so it won't wrap
    (spotted by LLM review)

net: resolve uncommon partial checksums at ingress
  - tunnel fixes to bugs noticed by Willem de Bruijn and
    Chris Arges
  - restore the csum_offset checks as suggested by Willem

net: add a sysctl to opt out of virtio net hdr checksum validation
  - the knob is no longer writable by root inside a user
    namespace: changing it now requires capable(CAP_NET_ADMIN)
    suggested by Willem de Bruijn

net: add ip4_parse()
  - comment converted to kernel-doc format (Willem de Bruijn)

um: vector: trim skbs before verifying the header
  - new patch, found by code review


Changes since v3:

net: account for ingress-resolved virtio net hdr state
  - um: vector: honor virtio net hdr conversion failures
    folded in (Björn Töpel)
  - count on tap and virtio_net, too (Willem de Bruijn)

net: resolve uncommon partial checksums at ingress
  - walk the frame on loopback, too: loopback_xmit() runs
    eth_type_trans() on it (Willem de Bruijn)
  - return the mismatch handling directly, GSO is implied in
    the tunnel path so drop there outright, and fold the helper
    into its one remaining caller (Willem de Bruijn)
  - read the IP version through struct iphdr (Willem de Bruijn)
  - document the ipv6_find_hdr() flags choice

net: add a sysctl to opt out of virtio net hdr checksum validation
  - renamed to virtio_net_hdr_validate, in case more validation
    is gated by the same knob later (Willem de Bruijn)
  - point .data at the value like the neighboring entries,
    the per-netns registration rebases it, and the handler reads
    the value directly - no ctl_table copy (Björn Töpel,
    Willem de Bruijn)
  - check the opt-out before parsing: with the knob off, packets
    the parser cannot handle were still dropped (Willem de Bruijn)
  - include virtio_net.h in net/core/dev.c so the static key
    definition sees its declaration (build warning reported by
    Jakub Kicinski)

net: add ip4_parse()
  - add the missing kernel-doc Return: section
    (Jakub Kicinski)


Changes since v4:

net: account for ingress-resolved virtio net hdr state
  - count on tun only: dropped the tap, macvtap, ipvtap and
    virtio_net counters (Willem de Bruijn)
  - the driver side branches are in their final form here now,
    moved from "net: resolve uncommon partial checksums at
    ingress" (Willem de Bruijn)

net: resolve uncommon partial checksums at ingress
  - no longer touches the drivers (Willem de Bruijn)
  - a packet too small to be GSO: ignore the tunnel GSO request,
    same as plain GSO ones, and complete wild metadata in place
    (sashiko)
  - document the zero copy interaction (sashiko)
  - honor a preset skb->protocol on ethernet: the stack uses it,
    except on loopback which re-parses the frame (sashiko)

net: add a sysctl to opt out of virtio net hdr checksum validation
  - document the netns lifetime requirement (sashiko)

net: add ip4_parse()
  - pull the whole header, options included, so that the
    transport header offset is within the linear data
    (Willem de Bruijn)

Tested: full suite on the final tree, tunnel included.
um part compiled only.


Michael S. Tsirkin (7):
  net: account for ingress-resolved virtio net hdr state
  um: vector: trim skbs before verifying the header
  virtio_net: parse tunnel csum_start little-endian
  net: resolve uncommon partial checksums at ingress
  net: add a sysctl to opt out of virtio net hdr checksum validation
  net: add ip4_parse()
  virtio_net: cleanup protocol calculation

 Documentation/admin-guide/sysctl/net.rst |  19 ++
 arch/um/drivers/vector_kern.c            |   6 +-
 arch/um/drivers/vector_transports.c      |   4 +-
 drivers/net/tap.c                        |   4 +-
 drivers/net/tun.c                        |  41 +++-
 drivers/net/virtio_net.c                 |   2 +-
 include/linux/ip.h                       |  37 +++
 include/linux/virtio_net.h               | 287 ++++++++++++++++++++++-
 include/net/netns/core.h                 |   1 +
 net/bridge/br_netfilter_hooks.c          |  15 +-
 net/core/dev.c                           |   7 +
 net/core/net_namespace.c                 |   1 +
 net/core/sysctl_net_core.c               |  50 ++++
 net/ipv4/ip_input.c                      |  13 +-
 net/ipv4/netfilter/nf_reject_ipv4.c      |  13 +-
 net/packet/af_packet.c                   |   4 +-
 16 files changed, 461 insertions(+), 43 deletions(-)

-- 
MST

[PATCH net v5 1/7] net: account for ingress-resolved virtio net hdr state

[Michael S. Tsirkin]

The following patch will add ingress resolution of uncommon virtio net
hdr checksum metadata (software checksum completion at ingress) which
adds performance overhead to the packet processing.

We should make it visible, so a hypothetical workload relying on
uncommon metadata can be distinguished from none at all. This way issues
in the parsing will be reported and not quietly pay the host-CPU
checksum overhead instead of offload.

Flip every call to virtio_net_hdr_to_skb to test for negative errno
only.  The um vector raw transport even ignored the return value
altogether: rejected packets went on into the stack with their
metadata unvalidated, so check it there as well.

Have tun count these events per device as rx_csum_sw, exposed via
ethtool -S.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Notes (changelog):
    v4->v5:
        count on tun only: dropped the tap, macvtap, ipvtap and
        virtio_net counters (Willem de Bruijn)

 arch/um/drivers/vector_transports.c |  4 ++-
 drivers/net/tap.c                   |  4 +--
 drivers/net/tun.c                   | 41 +++++++++++++++++++++++++++--
 drivers/net/virtio_net.c            |  2 +-
 net/packet/af_packet.c              |  4 +--
 5 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/arch/um/drivers/vector_transports.c b/arch/um/drivers/vector_transports.c
index e1fb2a76fdff..8b7548458663 100644
--- a/arch/um/drivers/vector_transports.c
+++ b/arch/um/drivers/vector_transports.c
@@ -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;
 }
 
diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index ff67d99deb39..73e883394446 100644
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -649,7 +649,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
 
 	if (vnet_hdr_len) {
 		err = tun_vnet_hdr_to_skb(q->flags, skb, &vnet_hdr);
-		if (err) {
+		if (err < 0) {
 			rcu_read_unlock();
 			drop_reason = SKB_DROP_REASON_DEV_HDR;
 			goto err_kfree;
@@ -1084,7 +1084,7 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
 
 	if (vnet_hdr_len) {
 		err = tun_vnet_hdr_to_skb(q->flags, skb, gso);
-		if (err) {
+		if (err < 0) {
 			rcu_read_unlock();
 			goto err_kfree;
 		}
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 242899f7fd07..197fb8726f8d 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -210,6 +210,7 @@ struct tun_struct {
 	u32 flow_count;
 	u32 rx_batched;
 	atomic_long_t rx_frame_errors;
+	atomic64_t rx_csum_sw;
 	struct bpf_prog __rcu *xdp_prog;
 	struct tun_prog __rcu *steering_prog;
 	struct tun_prog __rcu *filter_prog;
@@ -1935,10 +1936,14 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 		break;
 	}
 
-	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
+	err = tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr);
+	if (err < 0) {
 		atomic_long_inc(&tun->rx_frame_errors);
 		err = -EINVAL;
 		goto free_skb;
+	} else if (err > 0) {
+		atomic64_inc(&tun->rx_csum_sw);
+		err = 0;
 	}
 
 	if ((tun->flags & TUN_TYPE_MASK) == IFF_TAP)
@@ -2605,11 +2610,15 @@ static int tun_xdp_one(struct tun_struct *tun,
 	tnl_hdr = (struct virtio_net_hdr_v1_hash_tunnel *)gso;
 	skb->dev = tun->dev;
 	skb_set_network_header(skb, ETH_HLEN);
-	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, tnl_hdr)) {
+	ret = tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, tnl_hdr);
+	if (ret < 0) {
 		atomic_long_inc(&tun->rx_frame_errors);
 		kfree_skb(skb);
 		ret = -EINVAL;
 		goto out;
+	} else if (ret > 0) {
+		atomic64_inc(&tun->rx_csum_sw);
+		ret = 0;
 	}
 
 	skb->protocol = eth_type_trans(skb, tun->dev);
@@ -3739,7 +3748,35 @@ static void tun_get_channels(struct net_device *dev,
 	channels->max_combined = tun->flags & IFF_MULTI_QUEUE ? MAX_TAP_QUEUES : 1;
 }
 
+#define TUN_STATS_LEN	1
+static const char tun_ethtool_strings[TUN_STATS_LEN][ETH_GSTRING_LEN] = {
+	"rx_csum_sw",
+};
+
+static int tun_get_sset_count(struct net_device *dev, int sset)
+{
+	return sset == ETH_SS_STATS ? TUN_STATS_LEN : -EOPNOTSUPP;
+}
+
+static void tun_get_strings(struct net_device *dev, u32 stringset, u8 *data)
+{
+	if (stringset == ETH_SS_STATS)
+		memcpy(data, tun_ethtool_strings,
+		       sizeof(tun_ethtool_strings));
+}
+
+static void tun_get_ethtool_stats(struct net_device *dev,
+				  struct ethtool_stats *stats, u64 *data)
+{
+	struct tun_struct *tun = netdev_priv(dev);
+
+	data[0] = atomic64_read(&tun->rx_csum_sw);
+}
+
 static const struct ethtool_ops tun_ethtool_ops = {
+	.get_sset_count		= tun_get_sset_count,
+	.get_strings		= tun_get_strings,
+	.get_ethtool_stats	= tun_get_ethtool_stats,
 	.supported_coalesce_params = ETHTOOL_COALESCE_RX_MAX_FRAMES,
 	.get_drvinfo	= tun_get_drvinfo,
 	.get_msglevel	= tun_get_msglevel,
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index daab43ac92ce..2e6fd81df9c4 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2519,7 +2519,7 @@ static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *
 	skb_set_network_header(skb, ETH_HLEN);
 	if (virtio_net_hdr_tnl_to_skb(skb, &hdr->tnl_hdr, vi->rx_tnl,
 				      vi->rx_tnl_csum,
-				      virtio_is_little_endian(vi->vdev))) {
+				      virtio_is_little_endian(vi->vdev)) < 0) {
 		net_warn_ratelimited("%s: bad gso: type: %x, size: %u, flags %x tunnel %d tnl csum %d\n",
 				     dev->name, hdr->hdr.gso_type,
 				     hdr->hdr.gso_size, hdr->hdr.flags,
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 52ae3d875e97..e6aec9ecfa99 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2913,7 +2913,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 		}
 
 		if (has_vnet_hdr) {
-			if (virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le())) {
+			if (virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le()) < 0) {
 				tp_len = -EINVAL;
 				goto tpacket_error;
 			}
@@ -3136,7 +3136,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 
 	if (vnet_hdr_sz) {
 		err = virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le());
-		if (err)
+		if (err < 0)
 			goto out_free;
 		len += vnet_hdr_sz;
 	}
-- 
MST

Re: [PATCH net v5 1/7] net: account for ingress-resolved virtio net hdr state

[Willem de Bruijn]

Michael S. Tsirkin wrote:
> The following patch will add ingress resolution of uncommon virtio net
> hdr checksum metadata (software checksum completion at ingress) which
> adds performance overhead to the packet processing.
> 
> We should make it visible, so a hypothetical workload relying on
> uncommon metadata can be distinguished from none at all. This way issues
> in the parsing will be reported and not quietly pay the host-CPU
> checksum overhead instead of offload.
> 
> Flip every call to virtio_net_hdr_to_skb to test for negative errno
> only.  The um vector raw transport even ignored the return value
> altogether: rejected packets went on into the stack with their
> metadata unvalidated, so check it there as well.
> 
> Have tun count these events per device as rx_csum_sw, exposed via
> ethtool -S.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH net v5 1/7] net: account for ingress-resolved virtio net hdr state

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The concern: the newly checked virtio_net_hdr_to_skb() call in
  raw_verify_header() (arch/um/drivers/vector_transports.c:213) runs on…
- [Low] This patch quietly includes a real behaviour fix for the UML
  vector raw transport.
- [Low] The concern: at this commit, virtio_net_hdr_to_skb(),
  __virtio_net_hdr_to_skb() and virtio_net_hdr_tnl_to_skb() in…

--

net: account for ingress-resolved virtio net hdr state

This commit changes callers of virtio_net_hdr_to_skb() and
virtio_net_hdr_tnl_to_skb() to treat only negative return values as errors.
It prepares for a later patch that returns a positive value when the
checksum is completed in software. It also makes the UML vector raw
transport check the return value, and adds a tun ethtool -S counter,
rx_csum_sw, for the positive case.

> diff --git a/arch/um/drivers/vector_transports.c b/arch/um/drivers/vector_transports.c
> index e1fb2a76fdff5..8b75484586635 100644
> --- a/arch/um/drivers/vector_transports.c
> +++ b/arch/um/drivers/vector_transports.c
> @@ -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;
>  }

[Severity: Medium]
Can this drop valid frames at this point in the series? The skb passed in
here has not been trimmed to the received length yet. prep_skb() sizes it
to the whole receive buffer:

arch/um/drivers/vector_kern.c:prep_skb() {
    ...
	skb_put(result, vp->max_packet);
	result->data_len = len - vp->max_packet;
	result->len += len - vp->max_packet;
    ...
}

Both vector_legacy_rx() and vector_mmsg_rx() call verify_header() before
pskb_trim():

arch/um/drivers/vector_kern.c:vector_legacy_rx() {
    ...
				header_check = vp->verify_header(
					vp->header_rxbuffer, skb, vp);
				if (header_check < 0) {
    ...
			pskb_trim(skb, pkt_len - vp->rx_header_size);
    ...
}

That means the length checks in __virtio_net_hdr_to_skb() compare against
the buffer size (65536 with GSO), not the packet length. Take the USO check:

	if (skb->len - p_off > gso_size * UDP_MAX_SEGMENTS)
		return -EINVAL;

With a 64K buffer, this rejects a valid USO frame whose gso_size is below
about 511. Before this patch the error was ignored. Now raw_verify_header()
returns -1, and the caller frees the skb and increments rx_encaps_errors.

The same ordering also lets csum_start/csum_offset values that point past
the real packet, but still inside the buffer, pass validation.

The next patch in the series, "um: vector: trim skbs before verifying the
header", moves pskb_trim() ahead of verify_header() in both RX paths. That
fixes this, so only intermediate bisect points are affected. Could the trim
come before this change, or be folded into it?

[Severity: Low]
This hunk looks like a real behaviour fix for the UML raw transport, not
accounting preparation. The commit message says:

    The um vector raw transport even ignored the return value
    altogether: rejected packets went on into the stack with their
    metadata unvalidated, so check it there as well.

The return value has been ignored since commit 49da7e64f33e ("High
Performance UML Vector Network Driver").

For example, __virtio_net_hdr_to_skb() can call skb_partial_csum_set(),
which sets CHECKSUM_PARTIAL along with csum_start/csum_offset, and then
return -EINVAL from a later check. The old code still delivered that skb.

Should this be a separate UML patch, ordered together with the trim fix
from "um: vector: trim skbs before verifying the header", and carrying:

Fixes: 49da7e64f33e ("High Performance UML Vector Network Driver")

Right now the fix sits in a patch whose subject reads as accounting-only
and which also adds a new tun ethtool string. That makes it hard to pick
up cleanly for stable.

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 7cca1a6f72ff4..133923317eae8 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c

[ ... ]

> @@ -1939,10 +1940,14 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  		break;
>  	}
>  
> -	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
> +	err = tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr);
> +	if (err < 0) {
>  		atomic_long_inc(&tun->rx_frame_errors);
>  		err = -EINVAL;
>  		goto free_skb;
> +	} else if (err > 0) {
> +		atomic64_inc(&tun->rx_csum_sw);
> +		err = 0;
>  	}

[Severity: Low]
Can err be positive here at this commit? At this revision,
virtio_net_hdr_to_skb(), __virtio_net_hdr_to_skb() and
virtio_net_hdr_tnl_to_skb() in include/linux/virtio_net.h appear to return
only 0 or -EINVAL.

If so, this branch and the matching ret > 0 branch in tun_xdp_one() can't
be reached. The new rx_csum_sw value reported by tun_get_ethtool_stats()
would always read 0.

The later patch "net: resolve uncommon partial checksums at ingress"
documents the tri-state return and adds
__virtio_net_resolve_csum_partial_mismatch(), which returns
skb_checksum_help(skb) ?: 1. That makes these branches live at the end of
the series, so this only affects intermediate bisect points.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.v5.git.mst%40redhat.com

[PATCH net v5 2/7] um: vector: trim skbs before verifying the header

[Michael S. Tsirkin]

The vector receive path over-allocates the skb and only trims it
to the received length after the transport header is verified.

A follow up patch will complete the packet checksum in software within
virtio_net_hdr_to_skb so we need the packet trimmed (otherwise the
checksum would be computed incorrectly over the unreceived tail of the
buffer).

Trim first, so the import sees the packet as it was received.

Reviewed-by: Willem de Bruijn <willemb@google.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 arch/um/drivers/vector_kern.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c
index 8a70b3a62553..1afbef78df72 100644
--- a/arch/um/drivers/vector_kern.c
+++ b/arch/um/drivers/vector_kern.c
@@ -865,6 +865,7 @@ static int vector_legacy_rx(struct vector_private *vp)
 
 	if (skb != NULL) {
 		if (pkt_len > vp->header_size) {
+			pskb_trim(skb, pkt_len - vp->rx_header_size);
 			if (vp->header_size > 0) {
 				header_check = vp->verify_header(
 					vp->header_rxbuffer, skb, vp);
@@ -879,7 +880,6 @@ static int vector_legacy_rx(struct vector_private *vp)
 					skb->ip_summed = CHECKSUM_UNNECESSARY;
 				}
 			}
-			pskb_trim(skb, pkt_len - vp->rx_header_size);
 			skb->protocol = eth_type_trans(skb, skb->dev);
 			vp->dev->stats.rx_bytes += skb->len;
 			vp->dev->stats.rx_packets++;
@@ -981,6 +981,8 @@ static int vector_mmsg_rx(struct vector_private *vp, int budget)
 	for (i = 0; i < packet_count; i++) {
 		skb = (*skbuff_vector);
 		if (mmsg_vector->msg_len > vp->header_size) {
+			pskb_trim(skb,
+				mmsg_vector->msg_len - vp->rx_header_size);
 			if (vp->header_size > 0) {
 				header_check = vp->verify_header(
 					mmsg_vector->msg_hdr.msg_iov->iov_base,
@@ -1005,8 +1007,6 @@ static int vector_mmsg_rx(struct vector_private *vp, int budget)
 					skb->ip_summed = CHECKSUM_UNNECESSARY;
 				}
 			}
-			pskb_trim(skb,
-				mmsg_vector->msg_len - vp->rx_header_size);
 			skb->protocol = eth_type_trans(skb, skb->dev);
 			/*
 			 * We do not need to lock on updating stats here
-- 
MST

Re: [PATCH net v5 2/7] um: vector: trim skbs before verifying the header

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 (1) · Low: 1 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In vector_mmsg_rx() (arch/um/drivers/vector_kern.c),
  pskb_trim(skb, ...) now runs before verify_header(), and the skb is…
- [Low] The commit message presents the reorder only as groundwork for
  the later commit "net: resolve uncommon partial checksums at ingress".

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  touches these exact lines.
- [Medium] This problem wasn't introduced by this patch, but
  vector_legacy_rx() sets the receive header iovec to vp->header_size…
- [Low] This problem wasn't introduced by this patch, but
  vector_legacy_rx() leaks the skb that prep_skb() allocated when…

--

um: vector: trim skbs before verifying the header

This patch moves the pskb_trim() call in vector_legacy_rx() and
vector_mmsg_rx() so that it runs before verify_header(). It prepares for a
follow-up change, in which virtio_net_hdr_to_skb() will complete the
checksum in software and will need the skb trimmed to the received length.

> Trim first, so the import sees the packet as it was received.

[Severity: Low]
Should the commit message say that this reorder also changes current
validation behaviour, and not only prepare for the follow-up?

raw_verify_header() calls virtio_net_hdr_to_skb(). Today,
__virtio_net_hdr_to_skb() already checks lengths against skb->len and
skb_headlen() in several places:

  skb_partial_csum_set()
  pskb_may_pull(skb, p_off)
  the UDP_L4 bound against gso_size * UDP_MAX_SEGMENTS
  the skb->len - nh_off > gso_size test that decides whether gso_size
  and SKB_GSO_DODGY get set

Before this patch, those checks ran against the untrimmed prep_skb()
buffer (max_packet plus frags). A vnet header whose csum_start/csum_offset
pointed past the received data, but still inside the buffer, used to pass.
The later trim then left a CHECKSUM_PARTIAL skb whose checksum location
was beyond skb->len.

With the reorder, such headers are rejected and counted in
rx_encaps_errors. Could the changelog describe this? There is also no
Fixes: tag.

> diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c
> index 8a70b3a625537..1afbef78df725 100644
> --- a/arch/um/drivers/vector_kern.c
> +++ b/arch/um/drivers/vector_kern.c
> @@ -865,6 +865,7 @@ static int vector_legacy_rx(struct vector_private *vp)
>  
>  	if (skb != NULL) {

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch. Does
vector_legacy_rx() leak the skb from prep_skb() when uml_vector_recvmsg()
fails?

Just above this hunk:

	pkt_len = uml_vector_recvmsg(vp->fds->rx_fd, &hdr, 0);
	if (pkt_len < 0) {
		vp->in_error = true;
		return pkt_len;
	}

uml_vector_recvmsg() returns -errno for any readv() failure other than
EAGAIN. This path then returns without calling dev_kfree_skb_irq(skb).
Nothing else holds a reference, so the skb and its frags are lost.

>  		if (pkt_len > vp->header_size) {
> +			pskb_trim(skb, pkt_len - vp->rx_header_size);
>  			if (vp->header_size > 0) {
>  				header_check = vp->verify_header(
>  					vp->header_rxbuffer, skb, vp);

[Severity: Medium]
This isn't a bug introduced by this patch, but is the header iovec in
vector_legacy_rx() the right size for IPv4 GRE and raw IPv4 L2TPv3?

Earlier in vector_legacy_rx():

	if (vp->header_size > 0) {
		iov[0].iov_base = vp->header_rxbuffer;
		iov[0].iov_len = vp->header_size;
	}

build_gre_transport_data() sets:

	vp->rx_header_size = vp->header_size;
	if (!td->ipv6)
		vp->rx_header_size += sizeof(struct iphdr);

build_l2tpv3_transport_data() does the same when
(!td->ipv6) && (!td->udp).

gre_verify_header() skips sizeof(struct iphdr) into header_rxbuffer
before it compares. With vec=0, readv() only puts header_size bytes (the
start of the IPv4 header) into header_rxbuffer. The verifier then reads
uninitialized kmalloc memory, so nearly all packets get dropped.

For any packet that does pass, the rest of the IP and encapsulation
header stays at the front of the skb. The trim by rx_header_size then
removes 20 bytes of real payload from the tail.

[ ... ]

> @@ -981,6 +981,8 @@ static int vector_mmsg_rx(struct vector_private *vp, int budget)
>  	for (i = 0; i < packet_count; i++) {
>  		skb = (*skbuff_vector);
>  		if (mmsg_vector->msg_len > vp->header_size) {
> +			pskb_trim(skb,
> +				mmsg_vector->msg_len - vp->rx_header_size);

[Severity: High]
Can skb be NULL here?

If the GFP_ATOMIC alloc_skb_with_frags() in prep_skb() fails, the
skbuff_vector slot stays NULL, but the header iovec is still active:

	if (vp->header_size > 0)
		iov_index++;
	if (result == NULL) {
		iov[iov_index].iov_base = NULL;
		iov[iov_index].iov_len = 0;
		goto done;
	}
	...
done:
	msg->msg_iovlen = iov_index;

vector_net_open() sizes the rx queue header buffers with
vp->rx_header_size through create_queue(). For IPv4 GRE and non-UDP IPv4
L2TPv3, rx_header_size is header_size + sizeof(struct iphdr). A packet
that fills only the header iovec therefore gives
msg_len == rx_header_size > header_size.

That packet enters this branch and calls pskb_trim(NULL, 0), which
dereferences skb->len.

Before this patch, gre_verify_header() or l2tpv3_verify_header() ran
first. Neither touches the skb. A packet that failed them went to
dev_kfree_skb_irq(NULL), which returns early. Only packets with a valid
header reached the old pskb_trim() on a NULL skb.

Now a packet with the wrong GRE key or the wrong L2TPv3 session/cookie
can also hit the NULL dereference whenever an rx skb allocation has
failed. The GRE and L2TPv3 raw sockets are bound but not connected, so
any remote host can send such packets.

The else branch already checks skb != NULL. Is a similar check needed
before this pskb_trim()?

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but
the patch touches these lines. Is this guard strict enough for the trim
length?

The guard checks msg_len > vp->header_size, but the trim subtracts
vp->rx_header_size. Two configurations have rx_header_size larger than
header_size:

  IPv4 GRE: header_size 4 and rx_header_size 24 without a key, or 8 and
  28 with a key
  raw IPv4 L2TPv3

In these cases, a msg_len between header_size and rx_header_size makes
the difference negative. pskb_trim() takes it as a very large unsigned
int:

	return (len < skb->len) ? __pskb_trim(skb, len) : 0;

and returns 0 without trimming. recvmmsg() only wrote into the header
iovec, so the skb keeps its full prep_skb() length of unwritten
alloc_skb_with_frags() memory. That memory goes through eth_type_trans()
and napi_gro_receive() into the guest stack.

gre_verify_header() and l2tpv3_verify_header() also read bytes past
msg_len in the per-slot header buffer. Those bytes can still hold an
earlier packet's header.

For example, a keyed GRE packet with msg_len 24 supplies only the 4 GRE
flag bytes. The key at offsets 24..27 is left over from the last valid
packet, so the key check passes without the sender knowing the key.

When msg_len == rx_header_size, the skb is trimmed to 0 bytes and
eth_type_trans() parses uninitialized head data.

vector_legacy_rx() has the same mismatch:

	if (pkt_len > vp->header_size) {
		pskb_trim(skb, pkt_len - vp->rx_header_size);

Should both paths require the received length to be larger than
rx_header_size (plus ETH_HLEN) before trimming?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.v5.git.mst%40redhat.com

[PATCH net v5 3/7] virtio_net: parse tunnel csum_start little-endian

[Michael S. Tsirkin]

All tunnel related virtio features are > 32, so none of this
functionality can be enabled on legacy mixed-endian device.

Accordingly, all tunnel header fields, csum_start included, are
little-endian.

The nested virtio net header import parses csum_start that way, but the
inner transport offset is parsed with the endianness specified by the
interface.

If tun is misconfigured to legacy big-endian mode while the tunnel
features are enabled, the two parses of this field disagree, and the
value stored in skb->inner_transport_header - which is only
bounds-checked from below - can point past the end of the packet, where
GSO segmentation later dereferences it.

To fix, parse the field little-endian in both places.

Fixes: a2fb4bc4e2a6 ("net: implement virtio helpers to handle UDP GSO tunneling.")
Cc: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/linux/virtio_net.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index d6466f96cdd0..013c6c7e2dea 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -324,7 +324,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	inner_l3min = virtio_l3min(inner_isv6);
 	outer_l3min = ETH_HLEN + virtio_l3min(outer_isv6);
 
-	inner_th = __virtio16_to_cpu(little_endian, hdr->csum_start);
+	inner_th = __virtio16_to_cpu(true, hdr->csum_start);
 	inner_nh = le16_to_cpu(vhdr->inner_nh_offset);
 	outer_th = le16_to_cpu(vhdr->outer_th_offset);
 	if (outer_th < outer_l3min ||
-- 
MST

[PATCH net v5 4/7] net: resolve uncommon partial checksums at ingress

[Michael S. Tsirkin]

As reported first by Paulos Yibelo <habte.yibelo@gmail.com> (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

Note that for zero copy, userspace can change the data after the
checksum has been calculated.  This is fundamental to zero copy by
design, and the only outcome is a bad checksum and a packet drop.

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.

Reported-by: Paulos Yibelo <habte.yibelo@gmail.com>
Reported-by: Chris Arges <carges@cloudflare.com>
Tested-by: Chris Arges <carges@cloudflare.com>
Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Notes (changelog):
    v4->v5:
        caller changes moved to "account for ingress-resolved
        virtio net hdr state" (Willem de Bruijn)
        a packet too small to be GSO: ignore the tunnel GSO request,
        complete wild metadata in place (sashiko)
        document the zero copy interaction (sashiko)
        honor a preset skb->protocol on ethernet: the stack uses it,
        except on loopback which re-parses the frame (sashiko)

 include/linux/virtio_net.h | 283 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 279 insertions(+), 4 deletions(-)

diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 013c6c7e2dea..52852a33a2db 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -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,205 @@ 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)
+{
+	/* skb->dev is only guaranteed to be set since 44378d02c5aa
+	 * ("net: always dissect GSO packets in
+	 * __virtio_net_hdr_to_skb()"), do not crash when it is not.
+	 */
+	if (WARN_ON_ONCE(!skb->dev))
+		return false;
+
+	/* A preset protocol is what the stack will use, except on
+	 * loopback which re-parses the frame: loopback_xmit() calls
+	 * eth_type_trans() on it.
+	 */
+	if (skb->dev->type == ARPHRD_ETHER ||
+	    skb->dev->type == ARPHRD_LOOPBACK) {
+		if (!pskb_may_pull(skb, ETH_HLEN))
+			return false;
+		*nhoff = ETH_HLEN;
+		*proto = skb->protocol;
+		if (!*proto || skb->dev->type == ARPHRD_LOOPBACK)
+			*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);
+		/* An unset network header shows up as an offset
+		 * outside of the packet.
+		 */
+		if (*nhoff < 0 || (unsigned int)*nhoff >= skb->len)
+			return false;
+		*proto = skb->protocol;
+		if (!*proto) {
+			const struct iphdr *iph;
+
+			/* A packet socket bound to ETH_P_ALL leaves the
+			 * protocol unset: infer it from the IP version
+			 * at the network header.
+			 */
+			if (!pskb_may_pull(skb, *nhoff + 1))
+				return false;
+			iph = (const struct iphdr *)(skb->data + *nhoff);
+			if (iph->version == 4)
+				*proto = htons(ETH_P_IP);
+			else if (iph->version == 6)
+				*proto = htons(ETH_P_IPV6);
+		}
+	}
+	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;
+			/* Find the transport header behind any
+			 * extension headers, AH included.
+			 */
+			*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.
+ */
+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)
@@ -194,11 +397,35 @@ 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;
+
+	return __virtio_net_resolve_csum_partial_mismatch(skb);
 }
 
 /* This function must be called after virtio_net_hdr_from_skb(). */
@@ -282,6 +509,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 +549,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;
 
@@ -335,9 +592,12 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	/* 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;
 
+	if (!virtio_net_tnl_offsets_ok(skb, outer_isv6, outer_th))
+		return -EINVAL;
+
 	/* In case of USO, the inner protocol is still unknown and
 	 * `inner_isv6` is just a guess, additional parsing is needed.
 	 * The previous validation ensures that accessing an ipv4 inner
@@ -353,7 +613,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 +628,22 @@ 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;
+
+	if (!skb_is_gso(skb))
+		skb_shinfo(skb)->gso_type &= ~(SKB_GSO_UDP_TUNNEL_CSUM |
+					       SKB_GSO_UDP_TUNNEL);
+
+	/*
+	 * 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;
+
+	/* A tunnel header implies GSO, so drop. */
+	return __virtio_net_resolve_csum_partial_mismatch(skb);
 }
 
 /* Checksum-related fields validation for the driver */
-- 
MST

Re: [PATCH net v5 4/7] net: resolve uncommon partial checksums at ingress

[Willem de Bruijn]

Michael S. Tsirkin wrote:
> As reported first by Paulos Yibelo <habte.yibelo@gmail.com> (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
> 
> Note that for zero copy, userspace can change the data after the
> checksum has been calculated.  This is fundamental to zero copy by
> design, and the only outcome is a bad checksum and a packet drop.
> 
> 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.
> 
> Reported-by: Paulos Yibelo <habte.yibelo@gmail.com>
> Reported-by: Chris Arges <carges@cloudflare.com>
> Tested-by: Chris Arges <carges@cloudflare.com>
> Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
> Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> Notes (changelog):
>     v4->v5:
>         caller changes moved to "account for ingress-resolved
>         virtio net hdr state" (Willem de Bruijn)
>         a packet too small to be GSO: ignore the tunnel GSO request,
>         complete wild metadata in place (sashiko)
>         document the zero copy interaction (sashiko)
>         honor a preset skb->protocol on ethernet: the stack uses it,
>         except on loopback which re-parses the frame (sashiko)
> 
>  include/linux/virtio_net.h | 283 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 279 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 013c6c7e2dea..52852a33a2db 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -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,205 @@ 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;
> +

Pulling again if ihl != 0 should probably have been here too.

Else we have to verify that all callers safely pull. Note that
iph will become stale after another pull.

> +	*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)
> +{
> +	/* skb->dev is only guaranteed to be set since 44378d02c5aa
> +	 * ("net: always dissect GSO packets in
> +	 * __virtio_net_hdr_to_skb()"), do not crash when it is not.
> +	 */
> +	if (WARN_ON_ONCE(!skb->dev))
> +		return false;
> +
> +	/* A preset protocol is what the stack will use, except on
> +	 * loopback which re-parses the frame: loopback_xmit() calls
> +	 * eth_type_trans() on it.
> +	 */
> +	if (skb->dev->type == ARPHRD_ETHER ||
> +	    skb->dev->type == ARPHRD_LOOPBACK) {
> +		if (!pskb_may_pull(skb, ETH_HLEN))
> +			return false;
> +		*nhoff = ETH_HLEN;
> +		*proto = skb->protocol;

Why this change now?

skb->protocol cannot be trusted (to be consistent with actual packet
content).

Express goal is to take the actual packet content instead of relying
on this spoofable field.

On top of that there are issues around setting protocol to IP and
inserting VLAN headers. That would skip the eth_type_vlan branch.

IMHO, we should only use skb->protocol if no other source to detect
protocol exist, i.e., if no L2 header exists.

Also, instead of preferring one over the other, we could actually
read both and

    if (skb->protocol &&
        skb->protocol != eth_hdr(skb)->h_proto)
            return false;


> +		if (!*proto || skb->dev->type == ARPHRD_LOOPBACK)
> +			*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);
> +		/* An unset network header shows up as an offset
> +		 * outside of the packet.
> +		 */
> +		if (*nhoff < 0 || (unsigned int)*nhoff >= skb->len)
> +			return false;
> +		*proto = skb->protocol;

Same

> +		if (!*proto) {
> +			const struct iphdr *iph;
> +
> +			/* A packet socket bound to ETH_P_ALL leaves the
> +			 * protocol unset: infer it from the IP version
> +			 * at the network header.
> +			 */
> +			if (!pskb_may_pull(skb, *nhoff + 1))
> +				return false;
> +			iph = (const struct iphdr *)(skb->data + *nhoff);
> +			if (iph->version == 4)
> +				*proto = htons(ETH_P_IP);
> +			else if (iph->version == 6)
> +				*proto = htons(ETH_P_IPV6);
> +		}
> +	}
> +	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;
> +			/* Find the transport header behind any
> +			 * extension headers, AH included.
> +			 */
> +			*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.
> + */
> +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)
> @@ -194,11 +397,35 @@ 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;
> +
> +	return __virtio_net_resolve_csum_partial_mismatch(skb);
>  }
>  
>  /* This function must be called after virtio_net_hdr_from_skb(). */
> @@ -282,6 +509,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 +549,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;
>  
> @@ -335,9 +592,12 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
>  	/* 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;
>  
> +	if (!virtio_net_tnl_offsets_ok(skb, outer_isv6, outer_th))
> +		return -EINVAL;
> +
>  	/* In case of USO, the inner protocol is still unknown and
>  	 * `inner_isv6` is just a guess, additional parsing is needed.
>  	 * The previous validation ensures that accessing an ipv4 inner
> @@ -353,7 +613,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 +628,22 @@ 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;
> +
> +	if (!skb_is_gso(skb))
> +		skb_shinfo(skb)->gso_type &= ~(SKB_GSO_UDP_TUNNEL_CSUM |
> +					       SKB_GSO_UDP_TUNNEL);
> +
> +	/*
> +	 * 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);

on second thought, if having to respin, might as well fold the trivial
fix in patch 7 in here from the start.

> +	if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
> +						 inner_proto))
> +		return 0;
> +
> +	/* A tunnel header implies GSO, so drop. */

no longer true for the above !skb_is_gso(skb) case

> +	return __virtio_net_resolve_csum_partial_mismatch(skb);
>  }
>  
>  /* Checksum-related fields validation for the driver */
> -- 
> MST
> 

Re: [PATCH net v5 4/7] net: resolve uncommon partial checksums at ingress

[Michael S. Tsirkin]

On Thu, Oct 22, 2026 at 10:52:25AM -0400, Willem de Bruijn wrote:
> Michael S. Tsirkin wrote:
> > As reported first by Paulos Yibelo <habte.yibelo@gmail.com> (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
> > 
> > Note that for zero copy, userspace can change the data after the
> > checksum has been calculated.  This is fundamental to zero copy by
> > design, and the only outcome is a bad checksum and a packet drop.
> > 
> > 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.
> > 
> > Reported-by: Paulos Yibelo <habte.yibelo@gmail.com>
> > Reported-by: Chris Arges <carges@cloudflare.com>
> > Tested-by: Chris Arges <carges@cloudflare.com>
> > Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
> > Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > 
> > Notes (changelog):
> >     v4->v5:
> >         caller changes moved to "account for ingress-resolved
> >         virtio net hdr state" (Willem de Bruijn)
> >         a packet too small to be GSO: ignore the tunnel GSO request,
> >         complete wild metadata in place (sashiko)
> >         document the zero copy interaction (sashiko)
> >         honor a preset skb->protocol on ethernet: the stack uses it,
> >         except on loopback which re-parses the frame (sashiko)
> > 
> >  include/linux/virtio_net.h | 283 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 279 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > index 013c6c7e2dea..52852a33a2db 100644
> > --- a/include/linux/virtio_net.h
> > +++ b/include/linux/virtio_net.h
> > @@ -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,205 @@ 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;
> > +
> 
> Pulling again if ihl != 0 should probably have been here too.

oh right you told me. I forgot.

> Else we have to verify that all callers safely pull. Note that
> iph will become stale after another pull.

right.

> > +	*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)
> > +{
> > +	/* skb->dev is only guaranteed to be set since 44378d02c5aa
> > +	 * ("net: always dissect GSO packets in
> > +	 * __virtio_net_hdr_to_skb()"), do not crash when it is not.
> > +	 */
> > +	if (WARN_ON_ONCE(!skb->dev))
> > +		return false;
> > +
> > +	/* A preset protocol is what the stack will use, except on
> > +	 * loopback which re-parses the frame: loopback_xmit() calls
> > +	 * eth_type_trans() on it.
> > +	 */
> > +	if (skb->dev->type == ARPHRD_ETHER ||
> > +	    skb->dev->type == ARPHRD_LOOPBACK) {
> > +		if (!pskb_may_pull(skb, ETH_HLEN))
> > +			return false;
> > +		*nhoff = ETH_HLEN;
> > +		*proto = skb->protocol;
> 
> Why this change now?


I keep going back and forth about which to use where... however ...


> skb->protocol cannot be trusted (to be consistent with actual packet
> content).
> 
> Express goal is to take the actual packet content instead of relying
> on this spoofable field.
> 
> On top of that there are issues around setting protocol to IP and
> inserting VLAN headers. That would skip the eth_type_vlan branch.
> 
> IMHO, we should only use skb->protocol if no other source to detect
> protocol exist, i.e., if no L2 header exists.
> 
> Also, instead of preferring one over the other, we could actually
> read both and
> 
>     if (skb->protocol &&
>         skb->protocol != eth_hdr(skb)->h_proto)
>             return false;


That's best. I like that.


> 
> > +		if (!*proto || skb->dev->type == ARPHRD_LOOPBACK)
> > +			*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);
> > +		/* An unset network header shows up as an offset
> > +		 * outside of the packet.
> > +		 */
> > +		if (*nhoff < 0 || (unsigned int)*nhoff >= skb->len)
> > +			return false;
> > +		*proto = skb->protocol;
> 
> Same
> 
> > +		if (!*proto) {
> > +			const struct iphdr *iph;
> > +
> > +			/* A packet socket bound to ETH_P_ALL leaves the
> > +			 * protocol unset: infer it from the IP version
> > +			 * at the network header.
> > +			 */
> > +			if (!pskb_may_pull(skb, *nhoff + 1))
> > +				return false;
> > +			iph = (const struct iphdr *)(skb->data + *nhoff);
> > +			if (iph->version == 4)
> > +				*proto = htons(ETH_P_IP);
> > +			else if (iph->version == 6)
> > +				*proto = htons(ETH_P_IPV6);
> > +		}
> > +	}
> > +	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;
> > +			/* Find the transport header behind any
> > +			 * extension headers, AH included.
> > +			 */
> > +			*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.
> > + */
> > +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)
> > @@ -194,11 +397,35 @@ 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;
> > +
> > +	return __virtio_net_resolve_csum_partial_mismatch(skb);
> >  }
> >  
> >  /* This function must be called after virtio_net_hdr_from_skb(). */
> > @@ -282,6 +509,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 +549,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;
> >  
> > @@ -335,9 +592,12 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> >  	/* 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;
> >  
> > +	if (!virtio_net_tnl_offsets_ok(skb, outer_isv6, outer_th))
> > +		return -EINVAL;
> > +
> >  	/* In case of USO, the inner protocol is still unknown and
> >  	 * `inner_isv6` is just a guess, additional parsing is needed.
> >  	 * The previous validation ensures that accessing an ipv4 inner
> > @@ -353,7 +613,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 +628,22 @@ 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;
> > +
> > +	if (!skb_is_gso(skb))
> > +		skb_shinfo(skb)->gso_type &= ~(SKB_GSO_UDP_TUNNEL_CSUM |
> > +					       SKB_GSO_UDP_TUNNEL);
> > +
> > +	/*
> > +	 * 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);
> 
> on second thought, if having to respin, might as well fold the trivial
> fix in patch 7 in here from the start.

sure.

> > +	if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
> > +						 inner_proto))
> > +		return 0;
> > +
> > +	/* A tunnel header implies GSO, so drop. */
> 
> no longer true for the above !skb_is_gso(skb) case

indeed.

> > +	return __virtio_net_resolve_csum_partial_mismatch(skb);
> >  }
> >  
> >  /* Checksum-related fields validation for the driver */
> > -- 
> > MST
> > 
> 

Re: [PATCH net v5 4/7] net: resolve uncommon partial checksums at ingress

[Chris Arges]

On 2026-10-22 10:52:25, Willem de Bruijn wrote:
> Michael S. Tsirkin wrote:
> > As reported first by Paulos Yibelo <habte.yibelo@gmail.com> (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
> > 
> > Note that for zero copy, userspace can change the data after the
> > checksum has been calculated.  This is fundamental to zero copy by
> > design, and the only outcome is a bad checksum and a packet drop.
> > 
> > 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.
> > 
> > Reported-by: Paulos Yibelo <habte.yibelo@gmail.com>
> > Reported-by: Chris Arges <carges@cloudflare.com>
> > Tested-by: Chris Arges <carges@cloudflare.com>
> > Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
> > Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > 
> > Notes (changelog):
> >     v4->v5:
> >         caller changes moved to "account for ingress-resolved
> >         virtio net hdr state" (Willem de Bruijn)
> >         a packet too small to be GSO: ignore the tunnel GSO request,
> >         complete wild metadata in place (sashiko)
> >         document the zero copy interaction (sashiko)
> >         honor a preset skb->protocol on ethernet: the stack uses it,
> >         except on loopback which re-parses the frame (sashiko)
> > 
> >  include/linux/virtio_net.h | 283 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 279 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > index 013c6c7e2dea..52852a33a2db 100644
> > --- a/include/linux/virtio_net.h
> > +++ b/include/linux/virtio_net.h
> > @@ -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,205 @@ 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;
> > +
> 
> Pulling again if ihl != 0 should probably have been here too.
> 
> Else we have to verify that all callers safely pull. Note that
> iph will become stale after another pull.
> 
> > +	*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)
> > +{
> > +	/* skb->dev is only guaranteed to be set since 44378d02c5aa
> > +	 * ("net: always dissect GSO packets in
> > +	 * __virtio_net_hdr_to_skb()"), do not crash when it is not.
> > +	 */
> > +	if (WARN_ON_ONCE(!skb->dev))
> > +		return false;
> > +
> > +	/* A preset protocol is what the stack will use, except on
> > +	 * loopback which re-parses the frame: loopback_xmit() calls
> > +	 * eth_type_trans() on it.
> > +	 */
> > +	if (skb->dev->type == ARPHRD_ETHER ||
> > +	    skb->dev->type == ARPHRD_LOOPBACK) {
> > +		if (!pskb_may_pull(skb, ETH_HLEN))
> > +			return false;
> > +		*nhoff = ETH_HLEN;
> > +		*proto = skb->protocol;
> 
> Why this change now?
> 
> skb->protocol cannot be trusted (to be consistent with actual packet
> content).
> 
> Express goal is to take the actual packet content instead of relying
> on this spoofable field.
> 
> On top of that there are issues around setting protocol to IP and
> inserting VLAN headers. That would skip the eth_type_vlan branch.
> 
> IMHO, we should only use skb->protocol if no other source to detect
> protocol exist, i.e., if no L2 header exists.
> 
> Also, instead of preferring one over the other, we could actually
> read both and
> 
>     if (skb->protocol &&
>         skb->protocol != eth_hdr(skb)->h_proto)
>             return false;
> 
> 
> > +		if (!*proto || skb->dev->type == ARPHRD_LOOPBACK)
> > +			*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);
> > +		/* An unset network header shows up as an offset
> > +		 * outside of the packet.
> > +		 */
> > +		if (*nhoff < 0 || (unsigned int)*nhoff >= skb->len)
> > +			return false;
> > +		*proto = skb->protocol;
> 
> Same
> 
> > +		if (!*proto) {
> > +			const struct iphdr *iph;
> > +
> > +			/* A packet socket bound to ETH_P_ALL leaves the
> > +			 * protocol unset: infer it from the IP version
> > +			 * at the network header.
> > +			 */
> > +			if (!pskb_may_pull(skb, *nhoff + 1))
> > +				return false;
> > +			iph = (const struct iphdr *)(skb->data + *nhoff);
> > +			if (iph->version == 4)
> > +				*proto = htons(ETH_P_IP);
> > +			else if (iph->version == 6)
> > +				*proto = htons(ETH_P_IPV6);
> > +		}
> > +	}
> > +	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;
> > +			/* Find the transport header behind any
> > +			 * extension headers, AH included.
> > +			 */
> > +			*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.
> > + */
> > +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)
> > @@ -194,11 +397,35 @@ 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;
> > +
> > +	return __virtio_net_resolve_csum_partial_mismatch(skb);
> >  }
> >  
> >  /* This function must be called after virtio_net_hdr_from_skb(). */
> > @@ -282,6 +509,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 +549,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;
> >  
> > @@ -335,9 +592,12 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> >  	/* 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;
> >  

One other case I found in testing is that a producer can set gso_size
larger than the remaining packet data. __virtio_net_hdr_to_skb() then
succeeds, but intentionally does not install GSO state because the packet
is too small to segment. NEEDS_CSUM has still left the skb with
CHECKSUM_PARTIAL.

The following check fixes my test case:

	if (!virtio_net_hdr_validate_disabled(skb) &&
	    !skb_is_gso(skb))
		return -EINVAL;

A concern I have is this may reject valid packets, so maybe there's a better
place for this.

> > +	if (!virtio_net_tnl_offsets_ok(skb, outer_isv6, outer_th))
> > +		return -EINVAL;
> > +
> >  	/* In case of USO, the inner protocol is still unknown and
> >  	 * `inner_isv6` is just a guess, additional parsing is needed.
> >  	 * The previous validation ensures that accessing an ipv4 inner
> > @@ -353,7 +613,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 +628,22 @@ 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;
> > +
> > +	if (!skb_is_gso(skb))
> > +		skb_shinfo(skb)->gso_type &= ~(SKB_GSO_UDP_TUNNEL_CSUM |
> > +					       SKB_GSO_UDP_TUNNEL);
> > +
> > +	/*
> > +	 * 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);
> 
> on second thought, if having to respin, might as well fold the trivial
> fix in patch 7 in here from the start.
> 
> > +	if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
> > +						 inner_proto))
> > +		return 0;
> > +
> > +	/* A tunnel header implies GSO, so drop. */
> 
> no longer true for the above !skb_is_gso(skb) case
>

virtio_net_csum_partial_ok_at_offset()

 
> > +	return __virtio_net_resolve_csum_partial_mismatch(skb);
> >  }
> >  
> >  /* Checksum-related fields validation for the driver */
> > -- 
> > MST
> > 
> 
> 

Re: [PATCH net v5 4/7] net: resolve uncommon partial checksums at ingress

[Michael S. Tsirkin]

On Thu, Oct 22, 2026 at 12:52:36PM -0500, Chris Arges wrote:
> > > @@ -335,9 +592,12 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> > >  	/* 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;
> > >  
> 
> One other case I found in testing is that a producer can set gso_size
> larger than the remaining packet data. __virtio_net_hdr_to_skb() then
> succeeds, but intentionally does not install GSO state because the packet
> is too small to segment. NEEDS_CSUM has still left the skb with
> CHECKSUM_PARTIAL.

So why is this a problem? If checksum offset is correct we leave
packet as CHECKSUM_PARTIAL. That's intentional.
What does your test do that makes this a problem?

> The following check fixes my test case:
> 
> 	if (!virtio_net_hdr_validate_disabled(skb) &&
> 	    !skb_is_gso(skb))
> 		return -EINVAL;
> 
> A concern I have is this may reject valid packets, so maybe there's a better
> place for this.


Indeed, it's not uncommon to have small packets with !gso.

See also:
commit 7c6d2ecbda83150b2036a2b36b21381ad4667762
    net: be more gentle about silly gso requests coming from user
    


> 
> > > +	if (!virtio_net_tnl_offsets_ok(skb, outer_isv6, outer_th))
> > > +		return -EINVAL;
> > > +
> > >  	/* In case of USO, the inner protocol is still unknown and
> > >  	 * `inner_isv6` is just a guess, additional parsing is needed.
> > >  	 * The previous validation ensures that accessing an ipv4 inner
> > > @@ -353,7 +613,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 +628,22 @@ 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;
> > > +
> > > +	if (!skb_is_gso(skb))
> > > +		skb_shinfo(skb)->gso_type &= ~(SKB_GSO_UDP_TUNNEL_CSUM |
> > > +					       SKB_GSO_UDP_TUNNEL);
> > > +
> > > +	/*
> > > +	 * 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);
> > 
> > on second thought, if having to respin, might as well fold the trivial
> > fix in patch 7 in here from the start.
> > 
> > > +	if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
> > > +						 inner_proto))
> > > +		return 0;
> > > +
> > > +	/* A tunnel header implies GSO, so drop. */
> > 
> > no longer true for the above !skb_is_gso(skb) case
> >
> 
> virtio_net_csum_partial_ok_at_offset()

Sorry I don't get what you are saying.

>  
> > > +	return __virtio_net_resolve_csum_partial_mismatch(skb);
> > >  }
> > >  
> > >  /* Checksum-related fields validation for the driver */
> > > -- 
> > > MST
> > > 
> > 
> > 

Re: [PATCH net v5 4/7] net: resolve uncommon partial checksums at ingress

[Chris Arges]

On 2026-10-22 19:38:53, Michael S. Tsirkin wrote:
> On Thu, Oct 22, 2026 at 12:52:36PM -0500, Chris Arges wrote:
> > > > @@ -335,9 +592,12 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> > > >  	/* 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;
> > > >  
> > 
> > One other case I found in testing is that a producer can set gso_size
> > larger than the remaining packet data. __virtio_net_hdr_to_skb() then
> > succeeds, but intentionally does not install GSO state because the packet
> > is too small to segment. NEEDS_CSUM has still left the skb with
> > CHECKSUM_PARTIAL.
> 
> So why is this a problem? If checksum offset is correct we leave
> packet as CHECKSUM_PARTIAL. That's intentional.
> What does your test do that makes this a problem?

The problem I saw used this case and setting inner_nh_offset incorrectly.
I'll dig more into this, and follow-up separately if this indeed is a
real problem.

> 
> > The following check fixes my test case:
> > 
> > 	if (!virtio_net_hdr_validate_disabled(skb) &&
> > 	    !skb_is_gso(skb))
> > 		return -EINVAL;
> > 
> > A concern I have is this may reject valid packets, so maybe there's a better
> > place for this.
> 
> 
> Indeed, it's not uncommon to have small packets with !gso.
> 
> See also:
> commit 7c6d2ecbda83150b2036a2b36b21381ad4667762
>     net: be more gentle about silly gso requests coming from user
>     

Ok this is helpful, I can see why that doesn't work.
Thanks

> 
> 
> > 
> > > > +	if (!virtio_net_tnl_offsets_ok(skb, outer_isv6, outer_th))
> > > > +		return -EINVAL;
> > > > +
> > > >  	/* In case of USO, the inner protocol is still unknown and
> > > >  	 * `inner_isv6` is just a guess, additional parsing is needed.
> > > >  	 * The previous validation ensures that accessing an ipv4 inner
> > > > @@ -353,7 +613,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 +628,22 @@ 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;
> > > > +
> > > > +	if (!skb_is_gso(skb))
> > > > +		skb_shinfo(skb)->gso_type &= ~(SKB_GSO_UDP_TUNNEL_CSUM |
> > > > +					       SKB_GSO_UDP_TUNNEL);
> > > > +
> > > > +	/*
> > > > +	 * 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);
> > > 
> > > on second thought, if having to respin, might as well fold the trivial
> > > fix in patch 7 in here from the start.
> > > 
> > > > +	if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
> > > > +						 inner_proto))
> > > > +		return 0;
> > > > +
> > > > +	/* A tunnel header implies GSO, so drop. */
> > > 
> > > no longer true for the above !skb_is_gso(skb) case
> > >
> > 
> > virtio_net_csum_partial_ok_at_offset()
> 
> Sorry I don't get what you are saying.
> 
> >  
> > > > +	return __virtio_net_resolve_csum_partial_mismatch(skb);
> > > >  }
> > > >  
> > > >  /* Checksum-related fields validation for the driver */
> > > > -- 
> > > > MST
> > > > 
> > > 
> > > 
> 

Re: [PATCH net v5 4/7] net: resolve uncommon partial checksums at ingress

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 7 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 3 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In virtio_net_l3_start() (include/linux/virtio_net.h), a preset
  skb->protocol on ARPHRD_ETHER devices takes priority over the frame's…
- [High] On the tunnel path, the inner checksum validation is anchored at
  the sender-supplied inner_nh_offset.
- [Medium] This patch changes the return contract of
  virtio_net_hdr_to_skb() and virtio_net_hdr_tnl_to_skb(): >0 now means…
- [Medium] The new virtio_net_tnl_offsets_ok() parses through VLAN
  headers and requires outer_th_offset to equal the actual UDP offset in…
- [Medium] virtio_net_hdr_tnl_from_skb() sets
  VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6 only when skb->protocol ==…
- [Low] virtio_net_hdr_tnl_to_skb() now returns -EINVAL whenever
  virtio_net_tnl_offsets_ok() fails.
- [Low] The comment '/* A tunnel header implies GSO, so drop.

--

net: resolve uncommon partial checksums at ingress

This patch validates the virtio net header csum_start and csum_offset at
ingress. CHECKSUM_PARTIAL is kept only when they match a parsed TCP or UDP
header. Otherwise, non-GSO packets get their checksum completed in software
and GSO packets are dropped. UDP tunnel headers get the same treatment on
the inner header.

> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 013c6c7e2dea0..52852a33a2db7 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h

[ ... ]

> @@ -48,6 +52,205 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,

[ ... ]

> +static inline bool
> +virtio_net_l3_start(struct sk_buff *skb, int *nhoff, __be16 *proto)
> +{

[ ... ]

> +	/* A preset protocol is what the stack will use, except on
> +	 * loopback which re-parses the frame: loopback_xmit() calls
> +	 * eth_type_trans() on it.
> +	 */
> +	if (skb->dev->type == ARPHRD_ETHER ||
> +	    skb->dev->type == ARPHRD_LOOPBACK) {
> +		if (!pskb_may_pull(skb, ETH_HLEN))
> +			return false;
> +		*nhoff = ETH_HLEN;
> +		*proto = skb->protocol;
> +		if (!*proto || skb->dev->type == ARPHRD_LOOPBACK)
> +			*proto = eth_hdr(skb)->h_proto;

[Severity: High]
Is loopback really the only ARPHRD_ETHER device that re-parses the frame?
veth and the other dev_forward_skb() users also run eth_type_trans() on it:

veth_xmit()
  veth_forward_skb()
    __dev_forward_skb()
      __dev_forward_skb2()
        skb->protocol = eth_type_trans(skb, dev);

Along that path, ____dev_forward_skb() and skb_scrub_packet() leave
CHECKSUM_PARTIAL in place.

On the sending side, packet_snd() installs the protocol the sender chose
before it calls virtio_net_hdr_to_skb():

net/packet/af_packet.c:packet_snd() {
    ...
	if (proto != htons(ETH_P_ALL))
		skb->protocol = proto;
    ...
}

packet_parse_headers() only re-derives the protocol when it is zero.

So an AF_PACKET sender using PACKET_VNET_HDR on a veth could make this
validator parse a different L3 layout from the one the peer stack uses.
CAP_NET_RAW in a user namespace is enough. Two examples:

- preset ETH_P_IP on an 802.1Q-tagged frame: the IPv4 header is parsed
  at offset 14, on top of the TCI

- preset ETH_P_8021Q on an untagged frame: __vlan_get_protocol() reads
  the real tot_len as the encapsulated protocol, and an IPv4 header is
  parsed at offset 18

The fake header only needs version == 4, ihl >= 5, no fragment bits, and
protocol TCP or UDP. A csum_start of nhoff + ihl * 4 with the matching
csum_offset is then accepted.

In the second example, a fake ihl of 5 over a real ihl of 7, with a fake
protocol of UDP, puts the checksum write at packet offset 44. That is the
real TCP destination port.

CHECKSUM_PARTIAL is kept in that case. Netfilter or tc may already have
acted on the packet when a later skb_checksum_help() runs, for example on
egress to a device without checksum offload. That call would then write
into the real TCP header. Isn't that the same post-decision modification
the commit message says this patch closes?

Would it be safer to check eth_hdr(skb)->h_proto against a preset
skb->protocol, or always parse the frame, on every ARPHRD_ETHER device
instead of special-casing loopback?

[ ... ]

> @@ -194,11 +397,35 @@ 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);

[ ... ]

> +	skb->transport_header = old_transport;
> +
> +	return __virtio_net_resolve_csum_partial_mismatch(skb);
>  }

[Severity: Medium]
This changes the return contract of virtio_net_hdr_to_skb() and
virtio_net_hdr_tnl_to_skb(): a positive value now means the checksum was
completed in software.

The patch has a Fixes: tag for f43798c27684 ("tun: Allow GSO using
virtio_net_hdr"), so stable is likely to pick it up. The callers are only
updated by earlier patches in the series, "net: account for
ingress-resolved virtio net hdr state" and "um: vector: trim skbs before
verifying the header". Neither has a Fixes: or stable tag, and the commit
message does not mention them.

At the baseline, every caller treats any nonzero return as failure, for
example:

drivers/net/tun.c:tun_get_user() {
    ...
	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
    ...
}

tun_xdp_one(), tap ("if (err)"), tpacket_snd() and packet_snd()
("if (err) goto out_free") do the same.

If this patch is backported alone, would tun, tap and virtio_net drop
every packet whose checksum was completed in software as a frame error?
Would packet_snd() return 1 from sendmsg()? And without the um vector
change, would skb_checksum_help() also checksum the untrimmed buffer tail?

Could the prerequisites get stable tags too, or could the dependency be
noted in the commit message?

> @@ -282,6 +509,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;

[Severity: Medium]
Can this reject IPv6-outer tunnel packets from the in-tree encoder when
they carry an in-band VLAN tag? virtio_net_hdr_tnl_from_skb() picks the
tunnel family from skb->protocol alone:

	if (skb->protocol == htons(ETH_P_IPV6))
		hdr->gso_type |= VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6;
	else
		hdr->gso_type |= VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4;

If skb->protocol is ETH_P_8021Q or ETH_P_8021AD with an in-band tag, an
IPv6 outer header is advertised as IPV4. Such skbs can come from a VLAN
device with REORDER_HDR off, where vlan_dev_hard_header() sets
skb->protocol to vlan->vlan_proto, or from QinQ. tap's
passthru_features_check keeps the GSO features for them.

virtio_net_l3_start() parses through the VLAN header and returns the real
family. The receiver (a guest's virtio_net, or tun ingress) now returns
-EINVAL for packets it used to accept. The encoder's classification is
older, but the drop comes from this new check.

> +	if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
> +		return false;
> +
> +	return ip_proto == IPPROTO_UDP && outer_th == thoff;
> +}

[Severity: Medium]
Does this exact match fail for VLAN-tagged UDP tunnel GSO traffic sent by
tun?

For hwaccel tags, tun_vnet_hdr_tnl_from_skb() passes vlan_hlen =
VLAN_HLEN, and tun_put_user() inserts the tag into the exported frame.
virtio_net_hdr_tnl_from_skb() adds vlan_hlen to csum_start but not to
the tunnel offsets:

	inner_nh = skb->inner_network_header - skb_headroom(skb);
	outer_th = skb->transport_header - skb_headroom(skb);
	vhdr->inner_nh_offset = cpu_to_le16(inner_nh);
	vhdr->outer_th_offset = cpu_to_le16(outer_th);

tun's vlan_features include NETIF_F_GSO_UDP_TUNNEL. A UDP tunnel GSO skb
with an hwaccel VLAN tag can therefore reach tun_put_user() unsegmented,
for example VXLAN between VMs across a VLAN-filtering bridge with tagged
tap egress.

The receiving guest's virtnet_receive_done()->virtio_net_hdr_tnl_to_skb()
used to accept these offsets. Now virtio_net_l3_start() parses the 802.1Q
header (nhoff = 18), so thoff ends up 4 bytes past outer_th and the packet
is dropped as a frame error. The inner check at the 4-byte-short inner_nh
would fail as well.

The encoder's omission is older, but isn't this drop new with this
validator?

[ ... ]

> @@ -335,9 +592,12 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
>  	/* 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;
>  
> +	if (!virtio_net_tnl_offsets_ok(skb, outer_isv6, outer_th))
> +		return -EINVAL;
> +

[Severity: Low]
This drop happens before any GSO or non-GSO distinction. Tunnel packets
too small to be GSO are also dropped when their outer L3/UDP offsets do
not match the frame.

The commit message only describes checksum handling ("UDP tunnel headers
get the same handling on the inner header", "Complete other non-GSO
checksums immediately"). The v5 changelog note ("a packet too small to be
GSO: ignore the tunnel GSO request, complete wild metadata in place")
suggests such packets are completed rather than dropped.

Further down, the patch also clears SKB_GSO_UDP_TUNNEL and
SKB_GSO_UDP_TUNNEL_CSUM for non-GSO skbs without mentioning it.

Could the commit message describe both of these behaviour changes?

[ ... ]

> @@ -368,7 +628,22 @@ 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;
> +
> +	if (!skb_is_gso(skb))
> +		skb_shinfo(skb)->gso_type &= ~(SKB_GSO_UDP_TUNNEL_CSUM |
> +					       SKB_GSO_UDP_TUNNEL);
> +
> +	/*
> +	 * 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;

[Severity: High]
The inner_nh anchor used here comes from the sender's inner_nh_offset.
Earlier in virtio_net_hdr_tnl_to_skb() it is only checked for ordering:

	if (outer_th < outer_l3min ||
	    inner_nh < outer_th + sizeof(struct udphdr) ||
	    inner_th < inner_nh + inner_l3min)
		return -EINVAL;

Nothing ties it to the real encapsulated header. Can a forged IP-looking
header at inner_nh pass this check while csum_start and csum_offset point
into a different field of the real inner packet?

Take Eth/IPv4/UDP/VXLAN/Eth/IPv4/TCP, with the real inner IP header at 64
and the inner TCP header at 84:

- inner_nh_offset = 52, with the inner MAC bytes chosen so byte 52 is
  0x47 (version 4, ihl 7, so thoff is 80)
- the fake frag_off (src MAC bytes 2-3) is 0
- the fake protocol (src MAC byte 5) is 17
- gso_type is UDP_L4, csum_start = 80, csum_offset = 6

The checksum then lands at offset 86, which is the real inner TCP
destination port.

If gso_size is larger than the payload, the skb is not GSO. The tunnel
GSO bits are cleared, CHECKSUM_PARTIAL stays, and 0 is returned.

After VXLAN decap through iptunnel_pull_header(), the inner stack and
netfilter see the original dport. A later skb_checksum_help() would then
overwrite it with a value the sender controls.

In the GSO case, the same forged offsets become inner_transport_header
and send segmentation writes to the wrong fields.

Should inner_nh be bound to the real encapsulation, with the checksum
completed or the packet rejected when that cannot be established?

> +
> +	/* A tunnel header implies GSO, so drop. */
> +	return __virtio_net_resolve_csum_partial_mismatch(skb);
>  }

[Severity: Low]
This isn't a bug, but the comment does not match the code.
__virtio_net_resolve_csum_partial_mismatch() returns -EINVAL only when
skb_is_gso(skb). Otherwise it runs skb_checksum_help(), returns 1, and the
packet is delivered. The non-GSO tunnel case is also handled just above,
where the tunnel gso_type bits are cleared.

Could the comment say that non-GSO packets are completed in software?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.v5.git.mst%40redhat.com

[PATCH net v5 5/7] net: add a sysctl to opt out of virtio net hdr checksum validation

[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_validate

The default is validating.

Reviewed-by: Björn Töpel <bjorn@kernel.org>
Reviewed-by: Willem de Bruijn <willemb@google.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Notes (changelog):
    v4->v5:
        document the netns lifetime requirement (sashiko)

 Documentation/admin-guide/sysctl/net.rst | 19 +++++++++
 include/linux/virtio_net.h               | 27 +++++++++++++
 include/net/netns/core.h                 |  1 +
 net/core/dev.c                           |  7 ++++
 net/core/net_namespace.c                 |  1 +
 net/core/sysctl_net_core.c               | 50 ++++++++++++++++++++++++
 6 files changed, 105 insertions(+)

diff --git a/Documentation/admin-guide/sysctl/net.rst b/Documentation/admin-guide/sysctl/net.rst
index e586e17fc7a5..3c0223575438 100644
--- a/Documentation/admin-guide/sysctl/net.rst
+++ b/Documentation/admin-guide/sysctl/net.rst
@@ -246,6 +246,25 @@ processes are dropped unless socket option SOF_TIMESTAMPING_OPT_TSONLY is set.
 Default: 1 (on)
 
 
+virtio_net_hdr_validate
+-----------------------
+
+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.
+
+The setting is per network namespace, but only writable by the host
+root: root inside a network namespace created by an unprivileged user
+cannot disable the validation.
+
+Default: 1 (on)
+
+
 wmem_default
 ------------
 
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 52852a33a2db..cea70ccf0ed4 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -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,20 @@ 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_validate set to 0.
+ */
+DECLARE_STATIC_KEY_FALSE(virtio_net_hdr_validate_disabled_key);
+
+/* Failsafe: a namespace can disable the check with
+ * net.core.virtio_net_hdr_validate.
+ * The caller must keep the netns of skb->dev alive.
+ */
+static inline bool virtio_net_hdr_validate_disabled(struct sk_buff *skb)
+{
+	return static_branch_unlikely(&virtio_net_hdr_validate_disabled_key) &&
+	       !READ_ONCE(dev_net(skb->dev)->core.sysctl_virtio_net_hdr_validate);
+}
 
 /* Where does the L3 header start, and what protocol is it?
  * Handles Ethernet and VLAN headers; other devices use the
@@ -189,6 +204,12 @@ virtio_net_csum_partial_ok_at_offset(struct sk_buff *skb, u8 hdr_gso_type,
 	unsigned int thoff;
 	int ip_proto;
 
+	/* Failsafe: a namespace opted out of the validation,
+	 * trust the checksum metadata as before.
+	 */
+	if (virtio_net_hdr_validate_disabled(skb))
+		return true;
+
 	if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
 		return false;
 
@@ -231,6 +252,9 @@ virtio_net_csum_partial_ok(struct sk_buff *skb, u8 hdr_gso_type)
 	int nhoff;
 	__be16 proto;
 
+	if (virtio_net_hdr_validate_disabled(skb))
+		return true;
+
 	if (!virtio_net_l3_start(skb, &nhoff, &proto))
 		return false;
 
@@ -522,6 +546,9 @@ virtio_net_tnl_offsets_ok(struct sk_buff *skb, bool outer_isv6,
 	unsigned int thoff;
 	int ip_proto;
 
+	if (virtio_net_hdr_validate_disabled(skb))
+		return true;
+
 	if (!virtio_net_l3_start(skb, &nhoff, &proto))
 		return false;
 	if (outer_isv6 != (proto == htons(ETH_P_IPV6)))
diff --git a/include/net/netns/core.h b/include/net/netns/core.h
index 9ef3d70e5e9c..02ce232fe9a9 100644
--- a/include/net/netns/core.h
+++ b/include/net/netns/core.h
@@ -18,6 +18,7 @@ struct netns_core {
 	u8	sysctl_txrehash;
 	u8	sysctl_tstamp_allow_data;
 	u8	sysctl_bypass_prot_mem;
+	u8	sysctl_virtio_net_hdr_validate;
 
 #ifdef CONFIG_PROC_FS
 	struct prot_inuse __percpu *prot_inuse;
diff --git a/net/core/dev.c b/net/core/dev.c
index e76762e29360..43b8823c83f6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -128,6 +128,7 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <net/ip.h>
+#include <linux/virtio_net.h>
 #include <net/mpls.h>
 #include <linux/ipv6.h>
 #include <linux/in.h>
@@ -2389,6 +2390,12 @@ EXPORT_SYMBOL(tcf_sw_enabled_key);
 
 DEFINE_STATIC_KEY_FALSE(netstamp_needed_key);
 EXPORT_SYMBOL(netstamp_needed_key);
+
+/* Enabled while any network namespace has net.core.virtio_net_hdr_validate
+ * set to 0.
+ */
+DEFINE_STATIC_KEY_FALSE(virtio_net_hdr_validate_disabled_key);
+EXPORT_SYMBOL(virtio_net_hdr_validate_disabled_key);
 #ifdef CONFIG_JUMP_LABEL
 static atomic_t netstamp_needed_deferred;
 static atomic_t netstamp_wanted;
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index da5f881fbd3b..8a21baaa7e6d 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -396,6 +396,7 @@ static __net_init void preinit_net_sysctl(struct net *net)
 	net->core.sysctl_optmem_max = 128 * 1024;
 	net->core.sysctl_txrehash = SOCK_TXREHASH_ENABLED;
 	net->core.sysctl_tstamp_allow_data = 1;
+	net->core.sysctl_virtio_net_hdr_validate = 1;
 	net->core.sysctl_txq_reselection = msecs_to_jiffies(1000);
 }
 
diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
index eb35da3556f4..3e4037b82a97 100644
--- a/net/core/sysctl_net_core.c
+++ b/net/core/sysctl_net_core.c
@@ -18,6 +18,8 @@
 #include <linux/slab.h>
 #include <linux/sched/isolation.h>
 #include <linux/hex.h>
+#include <linux/virtio_net.h>
+#include <linux/jump_label.h>
 
 #include <net/ip.h>
 #include <net/sock.h>
@@ -678,6 +680,43 @@ static struct ctl_table net_core_table[] = {
 	},
 };
 
+/* Protects changes to virtio_net_hdr_validate_disabled_key and each
+ * net->core.sysctl_virtio_net_hdr_validate.  The packet path reads
+ * the value without a lock.
+ */
+static DEFINE_MUTEX(virtio_net_hdr_validate_mutex);
+
+static int virtio_net_hdr_validate_sysctl(const struct ctl_table *table,
+					  int write, void *buffer,
+					  size_t *lenp, loff_t *ppos)
+{
+	u8 *value = table->data;
+	u8 old, new;
+	int ret;
+
+	/* Root in a user namespace must not be able to weaken the
+	 * validation for the network namespace it controls.
+	 */
+	if (write && !capable(CAP_NET_ADMIN))
+		return -EPERM;
+
+	mutex_lock(&virtio_net_hdr_validate_mutex);
+
+	old = READ_ONCE(*value);
+	ret = proc_dou8vec_minmax(table, write, buffer, lenp, ppos);
+	new = READ_ONCE(*value);
+	if (write && !ret && old != new) {
+		if (new)
+			static_branch_dec(&virtio_net_hdr_validate_disabled_key);
+		else
+			static_branch_inc(&virtio_net_hdr_validate_disabled_key);
+	}
+
+	mutex_unlock(&virtio_net_hdr_validate_mutex);
+
+	return ret;
+}
+
 static const struct ctl_table netns_core_table[] = {
 #if IS_ENABLED(CONFIG_RPS)
 	{
@@ -737,6 +776,15 @@ static const struct ctl_table netns_core_table[] = {
 		.extra1		= SYSCTL_ZERO,
 		.extra2		= SYSCTL_ONE
 	},
+	{
+		.procname	= "virtio_net_hdr_validate",
+		.data		= &init_net.core.sysctl_virtio_net_hdr_validate,
+		.maxlen		= sizeof(u8),
+		.mode		= 0644,
+		.proc_handler	= virtio_net_hdr_validate_sysctl,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE
+	},
 	/* sysctl_core_net_init() will set the values after this
 	 * to readonly in network namespaces
 	 */
@@ -841,6 +889,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_validate)
+		static_branch_dec(&virtio_net_hdr_validate_disabled_key);
 #if IS_ENABLED(CONFIG_RPS)
 	kfree(net->core.rps_default_mask);
 #endif
-- 
MST

Re: [PATCH net v5 5/7] net: add a sysctl to opt out of virtio net hdr checksum validation

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] virtio_net_hdr_validate_disabled() reads
  dev_net(skb->dev)->core.sysctl_virtio_net_hdr_validate with no RCU…

--

net: add a sysctl to opt out of virtio net hdr checksum validation

This adds a per-netns sysctl, net.core.virtio_net_hdr_validate. Host root
can use it to turn off the virtio net header checksum validation added by
the previous patch. A static key gates a lockless read of the per-netns
value in virtio_net_hdr_validate_disabled(), and that helper is checked
at the start of the csum and tunnel offset validators.

> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 52852a33a2db7..cea70ccf0ed4b 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h

[ ... ]

> @@ -78,6 +79,20 @@ 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_validate set to 0.
> + */
> +DECLARE_STATIC_KEY_FALSE(virtio_net_hdr_validate_disabled_key);
> +
> +/* Failsafe: a namespace can disable the check with
> + * net.core.virtio_net_hdr_validate.
> + * The caller must keep the netns of skb->dev alive.
> + */
> +static inline bool virtio_net_hdr_validate_disabled(struct sk_buff *skb)
> +{
> +	return static_branch_unlikely(&virtio_net_hdr_validate_disabled_key) &&
> +	       !READ_ONCE(dev_net(skb->dev)->core.sysctl_virtio_net_hdr_validate);

[Severity: Medium]

Can this read hit a freed struct net? There is no rcu_read_lock() here
and no reference on the netns.

dev_net() is read_pnet(), which is just a plain load of dev->nd_net. The
comment asks the caller to keep the netns of skb->dev alive, but the
callers don't appear to do that.

For example, tun_get_user() gets here from preemptible process context,
before either of its rcu_read_lock() sections. The only thing it holds is
the device reference taken by tun_get():

tun_get_user()
  tun_vnet_hdr_tnl_to_skb()
    ...
      virtio_net_hdr_validate_disabled()

packet_snd() and tpacket_snd() reach virtio_net_hdr_to_skb() the same
way, also holding only a device reference.

Here is one possible sequence:

  - The writer loads dev->nd_net (net_B) and is preempted.
  - The device is moved from net_B to net_C. tun does not set
    netns_local, and dev_change_net_namespace() does not wait for
    device references.
  - The last reference to net_B is dropped and cleanup_net() runs.
    This reader is not in an RCU read-side section, so the
    synchronize_rcu() there does not wait for it, and net_B is freed.
  - The writer resumes and reads sysctl_virtio_net_hdr_validate from
    the freed net_B.

The window is small. It needs the static key enabled (some netns has
the sysctl set to 0) and a privileged netns move with teardown.

Would it be better to do the read under rcu_read_lock() and use
dev_net_rcu(), something like this?

	bool disabled;

	rcu_read_lock();
	disabled = !READ_ONCE(dev_net_rcu(skb->dev)->core.sysctl_virtio_net_hdr_validate);
	rcu_read_unlock();

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

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.v5.git.mst%40redhat.com

[PATCH net v5 6/7] net: add ip4_parse()

[Michael S. Tsirkin]

Multiple places in the net stack open-code the same minimal IPv4 header
validation: pull the fixed header, check version 4 and IHL >= 5 (where 5
is the fixed header size in 32-bit units) - and locate the transport
header at IHL * 4 past the network header.

Add a helper for doing that: it returns the validated header and
the transport header offset, or NULL if truncated or malformed.

As noted by Phil Sutter, there are more places that could use this
helper, e.g. ip_mc_check_iphdr(). They will be converted in a follow-up
patch.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Björn Töpel <bjorn@kernel.org>
Reviewed-by: Phil Sutter <phil@nwl.cc>
---

Notes (changelog):
    v4->v5:
        pull the options as well, so that the transport header
        offset is within the linear data (Willem de Bruijn)

 include/linux/ip.h                  | 37 +++++++++++++++++++++++++++++
 include/linux/virtio_net.h          | 28 +---------------------
 net/bridge/br_netfilter_hooks.c     | 15 ++++--------
 net/ipv4/ip_input.c                 | 13 ++++------
 net/ipv4/netfilter/nf_reject_ipv4.c | 13 ++++------
 5 files changed, 53 insertions(+), 53 deletions(-)

diff --git a/include/linux/ip.h b/include/linux/ip.h
index d11c25f5030a..7e047d18a61c 100644
--- a/include/linux/ip.h
+++ b/include/linux/ip.h
@@ -56,4 +56,41 @@ static inline void iph_set_totlen(struct iphdr *iph, unsigned int len)
 {
 	iph->tot_len = len <= IP_MAX_MTU ? htons(len) : 0;
 }
+
+/**
+ * ip4_parse - pull and minimally validate an IPv4 header
+ * @skb: buffer to parse
+ * @nhoff: offset of the IPv4 header within the linear data
+ * @thoff: returns the transport header offset
+ *
+ * The whole header, options included, is guaranteed linear on
+ * return, so the transport header offset is within the linear
+ * data.  Callers that need to read the transport header must
+ * pull further.
+ *
+ * Return: the validated IPv4 header, or NULL if truncated or
+ * malformed.
+ */
+static inline struct iphdr *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;
+
+	/* Pull the options as well: the transport header offset
+	 * returned below must be within the linear data.
+	 */
+	if (!pskb_may_pull(skb, nhoff + iph->ihl * sizeof(u32)))
+		return NULL;
+
+	*thoff = nhoff + iph->ihl * sizeof(u32);
+	return (struct iphdr *)(skb->data + nhoff);
+}
 #endif	/* _LINUX_IP_H */
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index cea70ccf0ed4..063cc116e51a 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -53,32 +53,6 @@ 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;
-}
 /* Defined in net/core/dev.c, enabled while any network namespace
  * has net.core.virtio_net_hdr_validate set to 0.
  */
@@ -161,7 +135,7 @@ 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);
+		const struct iphdr *iph = ip4_parse(skb, nhoff, thoff);
 
 		if (!iph || ip_is_fragment(iph))
 			return false;
diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
index 0a394e5f4391..e89da0e41542 100644
--- a/net/bridge/br_netfilter_hooks.c
+++ b/net/bridge/br_netfilter_hooks.c
@@ -206,18 +206,13 @@ static inline void nf_bridge_pull_encap_header_rcsum(struct sk_buff *skb)
 static int br_validate_ipv4(struct net *net, struct sk_buff *skb)
 {
 	const struct iphdr *iph;
-	u32 len;
+	u32 len, thoff;
 
-	if (!pskb_may_pull(skb, sizeof(struct iphdr)))
+	iph = ip4_parse(skb, 0, &thoff);
+	if (!iph)
 		goto inhdr_error;
 
-	iph = ip_hdr(skb);
-
-	/* Basic sanity checks */
-	if (iph->ihl < 5 || iph->version != 4)
-		goto inhdr_error;
-
-	if (!pskb_may_pull(skb, iph->ihl*4))
+	if (!pskb_may_pull(skb, thoff))
 		goto inhdr_error;
 
 	iph = ip_hdr(skb);
@@ -228,7 +223,7 @@ static int br_validate_ipv4(struct net *net, struct sk_buff *skb)
 	if (skb->len < len) {
 		__IP_INC_STATS(net, IPSTATS_MIB_INTRUNCATEDPKTS);
 		goto drop;
-	} else if (len < (iph->ihl*4))
+	} else if (len < thoff)
 		goto inhdr_error;
 
 	if (pskb_trim_rcsum(skb, len)) {
diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
index 9860178752b8..5adf01763e85 100644
--- a/net/ipv4/ip_input.c
+++ b/net/ipv4/ip_input.c
@@ -500,7 +500,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
 {
 	const struct iphdr *iph;
 	int drop_reason;
-	u32 len;
+	u32 len, thoff;
 
 	/* When the interface is in promisc. mode, drop all the crap
 	 * that it receives, do not try to analyse it.
@@ -520,10 +520,6 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
 	}
 
 	drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
-	if (!pskb_may_pull(skb, sizeof(struct iphdr)))
-		goto inhdr_error;
-
-	iph = ip_hdr(skb);
 
 	/*
 	 *	RFC1122: 3.2.1.2 MUST silently discard any IP frame that fails the checksum.
@@ -536,7 +532,8 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
 	 *	4.	Doesn't have a bogus length
 	 */
 
-	if (iph->ihl < 5 || iph->version != 4)
+	iph = ip4_parse(skb, 0, &thoff);
+	if (!iph)
 		goto inhdr_error;
 
 	BUILD_BUG_ON(IPSTATS_MIB_ECT1PKTS != IPSTATS_MIB_NOECTPKTS + INET_ECN_ECT_1);
@@ -546,7 +543,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
 		       IPSTATS_MIB_NOECTPKTS + (iph->tos & INET_ECN_MASK),
 		       max_t(unsigned short, 1, skb_shinfo(skb)->gso_segs));
 
-	if (!pskb_may_pull(skb, iph->ihl*4))
+	if (!pskb_may_pull(skb, thoff))
 		goto inhdr_error;
 
 	iph = ip_hdr(skb);
@@ -559,7 +556,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
 		drop_reason = SKB_DROP_REASON_PKT_TOO_SMALL;
 		__IP_INC_STATS(net, IPSTATS_MIB_INTRUNCATEDPKTS);
 		goto drop;
-	} else if (len < (iph->ihl*4))
+	} else if (len < thoff)
 		goto inhdr_error;
 
 	/* Our transport medium may have padded the buffer out. Now we know it
diff --git a/net/ipv4/netfilter/nf_reject_ipv4.c b/net/ipv4/netfilter/nf_reject_ipv4.c
index 59ec465a9df9..42d639854ddf 100644
--- a/net/ipv4/netfilter/nf_reject_ipv4.c
+++ b/net/ipv4/netfilter/nf_reject_ipv4.c
@@ -25,22 +25,19 @@ nf_reject_ip_tcphdr_get(struct sk_buff *oldskb,
 static int nf_reject_iphdr_validate(struct sk_buff *skb)
 {
 	struct iphdr *iph;
-	u32 len;
+	u32 len, thoff;
 
-	if (!pskb_may_pull(skb, sizeof(struct iphdr)))
-		return 0;
-
-	iph = ip_hdr(skb);
-	if (iph->ihl < 5 || iph->version != 4)
+	iph = ip4_parse(skb, 0, &thoff);
+	if (!iph)
 		return 0;
 
 	len = ntohs(iph->tot_len);
 	if (skb->len < len)
 		return 0;
-	else if (len < (iph->ihl*4))
+	else if (len < thoff)
 		return 0;
 
-	if (!pskb_may_pull(skb, iph->ihl*4))
+	if (!pskb_may_pull(skb, thoff))
 		return 0;
 
 	return 1;
-- 
MST

Re: [PATCH net v5 6/7] net: add ip4_parse()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Use-after-free read in the new ip4_parse() helper
  (include/linux/ip.h).
- [Low] The commit message presents the patch as adding a new helper for
  open-coded checks.
- [Low] The kernel-doc for ip4_parse() says "The whole header, options
  included, is guaranteed linear on return", and the helper does…

--

net: add ip4_parse()

This commit adds an ip4_parse() helper to include/linux/ip.h. The helper
pulls the IPv4 header including options, checks the version and IHL, and
returns the transport header offset. It replaces virtio_net_ip4_parse() and
converts ip_rcv_core(), br_validate_ipv4() and nf_reject_iphdr_validate()
to use it.

> diff --git a/include/linux/ip.h b/include/linux/ip.h
> index d11c25f5030a0..7e047d18a61c9 100644
> --- a/include/linux/ip.h
> +++ b/include/linux/ip.h
> @@ -56,4 +56,41 @@ static inline void iph_set_totlen(struct iphdr *iph, unsigned int len)
>  {
>  	iph->tot_len = len <= IP_MAX_MTU ? htons(len) : 0;
>  }
> +
> +/**
> + * ip4_parse - pull and minimally validate an IPv4 header
> + * @skb: buffer to parse
> + * @nhoff: offset of the IPv4 header within the linear data
> + * @thoff: returns the transport header offset
> + *
> + * The whole header, options included, is guaranteed linear on
> + * return, so the transport header offset is within the linear
> + * data.  Callers that need to read the transport header must
> + * pull further.
> + *
> + * Return: the validated IPv4 header, or NULL if truncated or
> + * malformed.
> + */
> +static inline struct iphdr *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;
> +
> +	/* Pull the options as well: the transport header offset
> +	 * returned below must be within the linear data.
> +	 */
> +	if (!pskb_may_pull(skb, nhoff + iph->ihl * sizeof(u32)))
> +		return NULL;
> +
> +	*thoff = nhoff + iph->ihl * sizeof(u32);

[Severity: High]
Can this read iph->ihl from a freed skb head?

iph is computed from skb->data after the first pskb_may_pull(). If the
options are not linear yet, the second pskb_may_pull() can go through
__pskb_pull_tail() -> pskb_expand_head(), which reallocates the head:

net/core/skbuff.c:__pskb_pull_tail() {
    ...
	if (eat > 0 || skb_cloned(skb)) {
		if (pskb_expand_head(skb, 0, eat > 0 ? eat + 128 : 0,
				     GFP_ATOMIC))
    ...
}

When the head is not shared, pskb_expand_head() calls skb_free_head() on
the old buffer. When the skb is cloned, it drops the dataref instead.

pskb_may_pull_reason() also calls skb_might_realloc(). With
CONFIG_FAIL_SKB_REALLOC the head is therefore reallocated on every call,
and KASAN should catch the stale read.

The __pskb_pull_tail() kernel-doc says: "All the pointers pointing into skb
header may change and must be reloaded after call to this function."

Only the return value is rebuilt from skb->data. The *thoff calculation
above still reads through the old iph.

The stale thoff is then used in several places:

- the len < thoff checks in ip_rcv_core(), br_validate_ipv4() and
  nf_reject_iphdr_validate()
- the csum_start comparison in
  virtio_net_l4_start()->virtio_net_csum_partial_ok_at_offset()

Suppose the freed byte has changed, for example because of init_on_free,
and thoff comes out as nhoff. Could a packet with ihl=15 and tot_len=20
then pass the len < thoff check? It would be trimmed below its header
length, and skb->transport_header would end up past the tail.

In the virtio path, could csum_start == nhoff with a TCP csum_offset then
pass validation? That would defeat the check added by "net: resolve
uncommon partial checksums at ingress".

The old virtio_net_ip4_parse() did only one pull. This seems to come from
the v5 change that also pulls the options.

The last patch in the series ("virtio_net: cleanup protocol calculation")
doesn't change ip4_parse(), so the problem appears to still be there at
the end of the series.

Would it work to compute the header length before the second pull, and to
use only the pointer rebuilt from skb->data after it? For example:

	hlen = iph->ihl * sizeof(u32);
	if (!pskb_may_pull(skb, nhoff + hlen))
		return NULL;

	*thoff = nhoff + hlen;
	return (struct iphdr *)(skb->data + nhoff);

> +	return (struct iphdr *)(skb->data + nhoff);
> +}
>  #endif	/* _LINUX_IP_H */

[ ... ]

> diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
> index 9860178752b8c..5adf01763e857 100644
> --- a/net/ipv4/ip_input.c
> +++ b/net/ipv4/ip_input.c

[ ... ]

> @@ -520,10 +520,6 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
>  	}
>  
>  	drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
> -	if (!pskb_may_pull(skb, sizeof(struct iphdr)))
> -		goto inhdr_error;
> -
> -	iph = ip_hdr(skb);
>  
>  	/*
>  	 *	RFC1122: 3.2.1.2 MUST silently discard any IP frame that fails the checksum.
> @@ -536,7 +532,8 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
>  	 *	4.	Doesn't have a bogus length
>  	 */
>  
> -	if (iph->ihl < 5 || iph->version != 4)
> +	iph = ip4_parse(skb, 0, &thoff);
> +	if (!iph)
>  		goto inhdr_error;
>  
>  	BUILD_BUG_ON(IPSTATS_MIB_ECT1PKTS != IPSTATS_MIB_NOECTPKTS + INET_ECN_ECT_1);

[Severity: Low]
The ihl * 4 pull now happens inside ip4_parse(), which runs before the ECN
accounting a few lines below:

	__IP_ADD_STATS(net,
		       IPSTATS_MIB_NOECTPKTS + (iph->tos & INET_ECN_MASK),
		       max_t(unsigned short, 1, skb_shinfo(skb)->gso_segs));

Before this patch, only the fixed 20 byte header was pulled before the
stats update, and the ihl * 4 pull came after it.

Is it intended that an IPv4 packet with ihl > 5 and truncated options, or
one whose pull fails to allocate, is no longer counted in the Ip ECN
counters? It is still counted in InHdrErrors.

The commit message also says:

  "Add a helper for doing that: it returns the validated header and
  the transport header offset, or NULL if truncated or malformed."

It doesn't mention that virtio_net_ip4_parse() is removed from
include/linux/virtio_net.h and renamed. It also doesn't mention that the
contract changes from "Only the fixed header is guaranteed linear" to
pulling the options as well.

Only the v4->v5 note below the cut mentions this, and that note won't be
kept in git history. Could the commit message describe these changes?

[ ... ]

> @@ -546,7 +543,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
>  		       IPSTATS_MIB_NOECTPKTS + (iph->tos & INET_ECN_MASK),
>  		       max_t(unsigned short, 1, skb_shinfo(skb)->gso_segs));
>  
> -	if (!pskb_may_pull(skb, iph->ihl*4))
> +	if (!pskb_may_pull(skb, thoff))
>  		goto inhdr_error;
>  
>  	iph = ip_hdr(skb);

[Severity: Low]
This isn't a bug, but ip4_parse() has already done
pskb_may_pull(skb, nhoff + iph->ihl * sizeof(u32)) with nhoff = 0, which is
exactly thoff. Its kernel-doc also says the options are guaranteed linear
on return.

Doesn't that mean this pull can never fail, so the goto inhdr_error branch
is dead code?

br_validate_ipv4() and nf_reject_iphdr_validate() have the same redundant
pskb_may_pull(skb, thoff).

The iph = ip_hdr(skb) reloads after the pull in ip_rcv_core() and
br_validate_ipv4() also look unnecessary, because ip4_parse() already
returns a pointer rebuilt from skb->data.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.v5.git.mst%40redhat.com

[PATCH net v5 7/7] virtio_net: cleanup protocol calculation

[Michael S. Tsirkin]

A trivial cleanup removing a duplicated code line.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Björn Töpel <bjorn@kernel.org>
---
 include/linux/virtio_net.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 063cc116e51a..e984e446ba44 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -613,8 +613,8 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 			return -EINVAL;
 	}
 
-	skb_set_inner_protocol(skb, inner_isv6 ? htons(ETH_P_IPV6) :
-			    htons(ETH_P_IP));
+	inner_proto = inner_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP);
+	skb_set_inner_protocol(skb, inner_proto);
 	if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) {
 		if (!tnl_csum_negotiated)
 			return -EINVAL;
@@ -638,7 +638,6 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	 * 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;
-- 
MST

Re: [PATCH net v5 7/7] virtio_net: cleanup protocol calculation

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] net: use-after-free read of iph->ihl after pskb_may_pull() in
  ip4_parse()

--

> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -638,7 +638,6 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
>  	 * 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;

[Severity: High]
This isn't introduced by this patch, but since inner_proto is what
steers virtio_net_csum_partial_ok_at_offset() into the IPv4 parser,
it's worth raising here.

virtio_net_csum_partial_ok_at_offset() calls virtio_net_l4_start(),
which for ETH_P_IP calls ip4_parse(). In ip4_parse() the fixed header
is pulled, iph is set to skb->data + nhoff, and then a second
pskb_may_pull(skb, nhoff + iph->ihl * sizeof(u32)) pulls in the
options. That second pull can go through __pskb_pull_tail() and
pskb_expand_head() when the options sit in paged frags, the skb is
cloned, or CONFIG_FAIL_SKB_REALLOC fires, and that frees the old
skb->head. The next line then computes *thoff from iph->ihl through
the stale pointer, before the header is reloaded from skb->data.

That is a use-after-free read that a guest, or a tun/tap/af_packet
user, can trigger with an IPv4 header whose options straddle the
linear area. If the freed buffer has been reused, we can also end up
with a bogus *thoff that is then checked against csum_start. Since
ip4_parse() is shared with ip_rcv_core(), br_validate_ipv4() and
nf_reject_iphdr_validate(), plain IPv4 ingress hits it too.

Reading ihl into a local before the second pskb_may_pull(), or
reloading iph from skb->data right after it, should be enough.
Do you want to fold a fix for ip4_parse() into this series, or send
it separately to net with a Fixes: tag for the commit that added
ip4_parse()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.v5.git.mst%40redhat.com

Re: [PATCH net v5 0/7] more virtio-net csum hardening

[Chris Arges]

On 2026-10-22 08:57:54, Michael S. Tsirkin wrote:
> Okay this seems to be converging.... But sashiko did find some issues I
> thought are best fixed. Specifically I thought of a better way to handle
> short gso in a tunnel - just clear tunnel gso! As a side effect we now
> fix up the checksum for these dodgy packets like we did before, instead
> of a drop. Good. And more false positives that prompted me to write a
> bit more comments. So here goes.
> 
> Apologies for reposting so frequently, I know netdev bot will be unhappy
> with that.  But Paulos's report made a lot of publicity, there is a lot
> of people looking into this, so I feel there's value in defending
> against all this class of issues sooner rather than later.
> 
> The issues were mostly found by code inspection, but all reproduced by
> me in a VM.
>

Tested this series again (minus UM) and it fixes my original test cases. Added
one comment after digging a bit deeper into the code. Again thank you for the
work here.

Tested-by: Chris Arges <carges@cloudflare.com>

 
> ------
> 
> As reported first by Paulos Yibelo <habte.yibelo@gmail.com> (after which
> others, credited below, found more ways the bug can affect users):
> 
> allowing untrusted hardware to specify csum start/offset for
> CHECKSUM_PARTIAL was not a good idea, because, if it edits the packet
> outside the actual checksum, and depending on the setup, the networking
> stack could be looking at the packet both before and after the edit, and
> seeing different results.
> 
> Previously, commit 6785011f8b16 ("ipv4: validate checksum_start before
> completing checksum") validated all known instances where this
> causes the most grief, but the real fix is to make sure
> that either only actual checksum fields are ever edited,
> or the checksum is completed before the net stack looks at it,
> so it is always consistent.
> 
> 
> This basically goes back to
> what we did before f43798c27684 ("tun: Allow GSO using virtio_net_hdr"),
> except in a narrow list of specified known good cases.
> 
> 
> Summary of the patchset:
> 
> These are probabably net material (and seem to be too big for stable,
> or I would recommend backporting there, too):
> 
>   net: account for ingress-resolved virtio net hdr state
> 	- accounting, not the fix itself, dependency of the fix
>   um: vector: trim skbs before verifying the header
> 	- fixes the um receive path: without it the checksum would be
> 	  completed over the unreceived tail of the buffer
>   virtio_net: parse tunnel csum_start little-endian
> 	- bugfix, a dependency of validating the header
>   net: resolve uncommon partial checksums at ingress
> 	- fix itself
>   net: add a sysctl to opt out of virtio net hdr checksum validation
> 	- reducing risk of the fix by adding a sysctl to disable it
> 
> These are cleanups, can be deferred to net-next, too:
> 
>   net: add ip4_parse()
>   virtio_net: cleanup protocol calculation
> 	- cleanup resulting code duplication
> 	 (kept separate to simplify backporting)
> 
> Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
> 
> Changes since v1:
> 	- reorder patches to fix bisectability, addressing
> 	  a comment by Björn Töpel.
> 	- code refactoring, different patch splitup,
> 	  a sysctl knob to disable as suggested by Willem de Bruijn.
> 	- fix a bunch of minor issues spotted by LLMs
> 
> 
> Changes since v2:
> 
> virtio_net: parse tunnel csum_start little-endian
>   - new patch. correctly parsing the header is a dependency
>     of validating it, otherwise i do not know what to validate
> 
> net: account for ingress-resolved virtio net hdr state
>   - make rx_csum_sw counter 64 bit so it won't wrap
>     (spotted by LLM review)
> 
> net: resolve uncommon partial checksums at ingress
>   - tunnel fixes to bugs noticed by Willem de Bruijn and
>     Chris Arges
>   - restore the csum_offset checks as suggested by Willem
> 
> net: add a sysctl to opt out of virtio net hdr checksum validation
>   - the knob is no longer writable by root inside a user
>     namespace: changing it now requires capable(CAP_NET_ADMIN)
>     suggested by Willem de Bruijn
> 
> net: add ip4_parse()
>   - comment converted to kernel-doc format (Willem de Bruijn)
> 
> um: vector: trim skbs before verifying the header
>   - new patch, found by code review
> 
> 
> Changes since v3:
> 
> net: account for ingress-resolved virtio net hdr state
>   - um: vector: honor virtio net hdr conversion failures
>     folded in (Björn Töpel)
>   - count on tap and virtio_net, too (Willem de Bruijn)
> 
> net: resolve uncommon partial checksums at ingress
>   - walk the frame on loopback, too: loopback_xmit() runs
>     eth_type_trans() on it (Willem de Bruijn)
>   - return the mismatch handling directly, GSO is implied in
>     the tunnel path so drop there outright, and fold the helper
>     into its one remaining caller (Willem de Bruijn)
>   - read the IP version through struct iphdr (Willem de Bruijn)
>   - document the ipv6_find_hdr() flags choice
> 
> net: add a sysctl to opt out of virtio net hdr checksum validation
>   - renamed to virtio_net_hdr_validate, in case more validation
>     is gated by the same knob later (Willem de Bruijn)
>   - point .data at the value like the neighboring entries,
>     the per-netns registration rebases it, and the handler reads
>     the value directly - no ctl_table copy (Björn Töpel,
>     Willem de Bruijn)
>   - check the opt-out before parsing: with the knob off, packets
>     the parser cannot handle were still dropped (Willem de Bruijn)
>   - include virtio_net.h in net/core/dev.c so the static key
>     definition sees its declaration (build warning reported by
>     Jakub Kicinski)
> 
> net: add ip4_parse()
>   - add the missing kernel-doc Return: section
>     (Jakub Kicinski)
> 
> 
> Changes since v4:
> 
> net: account for ingress-resolved virtio net hdr state
>   - count on tun only: dropped the tap, macvtap, ipvtap and
>     virtio_net counters (Willem de Bruijn)
>   - the driver side branches are in their final form here now,
>     moved from "net: resolve uncommon partial checksums at
>     ingress" (Willem de Bruijn)
> 
> net: resolve uncommon partial checksums at ingress
>   - no longer touches the drivers (Willem de Bruijn)
>   - a packet too small to be GSO: ignore the tunnel GSO request,
>     same as plain GSO ones, and complete wild metadata in place
>     (sashiko)
>   - document the zero copy interaction (sashiko)
>   - honor a preset skb->protocol on ethernet: the stack uses it,
>     except on loopback which re-parses the frame (sashiko)
> 
> net: add a sysctl to opt out of virtio net hdr checksum validation
>   - document the netns lifetime requirement (sashiko)
> 
> net: add ip4_parse()
>   - pull the whole header, options included, so that the
>     transport header offset is within the linear data
>     (Willem de Bruijn)
> 
> Tested: full suite on the final tree, tunnel included.
> um part compiled only.
> 
> 
> Michael S. Tsirkin (7):
>   net: account for ingress-resolved virtio net hdr state
>   um: vector: trim skbs before verifying the header
>   virtio_net: parse tunnel csum_start little-endian
>   net: resolve uncommon partial checksums at ingress
>   net: add a sysctl to opt out of virtio net hdr checksum validation
>   net: add ip4_parse()
>   virtio_net: cleanup protocol calculation
> 
>  Documentation/admin-guide/sysctl/net.rst |  19 ++
>  arch/um/drivers/vector_kern.c            |   6 +-
>  arch/um/drivers/vector_transports.c      |   4 +-
>  drivers/net/tap.c                        |   4 +-
>  drivers/net/tun.c                        |  41 +++-
>  drivers/net/virtio_net.c                 |   2 +-
>  include/linux/ip.h                       |  37 +++
>  include/linux/virtio_net.h               | 287 ++++++++++++++++++++++-
>  include/net/netns/core.h                 |   1 +
>  net/bridge/br_netfilter_hooks.c          |  15 +-
>  net/core/dev.c                           |   7 +
>  net/core/net_namespace.c                 |   1 +
>  net/core/sysctl_net_core.c               |  50 ++++
>  net/ipv4/ip_input.c                      |  13 +-
>  net/ipv4/netfilter/nf_reject_ipv4.c      |  13 +-
>  net/packet/af_packet.c                   |   4 +-
>  16 files changed, 461 insertions(+), 43 deletions(-)
> 
> -- 
> MST
>