Re: [PATCH] net: openvswitch: make transport checksums writable
flat view
From: Ilya Maximets <i.maximets@ovn.org>
Date: 2026-10-30 11:28:16
Also in:
lkml
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.
quoted hunk ↗ jump to hunk
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 <redacted> --- 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.