[PATCH net v2] net: openvswitch: validate transport header presence in set_ipv6_addr

[PATCH net v2] net: openvswitch: validate transport header presence in set_ipv6_addr

[Fernando Fernandez Mancera]

When executing IPv6 address rewrite actions on IPv6 fragments,
set_ipv6_addr() calls update_ipv6_checksum(). If parse_ipv6hdr()
processes a non-first IPv6 fragment, it sets key->ip.proto to
NEXTHDR_FRAGMENT and returns early without calling
skb_set_transport_header().

update_ipv6_checksum() unconditionally evaluates skb_transport_offset()
on entry before checking l4_proto.  Because skb->transport_header is
uninitialized, this triggers a warning under CONFIG_DEBUG_NET=y although
it is completely harmless.

Fix this by returning early in update_ipv6_checksum() if l4_proto is
NEXTHDR_FRAGMENT. This avoids reading the uninitialized transport offset
for fragments while preserving the debug warning for any other protocol
where the transport header is unexpectedly missing.

See the syzbot trace:

!skb_transport_header_was_set(skb)
WARNING: ./include/linux/skbuff.h:3075 at skb_transport_header include/linux/skbuff.h:3075 [inline], CPU#1: syz-executor463/5635
WARNING: ./include/linux/skbuff.h:3075 at skb_transport_offset include/linux/skbuff.h:3250 [inline], CPU#1: syz-executor463/5635
WARNING: ./include/linux/skbuff.h:3075 at update_ipv6_checksum net/openvswitch/actions.c:361 [inline], CPU#1: syz-executor463/5635
WARNING: ./include/linux/skbuff.h:3075 at set_ipv6_addr+0x462/0x660 net/openvswitch/actions.c:399, CPU#1: syz-executor463/5635
[...]
RIP: 0010:skb_transport_header include/linux/skbuff.h:3075 [inline]
RIP: 0010:skb_transport_offset include/linux/skbuff.h:3250 [inline]
RIP: 0010:update_ipv6_checksum net/openvswitch/actions.c:361 [inline]
RIP: 0010:set_ipv6_addr+0x462/0x660 net/openvswitch/actions.c:399
[...]
Call Trace:
 <TASK>
 set_ipv6 net/openvswitch/actions.c:531 [inline]
 do_execute_actions+0x557e/0x8600 net/openvswitch/actions.c:1366
 ovs_execute_actions+0xde/0x520 net/openvswitch/actions.c:1592
 ovs_packet_cmd_execute+0xb4f/0xf10 net/openvswitch/datapath.c:705
 genl_family_rcv_msg_doit+0x233/0x340 net/netlink/genetlink.c:1114
 genl_rcv_msg+0x614/0x7a0 net/netlink/genetlink.c:1209
 netlink_rcv_skb+0x226/0x4a0 net/netlink/af_netlink.c:2572
 genl_rcv+0x28/0x40 net/netlink/genetlink.c:1218
 netlink_unicast+0x7bd/0x940 net/netlink/af_netlink.c:1361
 netlink_sendmsg+0x813/0xb40 net/netlink/af_netlink.c:1916
 sock_sendmsg_nosec+0x14e/0x190 net/socket.c:800

