mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH net 0/3] xsk, ipv6: validate user-supplied checksum offsets
@ 2026-10-21 18:03 Nick Simpson
  2026-10-21 18:03 ` [PATCH net 1/3] xsk: validate checksum offload requests in copy mode Nick Simpson
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Nick Simpson @ 2026-10-21 18:03 UTC (permalink / raw)
  To: Magnus Karlsson, Maciej Fijalkowski, David Ahern, Ido Schimmel
  Cc: Stanislav Fomichev, David S . Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Simon Horman, Pablo Neira Ayuso,
	Florian Westphal, Phil Sutter, Alexei Starovoitov,
	Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
	Michael S . Tsirkin, Willem de Bruijn, Jason Xing,
	Song Yoong Siang, Rusty Russell, Max Krasnyansky,
	Sridhar Samudrala, Jonathan Corbet, Shuah Khan, Randy Dunlap,
	netdev, linux-doc, bpf, netfilter-devel, coreteam, linux-kernel

In copy mode, AF_XDP turns the csum_start/csum_offset of
XDP_TXMD_FLAGS_CHECKSUM into a CHECKSUM_PARTIAL skb after checking only
that the checksum fits in the frame. Sent over a veth into a namespace
that forwards IPv6 with nf_defrag_ipv6, the checksum is then completed
after the forward hook has accepted the packet, at an offset in the IPv6
header chosen by the sender; with csum_start before the network header,
skb_checksum_help() warns. Patch 1 has the details and the trace.

This is the AF_XDP counterpart of the virtio_net_hdr issue fixed for
IPv4 by commit 6785011f8b16 ("ipv4: validate checksum_start before
completing checksum") and addressed by the "more virtio-net csum
hardening" series [1], which does not cover AF_XDP.

Patch 1 is the fix: CHECKSUM_PARTIAL is kept only for the TCP or UDP
checksum of an unfragmented IPv4/IPv6 packet, not IP-in-IP or GRE
encapsulated, on an Ethernet device; other requests are completed in
software at the sender. Frames without a linear part
(IFF_TX_SKB_NO_LINEAR) are left unchanged. One visible change: a request
on a frame that is itself an IP fragment is completed over that fragment
alone, as a device would, so the reassembled datagram no longer has a
valid transport checksum and conntrack marks it invalid; before, if
every fragment carried a request, it stayed CHECKSUM_PARTIAL and was
completed on refragmentation.

Patch 2 documents the new behaviour in xsk-tx-metadata.rst. It is kept
separate so that patch 1 stays within the stable size limit.

Patch 3 is defence in depth: the IPv6 counterpart of
ip_check_csum_start() on the two IPv6 fragmentation paths,
ip6_fragment() and br_ip6_fragment(). For IPv4 the check is also
called from nfnetlink_queue, xt_CHECKSUM and xfrm_output; their IPv6
counterparts are not changed here, and a check at reassembly time would
cover more. Patch 1 does not depend on it, and it is not marked for
stable.

Testing, on netdev/net 82e31923c055, arm64 under QEMU:
- reproducer (fragmented IPv6/UDP over veth into a namespace with an
  nftables forward rule on "ip6 saddr", csum_start/csum_offset 22/22;
  an unfragmented packet with 14/22 and TX checksum offload off on the
  egress veth; csum_start 20): without the series the source address is
  rewritten after the forward hook accepted the packet, and
  skb_checksum_help() warns. With the series, or with patch 1 alone,
  nothing is rewritten after the accept and there is no warning; with
  patch 3 alone the fragmented cases are not sent on (ip6_fragment()
  drops them) and the unfragmented case is not covered
- a legitimate IPv6/UDP request (csum_start at the UDP header, offset 6)
  is still sent CHECKSUM_PARTIAL and delivered to a UDP socket
- which requests stay CHECKSUM_PARTIAL, observed on the veth peer with
  PACKET_AUXDATA: IPv4/UDP, IPv4/TCP, IPv4 with options, IPv6/UDP, IPv6
  with a hop-by-hop header and VLAN-tagged IPv6/UDP stay partial; a
  wrong csum_offset, csum_start in the IP header, IPv4 and IPv6 first
  fragments, IPIP and ICMP are completed in software with the correct
  value. Without the series all of them stay partial.
- tools/testing/selftests/bpf: test_progs -t xdp_metadata passes with
  and without the series
- on a kernel with KASAN, PROVE_LOCKING, PROVE_RCU,
  DEBUG_ATOMIC_SLEEP, DEBUG_PREEMPT, DEBUG_PAGEALLOC and
  DEBUG_OBJECTS_RCU_HEAD, no warnings: with the series, the
  reproducer, the classification test and the selftest; without it,
  the selftest and the legitimate request
- W=1 and sparse (C=1) on the changed files in allmodconfig builds for
  arm64, x86_64, i386 and s390: no warnings, with or without the
  series; checkstack unchanged; arm64 allnoconfig and an INET=n build
  with XDP_SOCKETS=y build; checkpatch --strict is clean; htmldocs for
  networking builds without warnings for xsk-tx-metadata.rst
- arm64 allmodconfig and allyesconfig with W=1 build, with no warning
  in the changed files (CONFIG_WERROR and the DRM *_WERROR options
  were turned off: with gcc 15, existing W=1 warnings in unrelated
  files otherwise stop the build)
Not tested: IFF_TX_SKB_NO_LINEAR (virtio_net) devices, multi-buffer
frames, an attached BPF flow dissector, a sender in an unprivileged
user namespace, x86 at runtime, and patch 3 with tun/tap or packet
socket sources of CHECKSUM_PARTIAL (the same header rewrite through a
tap device was reproduced on mainline earlier, but not re-run with
patch 3).

The issue was found, and the patches and tests prepared, with LLM
assistance (see the Assisted-by tags). I have reviewed the changes and
the test results. A reproducer is available on request.

[1] https://lore.kernel.org/r/cover.1792411304.git.mst@redhat.com

Nick Simpson (3):
  xsk: validate checksum offload requests in copy mode
  xsk: document checksum offload handling in copy mode
  ipv6: validate checksum_start before fragmenting

 Documentation/networking/xsk-tx-metadata.rst |  7 ++-
 include/net/ipv6.h                           | 12 ++++
 net/ipv6/ip6_output.c                        | 10 +++-
 net/ipv6/netfilter.c                         | 10 +++-
 net/xdp/xsk.c                                | 58 ++++++++++++++++++++
 5 files changed, 90 insertions(+), 7 deletions(-)


base-commit: 82e31923c0553fdbfcddcd910ca07952880ca9b9
-- 
2.53.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH net 1/3] xsk: validate checksum offload requests in copy mode
  2026-10-21 18:03 [PATCH net 0/3] xsk, ipv6: validate user-supplied checksum offsets Nick Simpson
@ 2026-10-21 18:03 ` Nick Simpson
  2026-10-21 18:03 ` [PATCH net 2/3] xsk: document checksum offload handling " Nick Simpson
  2026-10-21 18:03 ` [PATCH net 3/3] ipv6: validate checksum_start before fragmenting Nick Simpson
  2 siblings, 0 replies; 7+ messages in thread
