Re: [ovs-dev] [PATCH net] net: openvswitch: validate transport header presence in set_ipv6_addr

[Fernando Fernandez Mancera <fmancera@suse.de>]

On 10/2/26 12:10 AM, Ilya Maximets wrote:
> On 10/1/26 3:29 PM, Fernando Fernandez Mancera wrote:
>> On 10/1/26 2:00 PM, Ilya Maximets wrote:
>>> On 10/1/26 1:20 PM, Fernando Fernandez Mancera via dev wrote:
>>>> On 10/1/26 1:15 PM, Ilya Maximets wrote:
>>>>> On 10/1/26 10:52 AM, Fernando Fernandez Mancera wrote:
>>>>>> When executing IPv6 address rewrite actions on IPv6 fragments,
>>>>>> set_ipv6_addr() might attempt to recalculate the L4 header checksum
>>>>>> calling skb_transport_header() with an uninitialized transport header.
>>>>>> This can potentially lead to a OOB write.
>>>>>
>>>>> Not really.  later fragments have l4_proto set to NEXTHDR_FRAGMENT
>>>>> and so no writes will be performed.  See parse_ipv6hdr().
>>>>>
>>>>
>>>> Oh, I didn't notice that. In that case, this is harmless. I will update
>>>> the commit message, thanks!
>>>>
>>>
>>> [...]
>>>
>>>>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>>>>>> index dc5ff859f114..a14668565ec4 100644
>>>>>> --- a/net/openvswitch/actions.c
>>>>>> +++ b/net/openvswitch/actions.c
>>>>>> @@ -395,7 +395,7 @@ static void set_ipv6_addr(struct sk_buff *skb, u8 l4_proto,
>>>>>>     			  __be32 addr[4], const __be32 new_addr[4],
>>>>>>     			  bool recalculate_csum)
>>>>>>     {
>>>>>> -	if (recalculate_csum)
>>>>>> +	if (recalculate_csum && skb_transport_header_was_set(skb))
>>>>>>     		update_ipv6_checksum(skb, l4_proto, addr, new_addr);
>>>>> I'd suggest moving the check into update_ipv6_checksum() and maybe add
>>>>> a small comment that the check is only to avoid warning on reading the
>>>>> offset that wasn't set, offset must always be set for any l4_proto for
>>>>> which the checksum is actually getting updated.
>>>>>
>>>>
>>>> Will do that. Thanks Ilya.
>>> Actually, it might be better to check for NEXTHDR_FRAGMENT and return
>>> early only in this case.  This way if we ever have the offset missing
>>> for any l4_ptoto that needs it, we'll still get a warning instead of
>>> silently not updating the checksum.  This will be close to what the
>>> update_ip_l4_checksum() is doing.
>>>
>>> At the same time, update_ip_l4_checksum() is able to get the transport
>>> offset without a warning, so another alternative is to actually set the
>>> transport offset in parse_ipv6hdr() to something like frag_off, which
>>> would closer match the behavior of ipv4 where check_iphdr() sets the
>>> transport header unconditionally to the next byte after the network header.
>>> This is fine because transport headers are never accessed when they are
>>> not set in the key.  The network checksum update cases are the only ones
>>> where the actions are even allowed to execute and touch higher level
>>> headers that may not be present in the packet.  At the same time the
>>> frag_off is kind of an arbitrary value, so I'm not sure.
>>>
>>> WDYT?
>>
>> I noticed this but IMHO it is a worse pattern. In the sense that one
>> persone in the future might use the transport header without noticing
>> this and the warning won't show up because the transport header will be
>> set incorrectly. This could cause an actual OOB.
> 
> Yeah, that's true.  It's better to keep the transport header unset if
> there is actually no transport header.  We may also adjust the ipv4 path
> to avoid having it set for later fragments, but that's unrelated for the
> issue at hand.
> 

I do not mind giving a try to that in net-next. Good, so I will send the 
v2 with the proper check and commit message.

Thanks,
Fernando.

>> I will trust your taste in this matter since I am not that familiar with
>> the code.
> 
> Let's just go with the check inside update_ipv6_checksum() before reading
> the value.  Seems like the best option.
> 
> Best regards, Ilya Maximets.
> 
>>
>> Thanks,
>> Fernando.
>>
>>> Eelco, Aaron, do you maybe have a preference?
>>>
>>> Best regards, Ilya Maximets.
>>
>