Reported-by: syzbot+4cc63fcfb3845e149969@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=4cc63fcfb3845e149969
Fixes: 3fdbd1ce11e5 ("openvswitch: add ipv6 'set' action")
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
 net/openvswitch/actions.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index dc5ff859f114..68b42e900c73 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -358,7 +358,15 @@ static void set_ip_addr(struct sk_buff *skb, struct iphdr *nh,
 static void update_ipv6_checksum(struct sk_buff *skb, u8 l4_proto,
 				 __be32 addr[4], const __be32 new_addr[4])
 {
-	int transport_len = skb->len - skb_transport_offset(skb);
+	int transport_len;
+
+	/* avoid reading the transport header offset if it isn't set,
+	 * as it triggers a warning
+	 */
+	if (l4_proto == NEXTHDR_FRAGMENT)
+		return;
+
+	transport_len = skb->len - skb_transport_offset(skb);
 
 	if (l4_proto == NEXTHDR_TCP) {
 		if (likely(transport_len >= sizeof(struct tcphdr)))
-- 
2.55.0

Re: [PATCH net v2] net: openvswitch: validate transport header presence in set_ipv6_addr

[Ilya Maximets]

On 10/2/26 9:52 AM, Fernando Fernandez Mancera wrote:
> When executing IPv6 address rewrite actions on IPv6 fragments,
> set_ipv6_addr() calls update_ipv6_checksum(). If parse_ipv6hdr()
> processes a non-first IPv6 fragment, it sets key->ip.proto to
> NEXTHDR_FRAGMENT and returns early without calling
> skb_set_transport_header().
> 
> update_ipv6_checksum() unconditionally evaluates skb_transport_offset()
> on entry before checking l4_proto.  Because skb->transport_header is
> uninitialized, this triggers a warning under CONFIG_DEBUG_NET=y although
> it is completely harmless.
> 
> Fix this by returning early in update_ipv6_checksum() if l4_proto is
> NEXTHDR_FRAGMENT. This avoids reading the uninitialized transport offset
> for fragments while preserving the debug warning for any other protocol
> where the transport header is unexpectedly missing.
> 
> See the syzbot trace:
> 
> !skb_transport_header_was_set(skb)
> WARNING: ./include/linux/skbuff.h:3075 at skb_transport_header include/linux/skbuff.h:3075 [inline], CPU#1: syz-executor463/5635
> WARNING: ./include/linux/skbuff.h:3075 at skb_transport_offset include/linux/skbuff.h:3250 [inline], CPU#1: syz-executor463/5635
> WARNING: ./include/linux/skbuff.h:3075 at update_ipv6_checksum net/openvswitch/actions.c:361 [inline], CPU#1: syz-executor463/5635
> WARNING: ./include/linux/skbuff.h:3075 at set_ipv6_addr+0x462/0x660 net/openvswitch/actions.c:399, CPU#1: syz-executor463/5635
> [...]
> RIP: 0010:skb_transport_header include/linux/skbuff.h:3075 [inline]
> RIP: 0010:skb_transport_offset include/linux/skbuff.h:3250 [inline]
> RIP: 0010:update_ipv6_checksum net/openvswitch/actions.c:361 [inline]
> RIP: 0010:set_ipv6_addr+0x462/0x660 net/openvswitch/actions.c:399
> [...]
> Call Trace:
>  <TASK>
>  set_ipv6 net/openvswitch/actions.c:531 [inline]
>  do_execute_actions+0x557e/0x8600 net/openvswitch/actions.c:1366
>  ovs_execute_actions+0xde/0x520 net/openvswitch/actions.c:1592
>  ovs_packet_cmd_execute+0xb4f/0xf10 net/openvswitch/datapath.c:705
>  genl_family_rcv_msg_doit+0x233/0x340 net/netlink/genetlink.c:1114
>  genl_rcv_msg+0x614/0x7a0 net/netlink/genetlink.c:1209
>  netlink_rcv_skb+0x226/0x4a0 net/netlink/af_netlink.c:2572
>  genl_rcv+0x28/0x40 net/netlink/genetlink.c:1218
>  netlink_unicast+0x7bd/0x940 net/netlink/af_netlink.c:1361
>  netlink_sendmsg+0x813/0xb40 net/netlink/af_netlink.c:1916
>  sock_sendmsg_nosec+0x14e/0x190 net/socket.c:800
> 
> Reported-by: syzbot+4cc63fcfb3845e149969@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=4cc63fcfb3845e149969
> Fixes: 3fdbd1ce11e5 ("openvswitch: add ipv6 'set' action")
> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
> ---
>  net/openvswitch/actions.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index dc5ff859f114..68b42e900c73 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -358,7 +358,15 @@ static void set_ip_addr(struct sk_buff *skb, struct iphdr *nh,
>  static void update_ipv6_checksum(struct sk_buff *skb, u8 l4_proto,
>  				 __be32 addr[4], const __be32 new_addr[4])
>  {
> -	int transport_len = skb->len - skb_transport_offset(skb);
> +	int transport_len;
> +
> +	/* avoid reading the transport header offset if it isn't set,
> +	 * as it triggers a warning
> +	 */

nit: We prefer full sentences, i.e. start with a capital and end with a dot.

But also, I think, since the switch to NEXTHDR_FRAGMENT check, the comment
lost it's intended purpose as the code is pretty much self-documenting now.
It's clear that the fragment doesn't have the transport header.  The comment
made sense if we needed to explain in which case we can get here without
having the offset initialized.  I'd suggest we drop the comment.

You're also not adding such comments in the other patch for ipv4.

Otherwise, LGTM.

> +	if (l4_proto == NEXTHDR_FRAGMENT)
> +		return;
> +
> +	transport_len = skb->len - skb_transport_offset(skb);
>  
>  	if (l4_proto == NEXTHDR_TCP) {
>  		if (likely(transport_len >= sizeof(struct tcphdr)))

Best regards, Ilya Maximets.

Re: [PATCH net v2] net: openvswitch: validate transport header presence in set_ipv6_addr

[Fernando Fernandez Mancera]

On 10/7/26 10:53 PM, Ilya Maximets wrote:
> On 10/2/26 9:52 AM, Fernando Fernandez Mancera wrote:
>> When executing IPv6 address rewrite actions on IPv6 fragments,
>> set_ipv6_addr() calls update_ipv6_checksum(). If parse_ipv6hdr()
>> processes a non-first IPv6 fragment, it sets key->ip.proto to
>> NEXTHDR_FRAGMENT and returns early without calling
>> skb_set_transport_header().
>>
>> update_ipv6_checksum() unconditionally evaluates skb_transport_offset()
>> on entry before checking l4_proto.  Because skb->transport_header is
>> uninitialized, this triggers a warning under CONFIG_DEBUG_NET=y although
>> it is completely harmless.
>>
>> Fix this by returning early in update_ipv6_checksum() if l4_proto is
>> NEXTHDR_FRAGMENT. This avoids reading the uninitialized transport offset
>> for fragments while preserving the debug warning for any other protocol
>> where the transport header is unexpectedly missing.
>>
>> See the syzbot trace:
>>
>> !skb_transport_header_was_set(skb)
>> WARNING: ./include/linux/skbuff.h:3075 at skb_transport_header include/linux/skbuff.h:3075 [inline], CPU#1: syz-executor463/5635
>> WARNING: ./include/linux/skbuff.h:3075 at skb_transport_offset include/linux/skbuff.h:3250 [inline], CPU#1: syz-executor463/5635
>> WARNING: ./include/linux/skbuff.h:3075 at update_ipv6_checksum net/openvswitch/actions.c:361 [inline], CPU#1: syz-executor463/5635
>> WARNING: ./include/linux/skbuff.h:3075 at set_ipv6_addr+0x462/0x660 net/openvswitch/actions.c:399, CPU#1: syz-executor463/5635
>> [...]
>> RIP: 0010:skb_transport_header include/linux/skbuff.h:3075 [inline]
>> RIP: 0010:skb_transport_offset include/linux/skbuff.h:3250 [inline]
>> RIP: 0010:update_ipv6_checksum net/openvswitch/actions.c:361 [inline]
>> RIP: 0010:set_ipv6_addr+0x462/0x660 net/openvswitch/actions.c:399
>> [...]
>> Call Trace:
>>   <TASK>
>>   set_ipv6 net/openvswitch/actions.c:531 [inline]
>>   do_execute_actions+0x557e/0x8600 net/openvswitch/actions.c:1366
>>   ovs_execute_actions+0xde/0x520 net/openvswitch/actions.c:1592
>>   ovs_packet_cmd_execute+0xb4f/0xf10 net/openvswitch/datapath.c:705
>>   genl_family_rcv_msg_doit+0x233/0x340 net/netlink/genetlink.c:1114
>>   genl_rcv_msg+0x614/0x7a0 net/netlink/genetlink.c:1209
>>   netlink_rcv_skb+0x226/0x4a0 net/netlink/af_netlink.c:2572
>>   genl_rcv+0x28/0x40 net/netlink/genetlink.c:1218
>>   netlink_unicast+0x7bd/0x940 net/netlink/af_netlink.c:1361
>>   netlink_sendmsg+0x813/0xb40 net/netlink/af_netlink.c:1916
>>   sock_sendmsg_nosec+0x14e/0x190 net/socket.c:800
>>
>> Reported-by: syzbot+4cc63fcfb3845e149969@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=4cc63fcfb3845e149969
>> Fixes: 3fdbd1ce11e5 ("openvswitch: add ipv6 'set' action")
>> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
>> ---
>>   net/openvswitch/actions.c | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> index dc5ff859f114..68b42e900c73 100644
>> --- a/net/openvswitch/actions.c
>> +++ b/net/openvswitch/actions.c
>> @@ -358,7 +358,15 @@ static void set_ip_addr(struct sk_buff *skb, struct iphdr *nh,
>>   static void update_ipv6_checksum(struct sk_buff *skb, u8 l4_proto,
>>   				 __be32 addr[4], const __be32 new_addr[4])
>>   {
>> -	int transport_len = skb->len - skb_transport_offset(skb);
>> +	int transport_len;
>> +
>> +	/* avoid reading the transport header offset if it isn't set,
>> +	 * as it triggers a warning
>> +	 */
> 
> nit: We prefer full sentences, i.e. start with a capital and end with a dot.
> 
> But also, I think, since the switch to NEXTHDR_FRAGMENT check, the comment
> lost it's intended purpose as the code is pretty much self-documenting now.
> It's clear that the fragment doesn't have the transport header.  The comment
> made sense if we needed to explain in which case we can get here without
> having the offset initialized.  I'd suggest we drop the comment.
> 
> You're also not adding such comments in the other patch for ipv4.
> 

Fair, I thought that in IPv4 case it was more obvious than here. Anyway, 
let me just drop it in a v3. Thanks a lot for the reviews :-)

> Otherwise, LGTM.
> 
>> +	if (l4_proto == NEXTHDR_FRAGMENT)
>> +		return;
>> +
>> +	transport_len = skb->len - skb_transport_offset(skb);
>>   
>>   	if (l4_proto == NEXTHDR_TCP) {
>>   		if (likely(transport_len >= sizeof(struct tcphdr)))
> 
> Best regards, Ilya Maximets.
>