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

[Ilya Maximets]

On 10/30/26 3:53 PM, Aaron Conole wrote:
> Ilya Maximets <i.maximets@xxxxxxx> 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@xxxxxxxxxx>
>>> ---
>>> 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.