[PATCH net v2] net: openvswitch: make transport checksums writable

[PATCH net v2] net: openvswitch: make transport checksums writable

[Kyle Zeng]

An IP address rewrite can execute even if the transport-header pull in
key_extract_l3l4() failed. In that case the transport key is cleared,
but the IP protocol and transport offset remain available. The address
rewrite helpers check the total skb length before updating the transport
checksum. This does not guarantee that the checksum is in the linear
head, or that it is writable.

An AI-assisted source review identified the missing writability checks.
The supplied userspace reproducer sends non-linear IPv6 packets with
long, valid extension-header chains through TAP and Open vSwitch. It
triggers a size-2 KASAN access in inet_proto_csum_replace16() as UID
1000 in an owned user/network namespace. The reproduction uses natural
allocation pressure, without kernel changes or error injection. IPv4
TCP/UDP and IPv6 TCP/UDP/ICMPv6 address rewrites have the same issue.

Replace the total-length checks with skb_ensure_writable() immediately
before each transport-checksum access. Keep an unavailable transport
checksum nonfatal, as with the existing truncated-header handling.
Preserve the old address values across the operation and reload packet
pointers afterwards, since even a failed pull can have moved the skb
head. Retain the existing unchanged-address, later-fragment, and IPv6
routing-header behavior.

The Open vSwitch module builds with GCC and W=1. The changed object
also builds with allyesconfig and allmodconfig. Code review covers the
common SET action paths and adjacent checksum writers. The supplied
reproducer and kernel selftests have not been rerun against the rebuilt
module. Codex assisted with the patch review, validation, and report
preparation.

The issue was found by an LLM.

Fixes: ccb1352e76cf ("net: Add Open vSwitch kernel components.")
Fixes: 3fdbd1ce11e5 ("openvswitch: add ipv6 'set' action")
Link: https://lore.kernel.org/r/20261030042304.74291-1-kylebot@openai.com
Suggested-by: Ilya Maximets <i.maximets@ovn.org>
Suggested-by: Aaron Conole <aconole@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Kyle Zeng <kylebot@openai.com>
---
v2:
- Put writability checks directly in the transport-checksum branches.
- Keep an unavailable L4 checksum update nonfatal.
- Save the original addresses and refresh all potentially stale packet
  pointers, including after a failed pull.
- Target net, include all maintainers, and describe discovery and testing.
v1: https://lore.kernel.org/r/20261030042304.74291-1-kylebot@openai.com

I have a reproducer for this issue and can share through DM.

 net/openvswitch/actions.c | 56 +++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 20 deletions(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 12e339cbf791..fe472e950cb1 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -319,20 +319,22 @@ static int pop_nsh(struct sk_buff *skb, struct sw_flow_key *key)
 	return 0;
 }
 