From: Nick Simpson @ 2026-10-21 18:03 UTC (permalink / raw)
  To: Magnus Karlsson, Maciej Fijalkowski, David Ahern, Ido Schimmel
  Cc: Stanislav Fomichev, David S . Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Simon Horman, Pablo Neira Ayuso,
	Florian Westphal, Phil Sutter, Alexei Starovoitov,
	Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
	Michael S . Tsirkin, Willem de Bruijn, Jason Xing,
	Song Yoong Siang, Rusty Russell, Max Krasnyansky,
	Sridhar Samudrala, Jonathan Corbet, Shuah Khan, Randy Dunlap,
	netdev, linux-doc, bpf, netfilter-devel, coreteam, linux-kernel

In copy mode, XDP_TXMD_FLAGS_CHECKSUM turns the csum_start and
csum_offset from the TX metadata into a CHECKSUM_PARTIAL skb.
xsk_skb_metadata() only checks that the checksum fits in the frame, so
csum_start can point into the Ethernet or IP header.

On a veth, the skb reaches the peer namespace still CHECKSUM_PARTIAL.
When that namespace forwards IPv6 with nf_defrag_ipv6 loaded,
nf_ct_frag6_reasm() moves the IPv6 header forward by the 8 bytes of the
fragment header without adjusting csum_start, the forward hook accepts
the reassembled packet, and ip6_fragment() then completes the checksum
with skb_checksum_help(), writing two bytes into the accepted header.
With csum_start = 22 and csum_offset = 22 on a fragmented IPv6/UDP
datagram from fd00:1::100, the write lands on source address bytes
14-15: an nftables forward rule matching "ip6 saddr fd00:1::100"
accepted the packet and the fragments sent out carried saddr
fd00:1::e87. For an unfragmented packet the same write happens at
egress when the output device does not offload the checksum; with
csum_start = 14 and csum_offset = 22 the packet left as fd00:1::ebf. A
csum_start that ends up before the network header (14 to 21 on the
fragmented path) makes skb_checksum_help() warn instead:

  offset (-2) >= skb_headlen() (640)
  WARNING: net/core/dev.c:3647 at skb_checksum_help+0x4c8/0x850
  Call trace:
   skb_checksum_help
   ip6_fragment
   ip6_finish_output
   ip6_output
   ip6_forward
   ipv6_rcv
   ...
   __dev_direct_xmit
   __xsk_generic_xmit
   xsk_sendmsg

socket(AF_XDP) only requires CAP_NET_RAW in the user namespace that owns
the network namespace, so the sender can be a container that owns one
end of the veth. With panic_on_warn, the warning panics the forwarding
host.

