netfilter: conntrack: fix rmmod double-free race

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2189550
Upstream Status: commit e6d57e9

commit e6d57e9
Author: Florian Westphal <fw@strlen.de>
Date:   Tue Feb 14 17:23:59 2023 +0100

    netfilter: conntrack: fix rmmod double-free race

    nf_conntrack_hash_check_insert() callers free the ct entry directly, via
    nf_conntrack_free.

    This isn't safe anymore because
    nf_conntrack_hash_check_insert() might place the entry into the conntrack
    table and then delteted the entry again because it found that a conntrack
    extension has been removed at the same time.

    In this case, the just-added entry is removed again and an error is
    returned to the caller.

    Problem is that another cpu might have picked up this entry and
    incremented its reference count.

    This results in a use-after-free/double-free, once by the other cpu and
    once by the caller of nf_conntrack_hash_check_insert().

    Fix this by making nf_conntrack_hash_check_insert() not fail anymore
    after the insertion, just like before the 'Fixes' commit.

    This is safe because a racing nf_ct_iterate() has to wait for us
    to release the conntrack hash spinlocks.

    While at it, make the function return -EAGAIN in the rmmod (genid
    changed) case, this makes nfnetlink replay the command (suggested
    by Pablo Neira).

    Fixes: c56716c ("netfilter: extensions: introduce extension genid count")
    Signed-off-by: Florian Westphal <fw@strlen.de>
    Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Signed-off-by: Florian Westphal <fwestpha@redhat.com>

Author: fw-strlen

net/netfilter/nf_conntrack_bpf.c

@@ -381,7 +381,6 @@ struct nf_conn *bpf_ct_insert_entry(struct nf_conn___init *nfct_i)
 	struct nf_conn *nfct = (struct nf_conn *)nfct_i;
 	int err;
 
-	nfct->status |= IPS_CONFIRMED;
 	err = nf_conntrack_hash_check_insert(nfct);
 	if (err < 0) {
 		nf_conntrack_free(nfct);

net/netfilter/nf_conntrack_core.c

@@ -890,10 +890,8 @@ nf_conntrack_hash_check_insert(struct nf_conn *ct)
 
 	zone = nf_ct_zone(ct);
 
-	if (!nf_ct_ext_valid_pre(ct->ext)) {
-		NF_CT_STAT_INC_ATOMIC(net, insert_failed);
-		return -ETIMEDOUT;
-	}
+	if (!nf_ct_ext_valid_pre(ct->ext))
+		return -EAGAIN;
 
 	local_bh_disable();
 	do {
@@ -928,6 +926,19 @@ nf_conntrack_hash_check_insert(struct nf_conn *ct)
 			goto chaintoolong;
 	}
 
+	/* If genid has changed, we can't insert anymore because ct
+	 * extensions could have stale pointers and nf_ct_iterate_destroy
+	 * might have completed its table scan already.
+	 *
+	 * Increment of the ext genid right after this check is fine:
+	 * nf_ct_iterate_destroy blocks until locks are released.
+	 */
+	if (!nf_ct_ext_valid_post(ct->ext)) {
+		err = -EAGAIN;
+		goto out;
+	}
+
+	ct->status |= IPS_CONFIRMED;
 	smp_wmb();
 	/* The caller holds a reference to this object */
 	refcount_set(&ct->ct_general.use, 2);
@@ -936,12 +947,6 @@ nf_conntrack_hash_check_insert(struct nf_conn *ct)
 	NF_CT_STAT_INC(net, insert);
 	local_bh_enable();
 
-	if (!nf_ct_ext_valid_post(ct->ext)) {
-		nf_ct_kill(ct);
-		NF_CT_STAT_INC_ATOMIC(net, drop);
-		return -ETIMEDOUT;
-	}
-
 	return 0;
 chaintoolong:
 	NF_CT_STAT_INC(net, chaintoolong);

net/netfilter/nf_conntrack_netlink.c

@@ -2321,9 +2321,6 @@ ctnetlink_create_conntrack(struct net *net,
 	nfct_seqadj_ext_add(ct);
 	nfct_synproxy_ext_add(ct);
 
-	/* we must add conntrack extensions before confirmation. */
-	ct->status |= IPS_CONFIRMED;
-
 	if (cda[CTA_STATUS]) {
 		err = ctnetlink_change_status(ct, cda);
 		if (err < 0)