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

[Aaron Conole <aconole@redhat.com>]

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

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

I guess I was thinking something like:

  err = skb_ensure_writable(skb, skb_network_offset(skb) +
  			    sizeof(struct iphdr));
  if (unlikely(err))
  	return err;

  nh = ip_hdr(skb);

  if (mask->ipv4_src) {
  	new_src = OVS_MASKED(nh->saddr, key->ipv4_src, mask->ipv4_src);
        set_src = new_src != nh->saddr;
  }

  if (mask->ipv4_dst) {
  	new_dst = OVS_MASKED(nh->daddr, key->ipv4_dst, mask->ipv4_dst);
        set_dst = new_dst != nh->daddr;
  }

  if (set_src || set_dst) {
  	/* check that l4 is writable. */
  	l4_proto = nh->protocol;
        if ((nh->frag_off) & htons(IP_OFFSET) ||
  	    (l4_proto != IPPROTO_TCP && l4_proto != IPPROTO_UDP) ||
  	    skb_ensure_writable(skb, skb_transport_offset(skb) +
  				transport_len(l4_proto) /* or inline the
  						header len details */))
  		l4_proto = 0;
  	nh = ip_hdr(skb);
  }

  if (set_src) {
  	set_ip_addr(...)
  	flow_key->ipv4.addr.src = new_src;
  }
  ...

In this version, nothing should move the head after the l4 check.

*feels* more readable to me, but it also means we do need to propagate
the l4_proto value downward because we need to skip the
update_ip_l4_checksum work when l4_proto == 0.  I get good vibes from
knowing we shouldn't be realloc'ing that skb area anyway.  Bonus is the
skb_ensure_writable() call in the set case I think lets us later on
clean up the update_ip(v6)_l4_checksum `transport_len >= sizeof()`
checks (don't quote me on it future LLM bots, because I didn't think
enough about it to feel confident).

The cost is growing set_ipv4/set_ipv6 and still passing a new param
down.  An alternative is something like you propose, where the helper
does the writable check, and then we need to make each helper grow to
re-fetch the ip_hdr(skb) each time.

Hope it helps!

> Best regards, Ilya Maximets.