The interface is documented for L4 checksums. Leave CHECKSUM_PARTIAL in
place only when the request designates the TCP or UDP checksum field of
an unfragmented IPv4 or IPv6 packet, not IP-in-IP or GRE encapsulated,
on an Ethernet device, and complete any other request in software, as
XDP_UMEM_TX_SW_CSUM would, before the frame is handed to the device.
The frame is dissected from its Ethernet type, as skb->protocol is not
set for xsk skbs. The pending "net: resolve uncommon partial checksums
at ingress" applies the same rule to virtio_net_hdr sources.

A request on a frame that is itself an IP fragment is therefore
completed over that fragment alone, which is also all a device could
compute, so the reassembled datagram does not carry a valid transport
checksum. Before, if every fragment carried such a request, the
reassembled datagram stayed CHECKSUM_PARTIAL and was completed on
refragmentation; now conntrack marks it invalid.

Skbs without a linear part (IFF_TX_SKB_NO_LINEAR, i.e. virtio_net) are
left unchanged: their data stays in the umem, where user space can
still modify it after any check, and they go to the device rather than
to the local stack.

The problem was found with LLM-assisted code review.

Fixes: 48eb03dd2630 ("xsk: Add TX timestamp and TX checksum offload support")
Link: https://lore.kernel.org/r/9c08d12b3a7819ae03ded6a154e4f908276e93d4.1792411304.git.mst@redhat.com
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Nick Simpson <nick.simo@gmail.com>
---
 net/xdp/xsk.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 781ba46eb152..a1cf701d42fc 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -11,6 +11,8 @@
 
 #define pr_fmt(fmt) "AF_XDP: %s: " fmt, __func__
 
+#include <linux/if_arp.h>
+#include <linux/if_ether.h>
 #include <linux/if_xdp.h>
 #include <linux/init.h>
 #include <linux/sched/mm.h>
@@ -21,6 +23,8 @@
 #include <linux/uaccess.h>
 #include <linux/net.h>
 #include <linux/netdevice.h>
+#include <linux/tcp.h>
+#include <linux/udp.h>
 #include <linux/rculist.h>
 #include <linux/uio.h>
 #include <linux/vmalloc.h>
@@ -28,6 +32,7 @@
 #include <net/netdev_queues.h>
 #include <net/xdp_sock_drv.h>
 #include <net/busy_poll.h>
+#include <net/flow_dissector.h>
 #include <net/netdev_lock.h>
 #include <net/netdev_rx_queue.h>
 #include <net/xdp.h>
@@ -985,6 +990,54 @@ static void xsk_drop_skb(struct sk_buff *skb)
 	xs->skb = NULL;
 }
 
+/* Only TCP and UDP checksums of unfragmented IP packets that are not
+ * IP-in-IP or GRE encapsulated are left to the device. A CHECKSUM_PARTIAL
+ * skb can reach the local stack (e.g. through veth), and completing an
+ * arbitrary csum_start there may write into headers that were already
+ * inspected, e.g. after IPv6 reassembly. skb->protocol is not set for
+ * xsk skbs, so the frame is dissected from its Ethernet type instead.
+ */
+static bool xsk_csum_partial_ok(const struct sk_buff *skb)
+{
+	const struct ethhdr *eth = (const struct ethhdr *)skb->data;
+	struct flow_keys_basic keys;
+
+	if (skb->dev->type != ARPHRD_ETHER || skb_headlen(skb) < ETH_HLEN)
+		return false;
+
+	if (!skb_flow_dissect_flow_keys_basic(dev_net(skb->dev), NULL, &keys,
+					      skb->data, eth->h_proto, ETH_HLEN,
+					      skb_headlen(skb),
+					      FLOW_DISSECTOR_F_STOP_AT_ENCAP))
+		return false;
+
+	if ((keys.control.flags & FLOW_DIS_IS_FRAGMENT) ||
+	    skb_checksum_start_offset(skb) != keys.control.thoff)
+		return false;
+
+	switch (keys.basic.ip_proto) {
+	case IPPROTO_TCP:
+		return skb->csum_offset == offsetof(struct tcphdr, check);
+	case IPPROTO_UDP:
+		return skb->csum_offset == offsetof(struct udphdr, check);
+	default:
+		return false;
+	}
+}
+
+static int xsk_skb_resolve_csum(struct sk_buff *skb)
+{
+	/* Without a linear part (IFF_TX_SKB_NO_LINEAR), the frame stays in
+	 * the umem, where user space can still change it, and goes to the
+	 * device rather than the local stack: leave it as it is.
+	 */
+	if (skb->ip_summed != CHECKSUM_PARTIAL || !skb_headlen(skb) ||
+	    xsk_csum_partial_ok(skb))
+		return 0;
+
+	return skb_checksum_help(skb);
+}
+
 static int xsk_skb_metadata(struct sk_buff *skb, void *buffer,
 			    struct xdp_desc *desc, struct xsk_buff_pool *pool,
 			    u32 hr)
