linux-kernel - Re: [PATCH] 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 for Android: free password hash cracker in your pocket

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

Message-ID: <7aad22dd-5b89-41e3-8543-1d875c5f7d40@ovn.org>
Date: Tue, 20 May 2025 12:38:02 +0200
From: Ilya Maximets <i.maximets@....org>
To: Eelco Chaudron <echaudro@...hat.com>, Faicker Mo <faicker.mo@...layer.com>
Cc: i.maximets@....org, netdev@...r.kernel.org, ovs-dev@...nvswitch.org,
 aconole@...hat.com, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, horms@...nel.org,
 linux-kernel@...r.kernel.org, dev@...nvswitch.org
Subject: Re: [PATCH] net: openvswitch: Fix the dead loop of MPLS parse

On 5/20/25 9:09 AM, Eelco Chaudron wrote:
> 
> 
> On 20 May 2025, at 6:13, 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>
>>
>> Signed-off-by: Faicker Mo <faicker.mo@...layer.com>
>> ---
>>  net/openvswitch/flow.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
>> index 8a848ce72e29..834b1b9110ac 100644
>> --- a/net/openvswitch/flow.c
>> +++ b/net/openvswitch/flow.c
>> @@ -805,6 +805,8 @@ static int key_extract_l3l4(struct sk_buff *skb, struct sw_flow_key *key)
>>                         if (label_count <= MPLS_LABEL_DEPTH)
>>                                 memcpy(&key->mpls.lse[label_count - 1], &lse,
>>                                        MPLS_HLEN);
>> +                       else if (unlikely(label_count == 255))
>> +                               return 0;
> 
> Thanks for finding and solving this issue, Faicker. I think that if we hit 255
> stack labels, it's safe to say the packet is invalid, and we should probably
> return -EINVAL. This also makes sense because the inner_network_header is not
> correct based on the terminated decode.

The idea of not failing the parsing is to allow forwarding the packet
based on parsed ethernet header.  So, we shouldn't fail here.
We're also keeping num_labels_mask at zero in this case, so it'll be
an MPLS packet with zero labels and it should not be parsed further,
but can still be forwarded.

I'm not sure how the skb_inner_network_header() should be set in this
case though.  It's used in the MPLS GSO code, but since we don't know
the good value, we may need to set it to zero.

But also, there is another overflow here that is actually causing an
infinite loop - the label_count * MPLS_HLEN easily overflows u8, so
the check_header() a few lines above doesn't work properly starting
at 32 labels and doesn't break the loop.  We need to switch the
label_count back to size_t or other sufficiently large type to avoid
this overflow and make the parsing end naturally when we hit the end
of the packet.

With the type change we may still consider returning early, though it's
not clear what the value we should aim for in this case.  And we need to
figure out what the skb_inner_network_header() should be in this case.

Thoughts?

Best regards, Ilya Maximets.

> 
>>
>>                         skb_set_inner_network_header(skb, skb->mac_len +
>>                                                      label_count * MPLS_HLEN);
>> --
>> 2.34.1
>