Ilya Maximets <[email protected]> writes:
> On 10/30/26 3:53 PM, Aaron Conole wrote:
>> Ilya Maximets <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
