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

[Ilya Maximets <i.maximets@ovn.org>]

On 11/7/26 5:02 AM, Kyle Zeng wrote:
> 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

There is no reproducer supplied with this patch, so please drop that
part from the commit message or re-word.

> 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.

This paragraph also makes no sense in the commit message.

> 
> The issue was found by an LLM.

You already said that in the second paragraph.

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

This link shouldn't be here.  Patch will be linked on apply.

For the sashiko complaint, the resoning should be part of the commit
message, so the bots and the people reading it in the future understand
why the decision to skip the l4 checksum update instead of failing was
made.

> Suggested-by: Ilya Maximets <i.maximets@ovn.org>
> Suggested-by: Aaron Conole <aconole@redhat.com>

I don't think Aaron suggested this implementation.

> 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))))

This and all other similar lines are misaligned.  It shoudl be a tab
instead of 7 spaces.

>  			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;

There is too much unnecesary and unclear pointer ariphmetic happening.
It may be better if we just pass a boolean here and select nh->saddr
or nh->daddr with simple ternaries or if statements.  It would be much
easier to read and it will be less error prone.  Something like this
(not tested):

 /* May move the skb head; callers must reload packet pointers. */
-static void set_ip_addr(struct sk_buff *skb, __be32 *addr, __be32 new_addr)
+static void set_ip_addr(struct sk_buff *skb, __be32 new_addr, bool saddr)
 {
-       unsigned int addr_offset = (u8 *)addr - skb_network_header(skb);
-       __be32 old_addr = *addr;
+       __be32 old_addr = saddr ? ip_hdr(skb)->saddr : ip_hdr(skb)->daddr;
 
        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;
+
+       if (saddr)
+               ip_hdr(skb)->saddr = new_addr;
+       else
+               ip_hdr(skb)->daddr = 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;

There is no point checking this anymore.

>  
> -	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);
> +	}

Same here, too much of unnecessary and obscure ariphmetic.  Suggesting:

 /* 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)
+                         const __be32 new_addr[4],
+                         bool saddr, bool recalculate_csum)
 {
-       unsigned int addr_offset = (u8 *)addr - skb_network_header(skb);
-       __be32 old_addr[4];
+       struct ipv6_hdr nh = ipv6_hdr(skb);
 
        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);
+               update_ipv6_checksum(skb, l4_proto,
+                                    saddr ? &nh->saddr : &nh->daddr,
+                                    new_addr);
        }
 
        skb_clear_hash(skb);
        ovs_ct_clear(skb, NULL);
-       memcpy(addr, new_addr, sizeof(__be32[4]));
+
+       nh = ipv6_hdr(skb);
+       if (saddr)
+               memcpy(&nh->saddr, new_addr, sizeof(nh->saddr));
+       else
+               memcpy(&nh->daddr, new_addr, sizeof(nh->daddr));
 }


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