netdev - Re: [PATCH net v4] net: openvswitch: Fix the dead loop of MPLS parse

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f83525bc-c47f-4e80-a323-e9c2e1d4656f@ovn.org>
Date: Fri, 23 May 2025 13:00:00 +0200
From: Ilya Maximets <i.maximets@....org>
To: Faicker Mo <faicker.mo@...layer.com>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Cc: i.maximets@....org, "dev@...nvswitch.org" <dev@...nvswitch.org>,
 "aconole@...hat.com" <aconole@...hat.com>,
 "echaudro@...hat.com" <echaudro@...hat.com>,
 "davem@...emloft.net" <davem@...emloft.net>,
 "edumazet@...gle.com" <edumazet@...gle.com>,
 "kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com"
 <pabeni@...hat.com>, "horms@...nel.org" <horms@...nel.org>,
 "martin.varghese@...ia.com" <martin.varghese@...ia.com>
Subject: Re: [PATCH net v4] net: openvswitch: Fix the dead loop of MPLS parse

On 5/23/25 5:41 AM, Faicker Mo wrote:
> The unexpected MPLS packet may not end with the bottom label stack.
> When there are many stacks, The label count value has wrapped around.
> A dead loop occurs, soft lockup/CPU stuck finally.
> 
> stack backtrace:
> UBSAN: array-index-out-of-bounds in /build/linux-0Pa0xK/linux-5.15.0/net/openvswitch/flow.c:662:26
> index -1 is out of range for type '__be32 [3]'
> CPU: 34 PID: 0 Comm: swapper/34 Kdump: loaded Tainted: G           OE   5.15.0-121-generic #131-Ubuntu
> Hardware name: Dell Inc. PowerEdge C6420/0JP9TF, BIOS 2.12.2 07/14/2021
> Call Trace:
>  <IRQ>
>  show_stack+0x52/0x5c
>  dump_stack_lvl+0x4a/0x63
>  dump_stack+0x10/0x16
>  ubsan_epilogue+0x9/0x36
>  __ubsan_handle_out_of_bounds.cold+0x44/0x49
>  key_extract_l3l4+0x82a/0x840 [openvswitch]
>  ? kfree_skbmem+0x52/0xa0
>  key_extract+0x9c/0x2b0 [openvswitch]
>  ovs_flow_key_extract+0x124/0x350 [openvswitch]
>  ovs_vport_receive+0x61/0xd0 [openvswitch]
>  ? kernel_init_free_pages.part.0+0x4a/0x70
>  ? get_page_from_freelist+0x353/0x540
>  netdev_port_receive+0xc4/0x180 [openvswitch]
>  ? netdev_port_receive+0x180/0x180 [openvswitch]
>  netdev_frame_hook+0x1f/0x40 [openvswitch]
>  __netif_receive_skb_core.constprop.0+0x23a/0xf00
>  __netif_receive_skb_list_core+0xfa/0x240
>  netif_receive_skb_list_internal+0x18e/0x2a0
>  napi_complete_done+0x7a/0x1c0
>  bnxt_poll+0x155/0x1c0 [bnxt_en]
>  __napi_poll+0x30/0x180
>  net_rx_action+0x126/0x280
>  ? bnxt_msix+0x67/0x80 [bnxt_en]
>  handle_softirqs+0xda/0x2d0
>  irq_exit_rcu+0x96/0xc0
>  common_interrupt+0x8e/0xa0
>  </IRQ>
> 
> Fixes: fbdcdd78da7c ("Change in Openvswitch to support MPLS label depth of 3 in ingress direction")
> Signed-off-by: Faicker Mo <faicker.mo@...layer.com>
> ---
> v2
> - Changed return value based on Eelco's feedback.
> - Added Fixes.
> v3
> - Revert "Changed return value based on Eelco's feedback".
> - Changed the label_count variable type based on Ilya's feedback.
> v4
> - Changed the subject based on Aaron's feedback.
> - changed the format.
> ---
>  net/openvswitch/flow.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> index 8a848ce72e29..b80bd3a90773 100644
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -788,7 +788,7 @@ static int key_extract_l3l4(struct sk_buff *skb, struct sw_flow_key *key)
>  			memset(&key->ipv4, 0, sizeof(key->ipv4));
>  		}
>  	} else if (eth_p_mpls(key->eth.type)) {
> -		u8 label_count = 1;
> +		size_t label_count = 1;
>  
>  		memset(&key->mpls, 0, sizeof(key->mpls));
>  		skb_set_inner_network_header(skb, skb->mac_len);

For the future, see the checkpatch warning about using the full path
to the source file in the commit message instead of the relative one.

But the change looks good to me.  I tested it and it solves the issue
with the CPU soft lockup.

Acked-by: Ilya Maximets <i.maximets@....org>