netdev - Re: [ovs-dev] [PATCH net] openvswitch: add nf_ct_is_confirmed check before assigning the helper

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: <f7t5ygpdyq9.fsf@redhat.com>
Date:   Wed, 12 Oct 2022 08:15:42 -0400
From:   Aaron Conole <aconole@...hat.com>
To:     Xin Long <lucien.xin@...il.com>
Cc:     network dev <netdev@...r.kernel.org>, dev@...nvswitch.org,
        Florian Westphal <fw@...len.de>,
        Ilya Maximets <i.maximets@....org>,
        Eric Dumazet <edumazet@...gle.com>, kuba@...nel.org,
        Paolo Abeni <pabeni@...hat.com>, davem@...emloft.net,
        Pablo Neira Ayuso <pablo@...filter.org>
Subject: Re: [ovs-dev] [PATCH net] openvswitch: add nf_ct_is_confirmed check
 before assigning the helper

Xin Long <lucien.xin@...il.com> writes:

> A WARN_ON call trace would be triggered when 'ct(commit, alg=helper)'
> applies on a confirmed connection:
>
>   WARNING: CPU: 0 PID: 1251 at net/netfilter/nf_conntrack_extend.c:98
>   RIP: 0010:nf_ct_ext_add+0x12d/0x150 [nf_conntrack]
>   Call Trace:
>    <TASK>
>    nf_ct_helper_ext_add+0x12/0x60 [nf_conntrack]
>    __nf_ct_try_assign_helper+0xc4/0x160 [nf_conntrack]
>    __ovs_ct_lookup+0x72e/0x780 [openvswitch]
>    ovs_ct_execute+0x1d8/0x920 [openvswitch]
>    do_execute_actions+0x4e6/0xb60 [openvswitch]
>    ovs_execute_actions+0x60/0x140 [openvswitch]
>    ovs_packet_cmd_execute+0x2ad/0x310 [openvswitch]
>    genl_family_rcv_msg_doit.isra.15+0x113/0x150
>    genl_rcv_msg+0xef/0x1f0
>
> which can be reproduced with these OVS flows:
>
>   table=0, in_port=veth1,tcp,tcp_dst=2121,ct_state=-trk
>   actions=ct(commit, table=1)
>   table=1, in_port=veth1,tcp,tcp_dst=2121,ct_state=+trk+new
>   actions=ct(commit, alg=ftp),normal
>
> The issue was introduced by commit 248d45f1e193 ("openvswitch: Allow
> attaching helper in later commit") where it somehow removed the check
> of nf_ct_is_confirmed before asigning the helper. This patch is to fix
> it by bringing it back.
>
> Fixes: 248d45f1e193 ("openvswitch: Allow attaching helper in later commit")
> Reported-by: Pablo Neira Ayuso <pablo@...filter.org>
> Signed-off-by: Xin Long <lucien.xin@...il.com>
> ---

Thanks for all the help, I was able to reproduce the warning.  Looking
at it, I'm not sure why the check for confirmed was removed.

I did some additional testing with common scenarios, some testing with
bare DP flows, and I think it is good to apply.

Acked-by: Aaron Conole <aconole@...hat.com>
Tested-by: Aaron Conole <aconole@...hat.com>