[PATCH nf] netfilter: fix possible bug_on with enable_hooks=1

[PATCH nf] netfilter: fix possible bug_on with enable_hooks=1

[Florian Westphal]

I received a bug report (no reproducer so far) where we trip over

712         rcu_read_lock();
713         ct_hook = rcu_dereference(nf_ct_hook);
714         BUG_ON(ct_hook == NULL);  // here

In nf_conntrack_destroy().

First turn this BUG_ON into a WARN.  I think it was triggered
via enable_hooks=1 flag.

When this flag is turned on, the conntrack hooks are registered
before nf_ct_hook pointer gets assigned.
This opens a short window where packets enter the conntrack machinery,
can have skb->_nfct set up and a subsequent kfree_skb might occur
before nf_ct_hook is set.

Call nf_conntrack_init_end() to set nf_ct_hook before we register the
pernet ops.

Fixes: ba3fbe663635 ("netfilter: nf_conntrack: provide modparam to always register conntrack hooks")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/core.c                    | 6 ++++--
 net/netfilter/nf_conntrack_standalone.c | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index f0783e42108b..fdd1dd0c747d 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -711,9 +711,11 @@ void nf_conntrack_destroy(struct nf_conntrack *nfct)
 
 	rcu_read_lock();
 	ct_hook = rcu_dereference(nf_ct_hook);
-	BUG_ON(ct_hook == NULL);
-	ct_hook->destroy(nfct);
+	if (ct_hook)
+		ct_hook->destroy(nfct);
 	rcu_read_unlock();
+
+	WARN_ON(!ct_hook);
 }
 EXPORT_SYMBOL(nf_conntrack_destroy);
 
diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
index 57f6724c99a7..169e16fc2bce 100644
--- a/net/netfilter/nf_conntrack_standalone.c
+++ b/net/netfilter/nf_conntrack_standalone.c
@@ -1218,11 +1218,12 @@ static int __init nf_conntrack_standalone_init(void)
 	nf_conntrack_htable_size_user = nf_conntrack_htable_size;
 #endif
 
+	nf_conntrack_init_end();
+
 	ret = register_pernet_subsys(&nf_conntrack_net_ops);
 	if (ret < 0)
 		goto out_pernet;
 
-	nf_conntrack_init_end();
 	return 0;
 
 out_pernet:
-- 
2.39.2

Re: [PATCH nf] netfilter: fix possible bug_on with enable_hooks=1

[Pablo Neira Ayuso]

On Thu, May 04, 2023 at 02:55:02PM +0200, Florian Westphal wrote:
> I received a bug report (no reproducer so far) where we trip over
> 
> 712         rcu_read_lock();
> 713         ct_hook = rcu_dereference(nf_ct_hook);
> 714         BUG_ON(ct_hook == NULL);  // here
> 
> In nf_conntrack_destroy().
> 
> First turn this BUG_ON into a WARN.  I think it was triggered
> via enable_hooks=1 flag.
> 
> When this flag is turned on, the conntrack hooks are registered
> before nf_ct_hook pointer gets assigned.
> This opens a short window where packets enter the conntrack machinery,
> can have skb->_nfct set up and a subsequent kfree_skb might occur
> before nf_ct_hook is set.
> 
> Call nf_conntrack_init_end() to set nf_ct_hook before we register the
> pernet ops.

Applied to nf, thanks