@@ -1277,6 +1330,11 @@ static int __xsk_generic_xmit(struct sock *sk)
 			continue;
 		}
 
+		if (unlikely(xsk_skb_resolve_csum(skb))) {
+			xsk_drop_skb(skb);
+			continue;
+		}
+
 		err = __dev_direct_xmit(skb, xs->queue_id);
 		if  (err == NETDEV_TX_BUSY) {
 			/* Tell user-space to retry the send */
-- 
2.53.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH net 2/3] xsk: document checksum offload handling in copy mode
  2026-10-21 18:03 [PATCH net 0/3] xsk, ipv6: validate user-supplied checksum offsets Nick Simpson
  2026-10-21 18:03 ` [PATCH net 1/3] xsk: validate checksum offload requests in copy mode Nick Simpson
@ 2026-10-21 18:03 ` Nick Simpson
  2026-10-21 18:03 ` [PATCH net 3/3] ipv6: validate checksum_start before fragmenting Nick Simpson
  2 siblings, 0 replies; 7+ messages in thread
From: Nick Simpson @ 2026-10-21 18:03 UTC (permalink / raw)
  To: Magnus Karlsson, Maciej Fijalkowski, David Ahern, Ido Schimmel
  Cc: Stanislav Fomichev, David S . Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Simon Horman, Pablo Neira Ayuso,
	Florian Westphal, Phil Sutter, Alexei Starovoitov,
	Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
	Michael S . Tsirkin, Willem de Bruijn, Jason Xing,
	Song Yoong Siang, Rusty Russell, Max Krasnyansky,
	Sridhar Samudrala, Jonathan Corbet, Shuah Khan, Randy Dunlap,
	netdev, linux-doc, bpf, netfilter-devel, coreteam, linux-kernel

In copy mode, XDP_TXMD_FLAGS_CHECKSUM requests are now left to the
device only for the TCP or UDP checksum of an unfragmented IPv4 or IPv6
packet, not IP-in-IP or GRE encapsulated, on an Ethernet device; other
requests are completed in software before transmission. On devices that
take skbs without a linear part (IFF_TX_SKB_NO_LINEAR), every request is
still left to the device. Say so in the description of the flag.

Assisted-by: LLM
Signed-off-by: Nick Simpson <nick.simo@gmail.com>
---
 Documentation/networking/xsk-tx-metadata.rst | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/xsk-tx-metadata.rst b/Documentation/networking/xsk-tx-metadata.rst
index f240930ca1df..028564dba43d 100644
--- a/Documentation/networking/xsk-tx-metadata.rst
+++ b/Documentation/networking/xsk-tx-metadata.rst
@@ -50,7 +50,12 @@ The flags field enables the particular offload:
 - ``XDP_TXMD_FLAGS_CHECKSUM``: requests the device to calculate L4
   checksum. ``request.csum_start`` specifies byte offset of where the
   checksumming should start and ``request.csum_offset`` specifies byte offset
-  where the device should store the computed checksum.
+  where the device should store the computed checksum. In copy mode
+  (``XDP_COPY``), only the TCP or UDP checksum of an unfragmented IPv4 or
+  IPv6 packet, not IP-in-IP or GRE encapsulated, on an Ethernet device is
+  left to the device; other requests are completed in software before the
+  packet is transmitted. On devices that take skbs without a linear part
+  (``IFF_TX_SKB_NO_LINEAR``), every request is still left to the device.
 - ``XDP_TXMD_FLAGS_LAUNCH_TIME``: requests the device to schedule the
   packet for transmission at a pre-determined time called launch time. The
   value of launch time is indicated by ``request.launch_time`` field of
-- 
2.53.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH net 3/3] ipv6: validate checksum_start before fragmenting
  2026-10-21 18:03 [PATCH net 0/3] xsk, ipv6: validate user-supplied checksum offsets Nick Simpson
  2026-10-21 18:03 ` [PATCH net 1/3] xsk: validate checksum offload requests in copy mode Nick Simpson
  2026-10-21 18:03 ` [PATCH net 2/3] xsk: document checksum offload handling " Nick Simpson
@ 2026-10-21 18:03 ` Nick Simpson
  2026-10-21 18:20   ` Eric Dumazet
  2 siblings, 1 reply; 7+ messages in thread
From: Nick Simpson @ 2026-10-21 18:03 UTC (permalink / raw)
  To: Magnus Karlsson, Maciej Fijalkowski, David Ahern, Ido Schimmel
  Cc: Stanislav Fomichev, David S . Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Simon Horman, Pablo Neira Ayuso,
	Florian Westphal, Phil Sutter, Alexei Starovoitov,
	Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
	Michael S . Tsirkin, Willem de Bruijn, Jason Xing,
	Song Yoong Siang, Rusty Russell, Max Krasnyansky,
	Sridhar Samudrala, Jonathan Corbet, Shuah Khan, Randy Dunlap,
	netdev, linux-doc, bpf, netfilter-devel, coreteam, linux-kernel

ip6_fragment() and br_ip6_fragment() complete CHECKSUM_PARTIAL skbs with
skb_checksum_help() without checking where csum_start points. After
reassembly by nf_defrag_ipv6, the IPv6 header has moved forward by the
8 bytes of the fragment header while csum_start has not, so a
CHECKSUM_PARTIAL skb from an untrusted source (tun/tap or packet sockets
with a virtio_net_hdr, or AF_XDP TX metadata) can have csum_start inside
the IPv6 header, and the checksum is then written into a header that the
forward hook has already accepted. When csum_start lies before the
network header, skb_checksum_help() warns.

Add the IPv6 counterpart of ip_check_csum_start() from
commit 6785011f8b16 ("ipv4: validate checksum_start before completing
checksum") to the two IPv6 fragmentation paths: refuse to complete a
checksum that starts inside the unfragmentable part of the packet (the
IPv6 header and the extension headers before the fragment header) and
drop the packet instead.

For IPv4, ip_check_csum_start() is also called from nfnetlink_queue,
xt_CHECKSUM and xfrm_output; their IPv6 counterparts are not changed
here. A reassembled packet that is not fragmented again, such as an
atomic fragment, and an unfragmented packet whose checksum is completed
at egress are not covered either; those have to be handled where the
CHECKSUM_PARTIAL skb is created, or by a check at reassembly time.

The problem was found with LLM-assisted code review.

Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
Fixes: bfd5f4a3d605 ("packet: Add GSO/csum offload support.")
Fixes: 48eb03dd2630 ("xsk: Add TX timestamp and TX checksum offload support")
Assisted-by: LLM
Signed-off-by: Nick Simpson <nick.simo@gmail.com>
---
 include/net/ipv6.h    | 12 ++++++++++++
 net/ipv6/ip6_output.c | 10 +++++++---
 net/ipv6/netfilter.c  | 10 +++++++---
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 3de07e738538..949695cb6764 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -1029,6 +1029,18 @@ int ip6_xmit(const struct sock *sk, struct sk_buff *skb, struct flowi6 *fl6,
 
 int ip6_find_1stfragopt(struct sk_buff *skb, u8 **nexthdr);
 
+/* Refuse to complete a checksum that starts inside the unfragmentable
+ * part of the packet, i.e. the IPv6 header and the extension headers
+ * that precede the fragment header.
+ */
+static inline int ip6_check_csum_start(const struct sk_buff *skb,
+				       unsigned int hlen)
+{
+	if (unlikely(skb->csum_start < skb->network_header + hlen))
+		return -EINVAL;
+	return 0;
+}
+
 int ip6_append_data(struct sock *sk,
 		    int getfrag(void *from, char *to, int offset, int len,
 				int odd, struct sk_buff *skb),
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 550965058991..55d7af9084d5 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -942,9 +942,13 @@ int ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb,
 	frag_id = ipv6_select_ident(net, &ipv6_hdr(skb)->daddr,
 				    &ipv6_hdr(skb)->saddr);
 
-	if (skb->ip_summed == CHECKSUM_PARTIAL &&
-	    (err = skb_checksum_help(skb)))
-		goto fail;
+	if (skb->ip_summed == CHECKSUM_PARTIAL) {
+		err = ip6_check_csum_start(skb, hlen);
+		if (!err)
+			err = skb_checksum_help(skb);
+		if (err)
+			goto fail;
+	}
 
 	prevhdr = skb_network_header(skb) + nexthdr_offset;
 	hroom = LL_RESERVED_SPACE(rt->dst.dev);
diff --git a/net/ipv6/netfilter.c b/net/ipv6/netfilter.c
index a7025ec87035..8476e4ba399c 100644
--- a/net/ipv6/netfilter.c
+++ b/net/ipv6/netfilter.c
@@ -144,9 +144,13 @@ int br_ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb,
 	frag_id = ipv6_select_ident(net, &ipv6_hdr(skb)->daddr,
 				    &ipv6_hdr(skb)->saddr);
 
-	if (skb->ip_summed == CHECKSUM_PARTIAL &&
-	    (err = skb_checksum_help(skb)))
-		goto blackhole;
+	if (skb->ip_summed == CHECKSUM_PARTIAL) {
+		err = ip6_check_csum_start(skb, hlen);
+		if (!err)
+			err = skb_checksum_help(skb);
+		if (err)
+			goto blackhole;
+	}
 
 	prevhdr = skb_network_header(skb) + nexthdr_offset;
 	hroom = LL_RESERVED_SPACE(skb->dev);
-- 
2.53.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net 3/3] ipv6: validate checksum_start before fragmenting
  2026-10-21 18:03 ` [PATCH net 3/3] ipv6: validate checksum_start before fragmenting Nick Simpson
