[PATCH] net: openvswitch: make transport checksums writable

[PATCH] 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.

A non-linear IPv6 packet with a long, valid extension-header chain can
therefore cause an out-of-bounds checksum read/modify/write when the
GFP_ATOMIC head expansion fails. KASAN reports the access in
inet_proto_csum_replace16(). The IPv4 TCP/UDP and IPv6 TCP/UDP/ICMPv6
address rewrite paths all have the same missing check.

Make the applicable transport header writable before changing an
address, and propagate failure through the existing action error path.
Reload cached network-header and address pointers after the operation,
since it can move the skb head. Preserve the existing behavior for
unchanged addresses, truncated transport headers, non-first fragments,
and IPv6 destination rewrites that do not affect the pseudo-header.

Fixes: ccb1352e76cf ("net: Add Open vSwitch kernel components.")
Fixes: 3fdbd1ce11e5 ("openvswitch: add ipv6 'set' action")
Signed-off-by: Kyle Zeng <kylebot@openai.com>
---
I have a reproducer for this issue and can share via DM

 net/openvswitch/actions.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 12e339cbf791..36c2a1963222 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -319,6 +319,35 @@ static int pop_nsh(struct sk_buff *skb, struct sw_flow_key *key)
 	return 0;
 }
 
+static int ensure_l4_checksum_writable(struct sk_buff *skb, u8 l4_proto)
+{
+	unsigned int offset, len;
+
+	switch (l4_proto) {
+	case IPPROTO_TCP:
+		len = sizeof(struct tcphdr);
+		break;
+	case IPPROTO_UDP:
+		len = sizeof(struct udphdr);
+		break;
+	case IPPROTO_ICMPV6:
+		len = sizeof(struct icmp6hdr);
+		break;
+	default:
+		return 0;
+	}
+
+	offset = skb_transport_offset(skb);
+	if (unlikely(offset > skb->len))
+		return -EINVAL;
+
+	/* Truncated transport headers have no checksum to update. */
+	if (unlikely(skb->len - offset < len))
+		return 0;
+
+	return skb_ensure_writable(skb, offset + len);
+}
+
 static void update_ip_l4_checksum(struct sk_buff *skb, struct iphdr *nh,
 				  __be32 addr, __be32 new_addr)
 {
@@ -462,6 +491,7 @@ static int set_ipv4(struct sk_buff *skb, struct sw_flow_key *flow_key,
 {
 	struct iphdr *nh;
 	__be32 new_addr;
+	u8 l4_proto;
 	int err;
 
 	err = skb_ensure_writable(skb, skb_network_offset(skb) +
@@ -470,6 +500,10 @@ static int set_ipv4(struct sk_buff *skb, struct sw_flow_key *flow_key,
 		return err;
 
 	nh = ip_hdr(skb);
+	l4_proto = nh->protocol;
+	if (nh->frag_off & htons(IP_OFFSET) ||
+	    (l4_proto != IPPROTO_TCP && l4_proto != IPPROTO_UDP))
+		l4_proto = 0;
 
 	/* Setting an IP addresses is typically only a side effect of
 	 * matching on them in the current userspace implementation, so it
@@ -479,6 +513,11 @@ 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)) {
+			err = ensure_l4_checksum_writable(skb, l4_proto);
+			if (unlikely(err))
+				return err;
+
+			nh = ip_hdr(skb);
 			set_ip_addr(skb, nh, &nh->saddr, new_addr);
 			flow_key->ipv4.addr.src = new_addr;
 		}
@@ -487,6 +526,11 @@ static int set_ipv4(struct sk_buff *skb, struct sw_flow_key *flow_key,
 		new_addr = OVS_MASKED(nh->daddr, key->ipv4_dst, mask->ipv4_dst);
 
 		if (unlikely(new_addr != nh->daddr)) {
+			err = ensure_l4_checksum_writable(skb, l4_proto);
+			if (unlikely(err))
+				return err;
+
+			nh = ip_hdr(skb);
 			set_ip_addr(skb, nh, &nh->daddr, new_addr);
 			flow_key->ipv4.addr.dst = new_addr;
 		}
@@ -533,6 +577,13 @@ static int set_ipv6(struct sk_buff *skb, struct sw_flow_key *flow_key,
 		mask_ipv6_addr(saddr, key->ipv6_src, mask->ipv6_src, masked);
 
 		if (unlikely(memcmp(saddr, masked, sizeof(masked)))) {
+			err = ensure_l4_checksum_writable(skb,
+							  flow_key->ip.proto);
+			if (unlikely(err))
+				return err;
+
+			nh = ipv6_hdr(skb);
+			saddr = (__be32 *)&nh->saddr;
 			set_ipv6_addr(skb, flow_key->ip.proto, saddr, masked,
 				      true);
 			memcpy(&flow_key->ipv6.addr.src, masked,
@@ -555,6 +606,15 @@ static int set_ipv6(struct sk_buff *skb, struct sw_flow_key *flow_key,
 							     NULL, &flags)
 					       != NEXTHDR_ROUTING);
 
+			if (recalc_csum) {
+				err = ensure_l4_checksum_writable(skb,
+								  flow_key->ip.proto);
+				if (unlikely(err))
+					return err;
+
+				nh = ipv6_hdr(skb);
+				daddr = (__be32 *)&nh->daddr;
+			}
 			set_ipv6_addr(skb, flow_key->ip.proto, daddr, masked,
 				      recalc_csum);
 			memcpy(&flow_key->ipv6.addr.dst, masked,

base-commit: d89e1d186ec9271bf78e62771779e9fc03bf0fc3
-- 
2.56.0

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Ilya Maximets]

On 10/30/26 5:23 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.
> 
> A non-linear IPv6 packet with a long, valid extension-header chain can
> therefore cause an out-of-bounds checksum read/modify/write when the
> GFP_ATOMIC head expansion fails. KASAN reports the access in
> inet_proto_csum_replace16(). The IPv4 TCP/UDP and IPv6 TCP/UDP/ICMPv6
> address rewrite paths all have the same missing check.

While sending patches, make sure you Cc all maintainers.  And you need
to specify the target tree, which is 'net' in this case.

> 
> Make the applicable transport header writable before changing an
> address, and propagate failure through the existing action error path.
> Reload cached network-header and address pointers after the operation,
> since it can move the skb head. Preserve the existing behavior for
> unchanged addresses, truncated transport headers, non-first fragments,
> and IPv6 destination rewrites that do not affect the pseudo-header.
> 
> Fixes: ccb1352e76cf ("net: Add Open vSwitch kernel components.")
> Fixes: 3fdbd1ce11e5 ("openvswitch: add ipv6 'set' action")
> Signed-off-by: Kyle Zeng <kylebot@openai.com>
> ---
> I have a reproducer for this issue and can share via DM
> 
>  net/openvswitch/actions.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 12e339cbf791..36c2a1963222 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -319,6 +319,35 @@ static int pop_nsh(struct sk_buff *skb, struct sw_flow_key *key)
>  	return 0;
>  }
>  
> +static int ensure_l4_checksum_writable(struct sk_buff *skb, u8 l4_proto)
> +{
> +	unsigned int offset, len;
> +
> +	switch (l4_proto) {
> +	case IPPROTO_TCP:
> +		len = sizeof(struct tcphdr);
> +		break;
> +	case IPPROTO_UDP:
> +		len = sizeof(struct udphdr);
> +		break;
> +	case IPPROTO_ICMPV6:
> +		len = sizeof(struct icmp6hdr);
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	offset = skb_transport_offset(skb);
> +	if (unlikely(offset > skb->len))
> +		return -EINVAL;
> +
> +	/* Truncated transport headers have no checksum to update. */
> +	if (unlikely(skb->len - offset < len))
> +		return 0;

All the checks above are already executed at different places on the same
path.  With this and the new checks in set_ipv4(), the l4 protocol is
checked at least 3 times for no good reason.

While inability to update L3 header is fatal for set_ipv4/6(), failure to
update L4 checksum should not be.  And so, there is no point propagating
the error.  If the header doesn't exist - there is nothing to update.  And
if we ran out of memory trying to expand the linear parts of the skb, then
we have a more significant problem at hand than a non-updated L4 checksum.
In the worst case, the destination will just drop the packet.

Can we just replace trnasport_len checks with skb_ensure_writable()?

It sounds like that would be sufficient here.  And re-fetch the 'nh'
pointers right after the set_ipv4/6() calls with an inline comment like
this:

@@ -481,6 +480,7 @@ static int set_ipv4(struct sk_buff *skb, struct sw_flow_key *flow_key,
                if (unlikely(new_addr != nh->saddr)) {
                        set_ip_addr(skb, nh, &nh->saddr, new_addr);
                        flow_key->ipv4.addr.src = new_addr;
+                       nh = ip_hdr(skb); /* Potentially reallocated. */
                }
        }
        if (mask->ipv4_dst) {
---

Creating a separate function with all the duplicated checks doesn't make
much sense to me.

Best regards, Ilya Maximets.

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

[Aaron Conole]

Ilya Maximets <i.maximets@ovn.org> writes:

> On 10/30/26 5:23 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.
>> 
>> A non-linear IPv6 packet with a long, valid extension-header chain can
>> therefore cause an out-of-bounds checksum read/modify/write when the
>> GFP_ATOMIC head expansion fails. KASAN reports the access in
>> inet_proto_csum_replace16(). The IPv4 TCP/UDP and IPv6 TCP/UDP/ICMPv6
>> address rewrite paths all have the same missing check.
>
> While sending patches, make sure you Cc all maintainers.  And you need
> to specify the target tree, which is 'net' in this case.
>
>> 
>> Make the applicable transport header writable before changing an
>> address, and propagate failure through the existing action error path.
>> Reload cached network-header and address pointers after the operation,
>> since it can move the skb head. Preserve the existing behavior for
>> unchanged addresses, truncated transport headers, non-first fragments,
>> and IPv6 destination rewrites that do not affect the pseudo-header.
>> 
>> Fixes: ccb1352e76cf ("net: Add Open vSwitch kernel components.")
>> Fixes: 3fdbd1ce11e5 ("openvswitch: add ipv6 'set' action")
>> Signed-off-by: Kyle Zeng <kylebot@openai.com>
>> ---
>> I have a reproducer for this issue and can share via DM
>> 
>>  net/openvswitch/actions.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 60 insertions(+)
>> 
>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> index 12e339cbf791..36c2a1963222 100644
>> --- a/net/openvswitch/actions.c
>> +++ b/net/openvswitch/actions.c
>> @@ -319,6 +319,35 @@ static int pop_nsh(struct sk_buff *skb, struct sw_flow_key *key)
>>  	return 0;
>>  }
>>  
>> +static int ensure_l4_checksum_writable(struct sk_buff *skb, u8 l4_proto)
>> +{
>> +	unsigned int offset, len;
>> +
>> +	switch (l4_proto) {
>> +	case IPPROTO_TCP:
>> +		len = sizeof(struct tcphdr);
>> +		break;
>> +	case IPPROTO_UDP:
>> +		len = sizeof(struct udphdr);
>> +		break;
>> +	case IPPROTO_ICMPV6:
>> +		len = sizeof(struct icmp6hdr);
>> +		break;
>> +	default:
>> +		return 0;
>> +	}
>> +
>> +	offset = skb_transport_offset(skb);
>> +	if (unlikely(offset > skb->len))
>> +		return -EINVAL;
>> +
>> +	/* Truncated transport headers have no checksum to update. */
>> +	if (unlikely(skb->len - offset < len))
>> +		return 0;
>
> All the checks above are already executed at different places on the same
> path.  With this and the new checks in set_ipv4(), the l4 protocol is
> checked at least 3 times for no good reason.
>
> While inability to update L3 header is fatal for set_ipv4/6(), failure to
> update L4 checksum should not be.  And so, there is no point propagating
> the error.  If the header doesn't exist - there is nothing to update.  And
> if we ran out of memory trying to expand the linear parts of the skb, then
> we have a more significant problem at hand than a non-updated L4 checksum.
> In the worst case, the destination will just drop the packet.

Agreed.

We already do an skb_ensure_writable() on entry into the set_ipv(4/6)
functions that covers the IP header portion.  It's probably too early
there to try pulling, unless we want to bleed protocol details up one
level, and propagate down whether or not something like a pskb_may_pull
had an error (to skip updating the l4 region).  That would mean doing
the re-assignment of nh once.

> Can we just replace trnasport_len checks with skb_ensure_writable()?

If we do that, the nh can become stale within the scope of the
set_ip_addr function, too, I think (skb_ensure_writable inside
update_l4_* functions pulls and potentially unclones the head to a new
region).  So we might have to change the set_ip_* functions to fetch and
re-fetch their own header details.

> It sounds like that would be sufficient here.  And re-fetch the 'nh'
> pointers right after the set_ipv4/6() calls with an inline comment like
> this:
>
> @@ -481,6 +480,7 @@ static int set_ipv4(struct sk_buff *skb, struct sw_flow_key *flow_key,
>                 if (unlikely(new_addr != nh->saddr)) {
>                         set_ip_addr(skb, nh, &nh->saddr, new_addr);
>                         flow_key->ipv4.addr.src = new_addr;
> +                       nh = ip_hdr(skb); /* Potentially reallocated. */
>                 }
>         }
>         if (mask->ipv4_dst) {
> ---
>
> Creating a separate function with all the duplicated checks doesn't make
> much sense to me.

+1

My personal preference is to do the header movement once at the start,
since it keeps the write paths looking cleaner - but I don't know if
there's really a performance cost in some traffic cases.

WDYT?

> Best regards, Ilya Maximets.

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

[Ilya Maximets]

On 10/30/26 3:53 PM, Aaron Conole wrote:
> Ilya Maximets <i.maximets@ovn.org> writes:
> 
>> On 10/30/26 5:23 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.
>>>
>>> A non-linear IPv6 packet with a long, valid extension-header chain can
>>> therefore cause an out-of-bounds checksum read/modify/write when the
>>> GFP_ATOMIC head expansion fails. KASAN reports the access in
>>> inet_proto_csum_replace16(). The IPv4 TCP/UDP and IPv6 TCP/UDP/ICMPv6
>>> address rewrite paths all have the same missing check.
>>
>> While sending patches, make sure you Cc all maintainers.  And you need
>> to specify the target tree, which is 'net' in this case.
>>
>>>
>>> Make the applicable transport header writable before changing an
>>> address, and propagate failure through the existing action error path.
>>> Reload cached network-header and address pointers after the operation,
>>> since it can move the skb head. Preserve the existing behavior for
>>> unchanged addresses, truncated transport headers, non-first fragments,
>>> and IPv6 destination rewrites that do not affect the pseudo-header.
>>>
>>> Fixes: ccb1352e76cf ("net: Add Open vSwitch kernel components.")
>>> Fixes: 3fdbd1ce11e5 ("openvswitch: add ipv6 'set' action")
>>> Signed-off-by: Kyle Zeng <kylebot@openai.com>
>>> ---
>>> I have a reproducer for this issue and can share via DM
>>>
>>>  net/openvswitch/actions.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 60 insertions(+)
>>>
>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>>> index 12e339cbf791..36c2a1963222 100644
>>> --- a/net/openvswitch/actions.c
>>> +++ b/net/openvswitch/actions.c
>>> @@ -319,6 +319,35 @@ static int pop_nsh(struct sk_buff *skb, struct sw_flow_key *key)
>>>  	return 0;
>>>  }
>>>  
>>> +static int ensure_l4_checksum_writable(struct sk_buff *skb, u8 l4_proto)
>>> +{
>>> +	unsigned int offset, len;
>>> +
>>> +	switch (l4_proto) {
>>> +	case IPPROTO_TCP:
>>> +		len = sizeof(struct tcphdr);
>>> +		break;
>>> +	case IPPROTO_UDP:
>>> +		len = sizeof(struct udphdr);
>>> +		break;
>>> +	case IPPROTO_ICMPV6:
>>> +		len = sizeof(struct icmp6hdr);
>>> +		break;
>>> +	default:
>>> +		return 0;
>>> +	}
>>> +
>>> +	offset = skb_transport_offset(skb);
>>> +	if (unlikely(offset > skb->len))
>>> +		return -EINVAL;
>>> +
>>> +	/* Truncated transport headers have no checksum to update. */
>>> +	if (unlikely(skb->len - offset < len))
>>> +		return 0;
>>
>> All the checks above are already executed at different places on the same
>> path.  With this and the new checks in set_ipv4(), the l4 protocol is
>> checked at least 3 times for no good reason.
>>
>> While inability to update L3 header is fatal for set_ipv4/6(), failure to
>> update L4 checksum should not be.  And so, there is no point propagating
>> the error.  If the header doesn't exist - there is nothing to update.  And
>> if we ran out of memory trying to expand the linear parts of the skb, then
>> we have a more significant problem at hand than a non-updated L4 checksum.
>> In the worst case, the destination will just drop the packet.
> 
> Agreed.
> 
> We already do an skb_ensure_writable() on entry into the set_ipv(4/6)
> functions that covers the IP header portion.  It's probably too early
> there to try pulling, unless we want to bleed protocol details up one
> level, and propagate down whether or not something like a pskb_may_pull
> had an error (to skip updating the l4 region).  That would mean doing
> the re-assignment of nh once.
> 
>> Can we just replace trnasport_len checks with skb_ensure_writable()?
> 
> If we do that, the nh can become stale within the scope of the
> set_ip_addr function, too, I think (skb_ensure_writable inside
> update_l4_* functions pulls and potentially unclones the head to a new
> region).  So we might have to change the set_ip_* functions to fetch and
> re-fetch their own header details.

Yep.  We could drop the 'nh' argument from set_ip_addr and make it just
use ip_hdr() directly in place of it.

> 
>> It sounds like that would be sufficient here.  And re-fetch the 'nh'
>> pointers right after the set_ipv4/6() calls with an inline comment like
>> this:
>>
>> @@ -481,6 +480,7 @@ static int set_ipv4(struct sk_buff *skb, struct sw_flow_key *flow_key,
>>                 if (unlikely(new_addr != nh->saddr)) {
>>                         set_ip_addr(skb, nh, &nh->saddr, new_addr);
>>                         flow_key->ipv4.addr.src = new_addr;
>> +                       nh = ip_hdr(skb); /* Potentially reallocated. */
>>                 }
>>         }
>>         if (mask->ipv4_dst) {
>> ---
>>
>> Creating a separate function with all the duplicated checks doesn't make
>> much sense to me.
> 
> +1
> 
> My personal preference is to do the header movement once at the start,
> since it keeps the write paths looking cleaner - but I don't know if
> there's really a performance cost in some traffic cases.
I'm not sure I understand what you're suggesting here.  Could you elaborate
or give an example?

Best regards, Ilya Maximets.