-static void update_ip_l4_checksum(struct sk_buff *skb, struct iphdr *nh,
-				  __be32 addr, __be32 new_addr)
+static void update_ip_l4_checksum(struct sk_buff *skb, __be32 addr,
+				  __be32 new_addr)
 {
-	int transport_len = skb->len - skb_transport_offset(skb);
+	struct iphdr *nh = ip_hdr(skb);
 
 	if (nh->frag_off & htons(IP_OFFSET))
 		return;
 
 	if (nh->protocol == IPPROTO_TCP) {
-		if (likely(transport_len >= sizeof(struct tcphdr)))
+		if (likely(!skb_ensure_writable(skb, skb_transport_offset(skb) +
+					       sizeof(struct tcphdr))))
 			inet_proto_csum_replace4(&tcp_hdr(skb)->check, skb,
 						 addr, new_addr, true);
 	} else if (nh->protocol == IPPROTO_UDP) {
-		if (likely(transport_len >= sizeof(struct udphdr))) {
+		if (likely(!skb_ensure_writable(skb, skb_transport_offset(skb) +
+					       sizeof(struct udphdr)))) {
 			struct udphdr *uh = udp_hdr(skb);
 
 			if (uh->check || skb->ip_summed == CHECKSUM_PARTIAL) {
@@ -345,11 +347,15 @@ static void update_ip_l4_checksum(struct sk_buff *skb, struct iphdr *nh,
 	}
 }
 
-static void set_ip_addr(struct sk_buff *skb, struct iphdr *nh,
-			__be32 *addr, __be32 new_addr)
+/* May move the skb head; callers must reload packet pointers. */
+static void set_ip_addr(struct sk_buff *skb, __be32 *addr, __be32 new_addr)
 {
-	update_ip_l4_checksum(skb, nh, *addr, new_addr);
-	csum_replace4(&nh->check, *addr, new_addr);
+	unsigned int addr_offset = (u8 *)addr - skb_network_header(skb);
+	__be32 old_addr = *addr;
+
+	update_ip_l4_checksum(skb, old_addr, new_addr);
+	addr = (__be32 *)(skb_network_header(skb) + addr_offset);
+	csum_replace4(&ip_hdr(skb)->check, old_addr, new_addr);
 	skb_clear_hash(skb);
 	ovs_ct_clear(skb, NULL);
 	*addr = new_addr;
@@ -358,19 +364,17 @@ static void set_ip_addr(struct sk_buff *skb, struct iphdr *nh,
 static void update_ipv6_checksum(struct sk_buff *skb, u8 l4_proto,
 				 __be32 addr[4], const __be32 new_addr[4])
 {
-	int transport_len;
-
 	if (l4_proto == NEXTHDR_FRAGMENT)
 		return;
 
-	transport_len = skb->len - skb_transport_offset(skb);
-
 	if (l4_proto == NEXTHDR_TCP) {
-		if (likely(transport_len >= sizeof(struct tcphdr)))
+		if (likely(!skb_ensure_writable(skb, skb_transport_offset(skb) +
+					       sizeof(struct tcphdr))))
 			inet_proto_csum_replace16(&tcp_hdr(skb)->check, skb,
 						  addr, new_addr, true);
 	} else if (l4_proto == NEXTHDR_UDP) {
-		if (likely(transport_len >= sizeof(struct udphdr))) {
+		if (likely(!skb_ensure_writable(skb, skb_transport_offset(skb) +
+					       sizeof(struct udphdr)))) {
 			struct udphdr *uh = udp_hdr(skb);
 
 			if (uh->check || skb->ip_summed == CHECKSUM_PARTIAL) {
@@ -381,7 +385,8 @@ static void update_ipv6_checksum(struct sk_buff *skb, u8 l4_proto,
 			}
 		}
 	} else if (l4_proto == NEXTHDR_ICMP) {
-		if (likely(transport_len >= sizeof(struct icmp6hdr)))
+		if (likely(!skb_ensure_writable(skb, skb_transport_offset(skb) +
+					       sizeof(struct icmp6hdr))))
 			inet_proto_csum_replace16(&icmp6_hdr(skb)->icmp6_cksum,
 						  skb, addr, new_addr, true);
 	}
@@ -396,12 +401,19 @@ static void mask_ipv6_addr(const __be32 old[4], const __be32 addr[4],
 	masked[3] = OVS_MASKED(old[3], addr[3], mask[3]);
 }
 
+/* May move the skb head; callers must reload packet pointers. */
 static void set_ipv6_addr(struct sk_buff *skb, u8 l4_proto,
 			  __be32 addr[4], const __be32 new_addr[4],
 			  bool recalculate_csum)
 {
-	if (recalculate_csum)
-		update_ipv6_checksum(skb, l4_proto, addr, new_addr);
+	unsigned int addr_offset = (u8 *)addr - skb_network_header(skb);
+	__be32 old_addr[4];
+
+	if (recalculate_csum) {
+		memcpy(old_addr, addr, sizeof(old_addr));
+		update_ipv6_checksum(skb, l4_proto, old_addr, new_addr);
+		addr = (__be32 *)(skb_network_header(skb) + addr_offset);
+	}
 
 	skb_clear_hash(skb);
 	ovs_ct_clear(skb, NULL);
@@ -479,16 +491,18 @@ static int set_ipv4(struct sk_buff *skb, struct sw_flow_key *flow_key,
 		new_addr = OVS_MASKED(nh->saddr, key->ipv4_src, mask->ipv4_src);
 
 		if (unlikely(new_addr != nh->saddr)) {
-			set_ip_addr(skb, nh, &nh->saddr, new_addr);
+			set_ip_addr(skb, &nh->saddr, new_addr);
 			flow_key->ipv4.addr.src = new_addr;
+			nh = ip_hdr(skb);
 		}
 	}
 	if (mask->ipv4_dst) {
 		new_addr = OVS_MASKED(nh->daddr, key->ipv4_dst, mask->ipv4_dst);
 
 		if (unlikely(new_addr != nh->daddr)) {
-			set_ip_addr(skb, nh, &nh->daddr, new_addr);
+			set_ip_addr(skb, &nh->daddr, new_addr);
 			flow_key->ipv4.addr.dst = new_addr;
+			nh = ip_hdr(skb);
 		}
 	}
 	if (mask->ipv4_tos) {
@@ -537,6 +551,7 @@ static int set_ipv6(struct sk_buff *skb, struct sw_flow_key *flow_key,
 				      true);
 			memcpy(&flow_key->ipv6.addr.src, masked,
 			       sizeof(flow_key->ipv6.addr.src));
+			nh = ipv6_hdr(skb);
 		}
 	}
 	if (is_ipv6_mask_nonzero(mask->ipv6_dst)) {
@@ -559,6 +574,7 @@ static int set_ipv6(struct sk_buff *skb, struct sw_flow_key *flow_key,
 				      recalc_csum);
 			memcpy(&flow_key->ipv6.addr.dst, masked,
 			       sizeof(flow_key->ipv6.addr.dst));
+			nh = ipv6_hdr(skb);
 		}
 	}
 	if (mask->ipv6_tclass) {

base-commit: 05441d697f7f9a927264f4058cbbc6a03dfcac9d
-- 
2.55.0.openai.911.g06873a30d2f3

Re: [PATCH net v2] net: openvswitch: make transport checksums writable

[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] The patch handles every skb_ensure_writable() failure in
  update_ip_l4_checksum() and update_ipv6_checksum()…

--

net: openvswitch: make transport checksums writable

This patch changes the IPv4 and IPv6 address rewrite helpers in Open
vSwitch. Each L4 checksum update now calls skb_ensure_writable() first,
instead of checking skb->len. A failure stays nonfatal, and packet
pointers are reloaded afterwards because the skb head may have moved.

Link: https://lore.kernel.org/r/20261030042304.74291-1-kylebot@openai.com

> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 12e339cbf7912..fe472e950cb1a 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -319,20 +319,22 @@ static int pop_nsh(struct sk_buff *skb, struct sw_flow_key *key)
>  	return 0;
>  }
>  
> -static void update_ip_l4_checksum(struct sk_buff *skb, struct iphdr *nh,
> -				  __be32 addr, __be32 new_addr)
> +static void update_ip_l4_checksum(struct sk_buff *skb, __be32 addr,
> +				  __be32 new_addr)
>  {
> -	int transport_len = skb->len - skb_transport_offset(skb);
> +	struct iphdr *nh = ip_hdr(skb);
>  
>  	if (nh->frag_off & htons(IP_OFFSET))
>  		return;
>  
>  	if (nh->protocol == IPPROTO_TCP) {
> -		if (likely(transport_len >= sizeof(struct tcphdr)))
> +		if (likely(!skb_ensure_writable(skb, skb_transport_offset(skb) +
> +					       sizeof(struct tcphdr))))
>  			inet_proto_csum_replace4(&tcp_hdr(skb)->check, skb,
>  						 addr, new_addr, true);

[Severity: Medium]
Is it intended that every skb_ensure_writable() failure here is handled
the same way as a truncated transport header?

skb_ensure_writable() can fail even when the full TCP header is in the
packet. pskb_may_pull()->__pskb_pull_tail() returns NULL in three cases:
pskb_expand_head(GFP_ATOMIC) fails, a frag_list skb_clone() fails, or the
frags are unreadable:

net/core/skbuff.c:__pskb_pull_tail() {
    ...
	if (!skb_frags_readable(skb))
		return NULL;

	if (eat > 0 || skb_cloned(skb)) {
		if (pskb_expand_head(skb, 0, eat > 0 ? eat + 128 : 0,
				     GFP_ATOMIC))
			return NULL;
	}
    ...
}

pskb_expand_head(GFP_ATOMIC) can also fail later for a cloned skb.

In those cases the L4 checksum update is skipped. set_ip_addr() still
updates the IP header checksum and writes the new address:

	update_ip_l4_checksum(skb, old_addr, new_addr);
	addr = (__be32 *)(skb_network_header(skb) + addr_offset);
	csum_replace4(&ip_hdr(skb)->check, old_addr, new_addr);
	...
	*addr = new_addr;

set_ipv6_addr() does the same with memcpy(addr, new_addr, ...). Then
set_ipv4() or set_ipv6() returns 0.

Would the packet then be forwarded with a rewritten pseudo-header and a
stale TCP/UDP/ICMPv6 checksum?

With CHECKSUM_NONE or CHECKSUM_COMPLETE the checksum on the wire would be
wrong. With CHECKSUM_PARTIAL (including GSO), the stale seed in th->check
or uh->check would give a wrong final checksum for every segment.

The commit message says the reproducer relies on "natural allocation
pressure". That is the same condition that would make this new
skb_ensure_writable() call fail.

Other writers in the same file treat this failure as fatal. For example,
set_tcp():

	err = skb_ensure_writable(skb, skb_transport_offset(skb) +
				  sizeof(struct tcphdr));
	if (unlikely(err))
		return err;

set_udp() and set_sctp() do the same, so do_execute_actions() drops the
skb with OVS_DROP_ACTION_ERROR.

Could the update be skipped only when skb->len - skb_transport_offset(skb)
is smaller than the header size, with the skb_ensure_writable() error
returned in all other cases? That would need set_ip_addr() and
set_ipv6_addr() to return int.

The same pattern applies to the UDP branch here, and to the TCP, UDP and
ICMPv6 branches in update_ipv6_checksum().

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107040238.27606-1-kylebot%40openai.com