@ 2026-10-21 18:20   ` Eric Dumazet
  2026-10-21 18:47     ` Eric Dumazet
  0 siblings, 1 reply; 7+ messages in thread
From: Eric Dumazet @ 2026-10-21 18:20 UTC (permalink / raw)
  To: Nick Simpson
  Cc: Magnus Karlsson, Maciej Fijalkowski, David Ahern, Ido Schimmel,
	Stanislav Fomichev, David S . Miller, Jakub Kicinski,
	Paolo Abeni, Simon Horman, Pablo Neira Ayuso, Florian Westphal,
	Phil Sutter, Alexei Starovoitov, Daniel Borkmann,
	Jesper Dangaard Brouer, John Fastabend, Michael S . Tsirkin,
	Willem de Bruijn, Jason Xing, Song Yoong Siang, Rusty Russell,
	Max Krasnyansky, Sridhar Samudrala, Jonathan Corbet, Shuah Khan,
	Randy Dunlap, netdev, linux-doc, bpf, netfilter-devel, coreteam,
	linux-kernel

Le mer. 21 oct. 2026 à 20:05, Nick Simpson <nick.simo@gmail.com> a écrit :
>
> ip6_fragment() and br_ip6_fragment() complete CHECKSUM_PARTIAL skbs with
> skb_checksum_help() without checking where csum_start points. After
> reassembly by nf_defrag_ipv6, the IPv6 header has moved forward by the
> 8 bytes of the fragment header while csum_start has not, so a
> CHECKSUM_PARTIAL skb from an untrusted source (tun/tap or packet sockets
> with a virtio_net_hdr, or AF_XDP TX metadata) can have csum_start inside
> the IPv6 header, and the checksum is then written into a header that the
> forward hook has already accepted. When csum_start lies before the
> network header, skb_checksum_help() warns.
>
> Add the IPv6 counterpart of ip_check_csum_start() from
> commit 6785011f8b16 ("ipv4: validate checksum_start before completing
> checksum") to the two IPv6 fragmentation paths: refuse to complete a
> checksum that starts inside the unfragmentable part of the packet (the
> IPv6 header and the extension headers before the fragment header) and
> drop the packet instead.
>
> For IPv4, ip_check_csum_start() is also called from nfnetlink_queue,
> xt_CHECKSUM and xfrm_output; their IPv6 counterparts are not changed
> here. A reassembled packet that is not fragmented again, such as an
> atomic fragment, and an unfragmented packet whose checksum is completed
> at egress are not covered either; those have to be handled where the
> CHECKSUM_PARTIAL skb is created, or by a check at reassembly time.

Then let's do that, instead of adding checks to every consumer.

A CHECKSUM_PARTIAL fragment is meaningless: the L4 checksum covers the
whole datagram, and our own stack completes it before fragmenting
(ip_do_fragment(), ip6_fragment()). Only untrusted injectors produce
them. Yet inet_frag_reasm_finish() keeps the reassembled skb
CHECKSUM_PARTIAL when all fragments are.

Downgrading CHECKSUM_PARTIAL to CHECKSUM_NONE in inet_frag_reasm_finish()
covers IPv4, IPv6, nf_defrag_ipv6 (and the bridge path) in one place;
the bogus L4 checksum is then verified and the packet dropped.

Also, reassembly does not move the L4 bytes, so a valid csum_start stays
valid; the "header moved without adjusting csum_start" framing is not
the bug.

Finally, this patch is described as defence in depth and not for
stable, but carries three Fixes: tags. Please drop it.

Nacked-by: Eric Dumazet <edumazet@kernel.org>

I am so tired of these LLM patches working around the issues instead
of fixing the root causes.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net 3/3] ipv6: validate checksum_start before fragmenting
  2026-10-21 18:20   ` Eric Dumazet
@ 2026-10-21 18:47     ` Eric Dumazet
  2026-10-21 20:02       ` Michael S. Tsirkin
  0 siblings, 1 reply; 7+ messages in thread
From: Eric Dumazet @ 2026-10-21 18:47 UTC (permalink / raw)
  To: Nick Simpson
  Cc: Magnus Karlsson, Maciej Fijalkowski, David Ahern, Ido Schimmel,
	Stanislav Fomichev, David S . Miller, Jakub Kicinski,
	Paolo Abeni, Simon Horman, Pablo Neira Ayuso, Florian Westphal,
	Phil Sutter, Alexei Starovoitov, Daniel Borkmann,
	Jesper Dangaard Brouer, John Fastabend, Michael S . Tsirkin,
	Willem de Bruijn, Jason Xing, Song Yoong Siang, Rusty Russell,
	Max Krasnyansky, Sridhar Samudrala, Jonathan Corbet, Shuah Khan,
	Randy Dunlap, netdev, linux-doc, bpf, netfilter-devel, coreteam,
	linux-kernel

Le mer. 21 oct. 2026 à 20:20, Eric Dumazet <edumazet@kernel.org> a écrit :
>
> Le mer. 21 oct. 2026 à 20:05, Nick Simpson <nick.simo@gmail.com> a écrit :
> >
> > ip6_fragment() and br_ip6_fragment() complete CHECKSUM_PARTIAL skbs with
> > skb_checksum_help() without checking where csum_start points. After
> > reassembly by nf_defrag_ipv6, the IPv6 header has moved forward by the
> > 8 bytes of the fragment header while csum_start has not, so a
> > CHECKSUM_PARTIAL skb from an untrusted source (tun/tap or packet sockets
> > with a virtio_net_hdr, or AF_XDP TX metadata) can have csum_start inside
> > the IPv6 header, and the checksum is then written into a header that the
> > forward hook has already accepted. When csum_start lies before the
> > network header, skb_checksum_help() warns.
> >
> > Add the IPv6 counterpart of ip_check_csum_start() from
> > commit 6785011f8b16 ("ipv4: validate checksum_start before completing
> > checksum") to the two IPv6 fragmentation paths: refuse to complete a
> > checksum that starts inside the unfragmentable part of the packet (the
> > IPv6 header and the extension headers before the fragment header) and
> > drop the packet instead.
> >
> > For IPv4, ip_check_csum_start() is also called from nfnetlink_queue,
> > xt_CHECKSUM and xfrm_output; their IPv6 counterparts are not changed
> > here. A reassembled packet that is not fragmented again, such as an
> > atomic fragment, and an unfragmented packet whose checksum is completed
> > at egress are not covered either; those have to be handled where the
> > CHECKSUM_PARTIAL skb is created, or by a check at reassembly time.
>
> Then let's do that, instead of adding checks to every consumer.
>
> A CHECKSUM_PARTIAL fragment is meaningless: the L4 checksum covers the
> whole datagram, and our own stack completes it before fragmenting
> (ip_do_fragment(), ip6_fragment()). Only untrusted injectors produce
> them. Yet inet_frag_reasm_finish() keeps the reassembled skb
> CHECKSUM_PARTIAL when all fragments are.
>
> Downgrading CHECKSUM_PARTIAL to CHECKSUM_NONE in inet_frag_reasm_finish()
> covers IPv4, IPv6, nf_defrag_ipv6 (and the bridge path) in one place;
> the bogus L4 checksum is then verified and the packet dropped.
>
>

Scratch that suggestion, it would be one more consumer-side check.

The fix belongs where CHECKSUM_PARTIAL skbs are created, and Michael
is already doing this for virtio_net_hdr sources (tun, tap, packet
sockets, virtio_net):

https://lore.kernel.org/netdev/cover.1792411304.git.mst@redhat.com/

In particular "net: resolve uncommon partial checksums at ingress":
https://lore.kernel.org/netdev/9c08d12b3a7819ae03ded6a154e4f908276e93d4.1792411304.git.mst@redhat.com/
CHECKSUM_PARTIAL is only kept when csum_start/csum_offset match a
parsed TCP or UDP header, and fragments are explicitly refused
(ip_is_fragment(), IP6_FH_F_FRAG): their checksum is completed at
ingress, before any protocol processing.

Your patch 1/3 applies the same rule to AF_XDP copy mode, and already
links to Michael's patch.

With both, no untrusted source can feed a CHECKSUM_PARTIAL fragment
to reassembly, nor an unfragmented packet with a bogus csum_start, so
neither this patch nor a reassembly change is needed.

Once the ingress validation is merged, the ip_check_csum_start() calls
added by commit 6785011f8b16 can be removed as well.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net 3/3] ipv6: validate checksum_start before fragmenting
  2026-10-21 18:47     ` Eric Dumazet
@ 2026-10-21 20:02       ` Michael S. Tsirkin
  0 siblings, 0 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2026-10-21 20:02 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Nick Simpson, Magnus Karlsson, Maciej Fijalkowski, David Ahern,
	Ido Schimmel, Stanislav Fomichev, David S . Miller,
	Jakub Kicinski, Paolo Abeni, Simon Horman, Pablo Neira Ayuso,
	Florian Westphal, Phil Sutter, Alexei Starovoitov,
	Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
	Willem de Bruijn, Jason Xing, Song Yoong Siang, Rusty Russell,
	Max Krasnyansky, Sridhar Samudrala, Jonathan Corbet, Shuah Khan,
	Randy Dunlap, netdev, linux-doc, bpf, netfilter-devel, coreteam,
	linux-kernel

On Wed, Oct 21, 2026 at 08:47:40PM +0200, Eric Dumazet wrote:
> Le mer. 21 oct. 2026 à 20:20, Eric Dumazet <edumazet@kernel.org> a écrit :
> >
> > Le mer. 21 oct. 2026 à 20:05, Nick Simpson <nick.simo@gmail.com> a écrit :
> > >
> > > ip6_fragment() and br_ip6_fragment() complete CHECKSUM_PARTIAL skbs with
> > > skb_checksum_help() without checking where csum_start points. After
> > > reassembly by nf_defrag_ipv6, the IPv6 header has moved forward by the
> > > 8 bytes of the fragment header while csum_start has not, so a
> > > CHECKSUM_PARTIAL skb from an untrusted source (tun/tap or packet sockets
> > > with a virtio_net_hdr, or AF_XDP TX metadata) can have csum_start inside
> > > the IPv6 header, and the checksum is then written into a header that the
> > > forward hook has already accepted. When csum_start lies before the
> > > network header, skb_checksum_help() warns.
> > >
> > > Add the IPv6 counterpart of ip_check_csum_start() from
> > > commit 6785011f8b16 ("ipv4: validate checksum_start before completing
> > > checksum") to the two IPv6 fragmentation paths: refuse to complete a
> > > checksum that starts inside the unfragmentable part of the packet (the
> > > IPv6 header and the extension headers before the fragment header) and
> > > drop the packet instead.
> > >
> > > For IPv4, ip_check_csum_start() is also called from nfnetlink_queue,
> > > xt_CHECKSUM and xfrm_output; their IPv6 counterparts are not changed
> > > here. A reassembled packet that is not fragmented again, such as an
> > > atomic fragment, and an unfragmented packet whose checksum is completed
> > > at egress are not covered either; those have to be handled where the
> > > CHECKSUM_PARTIAL skb is created, or by a check at reassembly time.
> >
> > Then let's do that, instead of adding checks to every consumer.
> >
> > A CHECKSUM_PARTIAL fragment is meaningless: the L4 checksum covers the
> > whole datagram, and our own stack completes it before fragmenting
> > (ip_do_fragment(), ip6_fragment()). Only untrusted injectors produce
> > them. Yet inet_frag_reasm_finish() keeps the reassembled skb
> > CHECKSUM_PARTIAL when all fragments are.
> >
> > Downgrading CHECKSUM_PARTIAL to CHECKSUM_NONE in inet_frag_reasm_finish()
> > covers IPv4, IPv6, nf_defrag_ipv6 (and the bridge path) in one place;
> > the bogus L4 checksum is then verified and the packet dropped.
> >
> >
> 
> Scratch that suggestion, it would be one more consumer-side check.
> 
> The fix belongs where CHECKSUM_PARTIAL skbs are created, and Michael
> is already doing this for virtio_net_hdr sources (tun, tap, packet
> sockets, virtio_net):
> 
> https://lore.kernel.org/netdev/cover.1792411304.git.mst@redhat.com/
> 
> In particular "net: resolve uncommon partial checksums at ingress":
> https://lore.kernel.org/netdev/9c08d12b3a7819ae03ded6a154e4f908276e93d4.1792411304.git.mst@redhat.com/
> CHECKSUM_PARTIAL is only kept when csum_start/csum_offset match a
> parsed TCP or UDP header, and fragments are explicitly refused
> (ip_is_fragment(), IP6_FH_F_FRAG): their checksum is completed at
> ingress, before any protocol processing.
> 
> Your patch 1/3 applies the same rule to AF_XDP copy mode, and already
> links to Michael's patch.

Oh great, AF_XDP. Another one no one noticed.

> With both, no untrusted source can feed a CHECKSUM_PARTIAL fragment
> to reassembly, nor an unfragmented packet with a bogus csum_start, so
> neither this patch nor a reassembly change is needed.
> 
> Once the ingress validation is merged, the ip_check_csum_start() calls
> added by commit 6785011f8b16 can be removed as well.

I think this would be a mistake. While plugging all places where some
packet change can confuse the stack is hard, making at least sure
there's no slab oob if we missed one is a no brainer imho: this is the
extent of the overhead:


        if (unlikely(skb->csum_start < skb->network_header + ip_hdrlen(skb)))
                return -EINVAL;
        return 0;

I will be frank, messing up one of the subtle checks we are adding in
virtio_net_hdr_from_skb is *trivially easy*. It is getting them right
that is hard. Can we pretty please get help in not turning every such
mistake into a publicity stunt with most major cloud vendors affected by
guest driven host code execution?

I know, we should write less bugs (
Still, you guys are the net maintainers.

-- 
MST


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-10-21 20:02 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-21 18:03 [PATCH net 0/3] xsk, ipv6: validate user-supplied checksum offsets Nick Simpson
2026-10-21 18:03 ` [PATCH net 1/3] xsk: validate checksum offload requests in copy mode Nick Simpson
2026-10-21 18:03 ` [PATCH net 2/3] xsk: document checksum offload handling " Nick Simpson
2026-10-21 18:03 ` [PATCH net 3/3] ipv6: validate checksum_start before fragmenting Nick Simpson
2026-10-21 18:20   ` Eric Dumazet
2026-10-21 18:47     ` Eric Dumazet
2026-10-21 20:02       ` Michael S. Tsirkin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®