[PATCH nf 1/7] netfilter: nfnetlink_cthelper: use {READ,WRITE}_ONCE for accessing helper flags

[PATCH nf 1/7] netfilter: nfnetlink_cthelper: use {READ,WRITE}_ONCE for accessing helper flags

[Pablo Neira Ayuso]

Conntrack helper flags are accessed from packet and netlink dump path.
Concurrent update of userspace helper flags is not possible, because the
nfnl_mutex in held on updates. These flags are only used by userspace
helpers. Use {READ,WRITE}_ONCE() to access this flags from lockless
paths.

Fixes: 12f7a505331e ("netfilter: add user-space connection tracking helper infrastructure")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_conntrack_core.c  |  2 +-
 net/netfilter/nfnetlink_cthelper.c | 20 +++++++++++++-------
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 8ba5b22a1eef..2938df5a6a18 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -2216,7 +2216,7 @@ static int nf_confirm_cthelper(struct sk_buff *skb, struct nf_conn *ct,
 	if (!helper)
 		return NF_ACCEPT;
 
-	if (!(helper->flags & NF_CT_HELPER_F_USERSPACE))
+	if (!(READ_ONCE(helper->flags) & NF_CT_HELPER_F_USERSPACE))
 		return NF_ACCEPT;
 
 	switch (nf_ct_l3num(ct)) {
diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
index 0d16ad82d70c..61a2407b53bd 100644
--- a/net/netfilter/nfnetlink_cthelper.c
+++ b/net/netfilter/nfnetlink_cthelper.c
@@ -41,8 +41,9 @@ static int
 nfnl_userspace_cthelper(struct sk_buff *skb, unsigned int protoff,
 			struct nf_conn *ct, enum ip_conntrack_info ctinfo)
 {
-	const struct nf_conn_help *help;
 	struct nf_conntrack_helper *helper;
+	const struct nf_conn_help *help;
+	unsigned int helper_flags;
 
 	help = nfct_help(ct);
 	if (help == NULL)
@@ -53,8 +54,10 @@ nfnl_userspace_cthelper(struct sk_buff *skb, unsigned int protoff,
 	if (helper == NULL)
 		return NF_DROP;
 
+	helper_flags = READ_ONCE(helper->flags);
+
 	/* This is a user-space helper not yet configured, skip. */
-	if ((helper->flags &
+	if ((helper_flags &
 	    (NF_CT_HELPER_F_USERSPACE | NF_CT_HELPER_F_CONFIGURED)) ==
 	     NF_CT_HELPER_F_USERSPACE)
 		return NF_ACCEPT;
@@ -404,10 +407,10 @@ nfnl_cthelper_update(const struct nlattr * const tb[],
 
 		switch(status) {
 		case NFCT_HELPER_STATUS_ENABLED:
-			helper->flags |= NF_CT_HELPER_F_CONFIGURED;
+			WRITE_ONCE(helper->flags, helper->flags | NF_CT_HELPER_F_CONFIGURED);
 			break;
 		case NFCT_HELPER_STATUS_DISABLED:
-			helper->flags &= ~NF_CT_HELPER_F_CONFIGURED;
+			WRITE_ONCE(helper->flags, helper->flags & ~NF_CT_HELPER_F_CONFIGURED);
 			break;
 		}
 	}
@@ -529,8 +532,8 @@ static int
 nfnl_cthelper_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
 			int event, struct nf_conntrack_helper *helper)
 {
-	struct nlmsghdr *nlh;
 	unsigned int flags = portid ? NLM_F_MULTI : 0;
+	struct nlmsghdr *nlh;
 	int status;
 
 	event = nfnl_msg_type(NFNL_SUBSYS_CTHELPER, event);
@@ -554,7 +557,7 @@ nfnl_cthelper_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
 	if (nla_put_be32(skb, NFCTH_PRIV_DATA_LEN, htonl(helper->data_len)))
 		goto nla_put_failure;
 
-	if (helper->flags & NF_CT_HELPER_F_CONFIGURED)
+	if (READ_ONCE(helper->flags) & NF_CT_HELPER_F_CONFIGURED)
 		status = NFCT_HELPER_STATUS_ENABLED;
 	else
 		status = NFCT_HELPER_STATUS_DISABLED;
@@ -575,6 +578,7 @@ static int
 nfnl_cthelper_dump_table(struct sk_buff *skb, struct netlink_callback *cb)
 {
 	struct nf_conntrack_helper *cur, *last;
+	unsigned int helper_flags;
 
 	rcu_read_lock();
 	last = (struct nf_conntrack_helper *)cb->args[1];
@@ -583,8 +587,10 @@ nfnl_cthelper_dump_table(struct sk_buff *skb, struct netlink_callback *cb)
 		hlist_for_each_entry_rcu(cur,
 				&nf_ct_helper_hash[cb->args[0]], hnode) {
 
+			helper_flags = READ_ONCE(cur->flags);
+
 			/* skip non-userspace conntrack helpers. */
-			if (!(cur->flags & NF_CT_HELPER_F_USERSPACE))
+			if (!(helper_flags & NF_CT_HELPER_F_USERSPACE))
 				continue;
 
 			if (cb->args[1]) {
-- 
2.47.3

[PATCH nf 2/7] netfilter: conntrack: add dead flag to helpers

[Pablo Neira Ayuso]

Add a new NF_CT_HELPER_F_DEAD helper flag to notify the packet path that
this helper is going away. Thus, helpers are effectively disabled and no
new expectations are created while removing the expectations created by
this helper as well as unhelping the existing conntrack entries.

Add the check for NF_CT_HELPER_F_DEAD in the packet path to:

- Conntrack confirmation path which invokes the helper callback.
- Propagation of helper to conntrack via expectation.
- ctnetlink expectation creation path.
- OVS ct helper invocations.

nf_conntrack_helper_unregister() is only called under nfnl_mutex when
unregistering the helper, else by helper module which already owns this
helper from the module removal path. Therefore, concurrent update of
helper flags via nfnetlink_cthelper is not possible, but packet path and
netlink dump path can read these flags locklessly.

The kernel helpers never use flags, so concurrent update on these flags
can only happen in userspace helper via nfnetlink interface, where the
nfnl_mutex is held.

This patch also requires:

  c56716c69ce1 ("netfilter: extensions: introduce extension genid count")

which adds an extension genid for unconfirmed conntrack entries. This
allows to invalidate the conntrack helper extension in case a packet
holding a reference on the unconfirmed conntrack sits in nfqueue, or
elsewhere, and the helper goes away. This ensures that the access to the
helper area is disabled so the dandling pointer to the helper that went
away is not reachable anymore.

Fixes: 12f7a505331e ("netfilter: add user-space connection tracking helper infrastructure")
Reported-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/netfilter/nf_conntrack_helper.h | 8 ++++++++
 net/netfilter/nf_conntrack_core.c           | 2 +-
 net/netfilter/nf_conntrack_helper.c         | 5 ++++-
 net/netfilter/nf_conntrack_netlink.c        | 2 +-
 net/netfilter/nf_conntrack_ovs.c            | 2 +-
 net/netfilter/nf_conntrack_proto.c          | 2 +-
 6 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
index de2f956abf34..b6ff7dc65c97 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -25,6 +25,7 @@ struct module;
 enum nf_ct_helper_flags {
 	NF_CT_HELPER_F_USERSPACE	= (1 << 0),
 	NF_CT_HELPER_F_CONFIGURED	= (1 << 1),
+	NF_CT_HELPER_F_DEAD		= (1 << 2),
 };
 
 #define NF_CT_HELPER_NAME_LEN	16
@@ -63,6 +64,13 @@ struct nf_conntrack_helper {
 	char nat_mod_name[NF_CT_HELPER_NAME_LEN];
 };
 
+static inline bool nf_ct_helper_alive(const struct nf_conntrack_helper *helper)
+{
+	unsigned int helper_flags = READ_ONCE(helper->flags);
+
+	return likely(!(helper_flags & NF_CT_HELPER_F_DEAD));
+}
+
 /* Must be kept in sync with the classes defined by helpers */
 #define NF_CT_MAX_EXPECT_CLASSES	4
 
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 2938df5a6a18..1c04ef9dd17c 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1818,7 +1818,7 @@ init_conntrack(struct net *net, struct nf_conn *tmpl,
 			/* exp->master safe, refcnt bumped in nf_ct_find_expectation */
 			ct->master = exp->master;
 			assign_helper = rcu_dereference(exp->assign_helper);
-			if (assign_helper) {
+			if (assign_helper && nf_ct_helper_alive(assign_helper)) {
 				help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
 				if (help)
 					rcu_assign_pointer(help->helper, assign_helper);
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 17e971bd4c74..9a10b3449957 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -418,8 +418,11 @@ void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
 	nf_ct_helper_count--;
 	mutex_unlock(&nf_ct_helper_mutex);
 
+	WRITE_ONCE(me->flags, me->flags | NF_CT_HELPER_F_DEAD);
+
 	/* Make sure every nothing is still using the helper unless its a
-	 * connection in the hash.
+	 * connection in the hash, no more expectations are created after
+	 * this rcu grace period.
 	 */
 	synchronize_rcu();
 
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index befa7e83ee49..41926844d1be 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -3546,7 +3546,7 @@ ctnetlink_alloc_expect(const struct nlattr * const cda[], struct nf_conn *ct,
 		return ERR_PTR(-EOPNOTSUPP);
 
 	helper = rcu_dereference(help->helper);
-	if (!helper)
+	if (!helper || !nf_ct_helper_alive(helper))
 		return ERR_PTR(-EOPNOTSUPP);
 
 	if (cda[CTA_EXPECT_CLASS]) {
diff --git a/net/netfilter/nf_conntrack_ovs.c b/net/netfilter/nf_conntrack_ovs.c
index a6988eeb1579..fc4de20b5ccf 100644
--- a/net/netfilter/nf_conntrack_ovs.c
+++ b/net/netfilter/nf_conntrack_ovs.c
@@ -25,7 +25,7 @@ int nf_ct_helper(struct sk_buff *skb, struct nf_conn *ct,
 		return NF_ACCEPT;
 
 	helper = rcu_dereference(help->helper);
-	if (!helper)
+	if (!helper || !nf_ct_helper_alive(helper))
 		return NF_ACCEPT;
 
 	if (helper->tuple.src.l3num != NFPROTO_UNSPEC &&
diff --git a/net/netfilter/nf_conntrack_proto.c b/net/netfilter/nf_conntrack_proto.c
index 50ddd3d613e1..b2ac5bd491cb 100644
--- a/net/netfilter/nf_conntrack_proto.c
+++ b/net/netfilter/nf_conntrack_proto.c
@@ -174,7 +174,7 @@ unsigned int nf_confirm(void *priv,
 
 		/* rcu_read_lock()ed by nf_hook */
 		helper = rcu_dereference(help->helper);
-		if (helper) {
+		if (helper && nf_ct_helper_alive(helper)) {
 			ret = helper->help(skb,
 					   protoff,
 					   ct, ctinfo);
-- 
2.47.3

[PATCH nf 3/7] netfilter: nf_conntrack_helper: add null check in nfct_help_data() calls

[Pablo Neira Ayuso]

When helper is removed, nf_ct_iterate_destroy() unhelps the conntrack
entries. Then, the nf_ct_ext_find() might return NULL if the extension
is stale for unconfirmed conntracks if the genid validation fails.

Add the null check to nfct_help_data() and helpers that call this
function since packet path could be walking over helper while it is
being removed.

Fixes: c56716c69ce1 ("netfilter: extensions: introduce extension genid count").
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/netfilter/nf_conntrack_helper.h |  2 ++
 net/ipv4/netfilter/nf_nat_h323.c            | 12 ++++++++++++
 net/ipv4/netfilter/nf_nat_pptp.c            |  6 ++++++
 net/netfilter/nf_conntrack_ftp.c            |  6 ++++++
 net/netfilter/nf_conntrack_h323_main.c      | 18 ++++++++++++++++++
 net/netfilter/nf_conntrack_pptp.c           | 12 ++++++++++++
 net/netfilter/nf_conntrack_proto_gre.c      |  6 ++++++
 net/netfilter/nf_conntrack_sane.c           |  3 +++
 net/netfilter/nf_conntrack_sip.c            | 21 +++++++++++++++++++++
 net/netfilter/nf_nat_sip.c                  |  9 +++++++++
 10 files changed, 95 insertions(+)

diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
index b6ff7dc65c97..a712288fe162 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -140,6 +140,8 @@ static inline void *nfct_help_data(const struct nf_conn *ct)
 	struct nf_conn_help *help;
 
 	help = nf_ct_ext_find(ct, NF_CT_EXT_HELPER);
+	if (!help)
+		return NULL;
 
 	return (void *)help->data;
 }
diff --git a/net/ipv4/netfilter/nf_nat_h323.c b/net/ipv4/netfilter/nf_nat_h323.c
index faee20af4856..19dad54ada09 100644
--- a/net/ipv4/netfilter/nf_nat_h323.c
+++ b/net/ipv4/netfilter/nf_nat_h323.c
@@ -100,6 +100,9 @@ static int set_sig_addr(struct sk_buff *skb, struct nf_conn *ct,
 	__be16 port;
 	union nf_inet_addr addr;
 
+	if (!info)
+		return -1;
+
 	for (i = 0; i < count; i++) {
 		if (get_h225_addr(ct, *data, &taddr[i], &addr, &port)) {
 			if (addr.ip == ct->tuplehash[dir].tuple.src.u3.ip &&
@@ -184,6 +187,9 @@ static int nat_rtp_rtcp(struct sk_buff *skb, struct nf_conn *ct,
 	int i;
 	u_int16_t nated_port;
 
+	if (!info)
+		return -1;
+
 	/* Set expectations for NAT */
 	rtp_exp->saved_proto.udp.port = rtp_exp->tuple.dst.u.udp.port;
 	rtp_exp->expectfn = nf_nat_follow_master;
@@ -325,6 +331,9 @@ static int nat_h245(struct sk_buff *skb, struct nf_conn *ct,
 	int dir = CTINFO2DIR(ctinfo);
 	u_int16_t nated_port = ntohs(port);
 
+	if (!info)
+		return -1;
+
 	/* Set expectations for NAT */
 	exp->saved_proto.tcp.port = exp->tuple.dst.u.tcp.port;
 	exp->expectfn = nf_nat_follow_master;
@@ -404,6 +413,9 @@ static int nat_q931(struct sk_buff *skb, struct nf_conn *ct,
 	u_int16_t nated_port = ntohs(port);
 	union nf_inet_addr addr;
 
+	if (!info)
+		return -1;
+
 	/* Set expectations for NAT */
 	exp->saved_proto.tcp.port = exp->tuple.dst.u.tcp.port;
 	exp->expectfn = ip_nat_q931_expect;
diff --git a/net/ipv4/netfilter/nf_nat_pptp.c b/net/ipv4/netfilter/nf_nat_pptp.c
index fab357cc8559..efda9b64e4cf 100644
--- a/net/ipv4/netfilter/nf_nat_pptp.c
+++ b/net/ipv4/netfilter/nf_nat_pptp.c
@@ -58,6 +58,8 @@ static void pptp_nat_expected(struct nf_conn *ct,
 
 	nat_pptp_info = &nat->help.nat_pptp_info;
 	ct_pptp_info = nfct_help_data(master);
+	if (!ct_pptp_info)
+		return;
 
 	/* And here goes the grand finale of corrosion... */
 	if (exp->dir == IP_CT_DIR_ORIGINAL) {
@@ -137,6 +139,8 @@ pptp_outbound_pkt(struct sk_buff *skb,
 
 	nat_pptp_info = &nat->help.nat_pptp_info;
 	ct_pptp_info = nfct_help_data(ct);
+	if (!ct_pptp_info)
+		return NF_DROP;
 
 	new_callid = ct_pptp_info->pns_call_id;
 
@@ -209,6 +213,8 @@ pptp_exp_gre(struct nf_conntrack_expect *expect_orig,
 
 	nat_pptp_info = &nat->help.nat_pptp_info;
 	ct_pptp_info = nfct_help_data(ct);
+	if (!ct_pptp_info)
+		return;
 
 	/* save original PAC call ID in nat_info */
 	nat_pptp_info->pac_call_id = ct_pptp_info->pac_call_id;
diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
index de83bf9e6c61..a083697c3a54 100644
--- a/net/netfilter/nf_conntrack_ftp.c
+++ b/net/netfilter/nf_conntrack_ftp.c
@@ -381,6 +381,9 @@ static int help(struct sk_buff *skb,
 	int found = 0, ends_in_nl;
 	nf_nat_ftp_hook_fn *nf_nat_ftp;
 
+	if (!ct_ftp_info)
+		return NF_DROP;
+
 	/* Until there's been traffic both ways, don't look in packets. */
 	if (ctinfo != IP_CT_ESTABLISHED &&
 	    ctinfo != IP_CT_ESTABLISHED_REPLY) {
@@ -542,6 +545,9 @@ static int nf_ct_ftp_from_nlattr(struct nlattr *attr, struct nf_conn *ct)
 {
 	struct nf_ct_ftp_master *ftp = nfct_help_data(ct);
 
+	if (!ftp)
+		return -1;
+
 	/* This conntrack has been injected from user-space, always pick up
 	 * sequence tracking. Otherwise, the first FTP command after the
 	 * failover breaks.
diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
index b2fe6554b9cf..0ffd191a29ee 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c
@@ -76,6 +76,9 @@ static int get_tpkt_data(struct sk_buff *skb, unsigned int protoff,
 	int tpktlen;
 	int tpktoff;
 
+	if (!info)
+		return 0;
+
 	/* Get TCP header */
 	th = skb_header_pointer(skb, protoff, sizeof(_tcph), &_tcph);
 	if (th == NULL)
@@ -1215,6 +1218,9 @@ static int expect_q931(struct sk_buff *skb, struct nf_conn *ct,
 	union nf_inet_addr addr;
 	struct nf_conntrack_expect *exp;
 
+	if (!info)
+		return -1;
+
 	/* Look for the first related address */
 	for (i = 0; i < count; i++) {
 		if (get_h225_addr(ct, *data, &taddr[i], &addr, &port) &&
@@ -1328,6 +1334,9 @@ static int process_rrq(struct sk_buff *skb, struct nf_conn *ct,
 	const struct nfct_h323_nat_hooks *nathook;
 	int ret;
 
+	if (!info)
+		return -1;
+
 	pr_debug("nf_ct_ras: RRQ\n");
 
 	ret = expect_q931(skb, ct, ctinfo, protoff, data,
@@ -1366,6 +1375,9 @@ static int process_rcf(struct sk_buff *skb, struct nf_conn *ct,
 	int ret;
 	struct nf_conntrack_expect *exp;
 
+	if (!info)
+		return -1;
+
 	pr_debug("nf_ct_ras: RCF\n");
 
 	nathook = rcu_dereference(nfct_h323_nat_hook);
@@ -1416,6 +1428,9 @@ static int process_urq(struct sk_buff *skb, struct nf_conn *ct,
 	int dir = CTINFO2DIR(ctinfo);
 	int ret;
 
+	if (!info)
+		return -1;
+
 	pr_debug("nf_ct_ras: URQ\n");
 
 	nathook = rcu_dereference(nfct_h323_nat_hook);
@@ -1450,6 +1465,9 @@ static int process_arq(struct sk_buff *skb, struct nf_conn *ct,
 	__be16 port;
 	union nf_inet_addr addr;
 
+	if (!info)
+		return 0;
+
 	pr_debug("nf_ct_ras: ARQ\n");
 
 	nathook = rcu_dereference(nfct_h323_nat_hook);
diff --git a/net/netfilter/nf_conntrack_pptp.c b/net/netfilter/nf_conntrack_pptp.c
index 4c679638df06..57f85f6c2625 100644
--- a/net/netfilter/nf_conntrack_pptp.c
+++ b/net/netfilter/nf_conntrack_pptp.c
@@ -164,6 +164,9 @@ static void pptp_destroy_siblings(struct nf_conn *ct)
 	const struct nf_ct_pptp_master *ct_pptp_info = nfct_help_data(ct);
 	struct nf_conntrack_tuple t;
 
+	if (!ct_pptp_info)
+		return;
+
 	nf_ct_gre_keymap_destroy(ct);
 
 	/* try original (pns->pac) tuple */
@@ -261,6 +264,9 @@ pptp_inbound_pkt(struct sk_buff *skb, unsigned int protoff,
 	u_int16_t msg;
 	__be16 cid = 0, pcid = 0;
 
+	if (!info)
+		return NF_DROP;
+
 	msg = ntohs(ctlh->messageType);
 	pr_debug("inbound control message %s\n", pptp_msg_name(msg));
 
@@ -388,6 +394,9 @@ pptp_outbound_pkt(struct sk_buff *skb, unsigned int protoff,
 	u_int16_t msg;
 	__be16 cid = 0, pcid = 0;
 
+	if (!info)
+		return NF_DROP;
+
 	msg = ntohs(ctlh->messageType);
 	pr_debug("outbound control message %s\n", pptp_msg_name(msg));
 
@@ -506,6 +515,9 @@ conntrack_pptp_help(struct sk_buff *skb, unsigned int protoff,
 	int ret;
 	u_int16_t msg;
 
+	if (!info)
+		return NF_DROP;
+
 #if IS_ENABLED(CONFIG_NF_NAT)
 	if (!nf_ct_is_confirmed(ct) && (ct->status & IPS_NAT_MASK)) {
 		struct nf_conn_nat *nat = nf_ct_ext_find(ct, NF_CT_EXT_NAT);
diff --git a/net/netfilter/nf_conntrack_proto_gre.c b/net/netfilter/nf_conntrack_proto_gre.c
index 94c19bc4edc5..fa99380aaf64 100644
--- a/net/netfilter/nf_conntrack_proto_gre.c
+++ b/net/netfilter/nf_conntrack_proto_gre.c
@@ -96,6 +96,9 @@ int nf_ct_gre_keymap_add(struct nf_conn *ct, enum ip_conntrack_dir dir,
 	struct nf_ct_pptp_master *ct_pptp_info = nfct_help_data(ct);
 	struct nf_ct_gre_keymap **kmp, *km;
 
+	if (!ct_pptp_info)
+		return -ENOENT;
+
 	kmp = &ct_pptp_info->keymap[dir];
 	if (*kmp) {
 		/* check whether it's a retransmission */
@@ -131,6 +134,9 @@ void nf_ct_gre_keymap_destroy(struct nf_conn *ct)
 	struct nf_ct_pptp_master *ct_pptp_info = nfct_help_data(ct);
 	enum ip_conntrack_dir dir;
 
+	if (!ct_pptp_info)
+		return;
+
 	pr_debug("entering for ct %p\n", ct);
 
 	spin_lock_bh(&keymap_lock);
diff --git a/net/netfilter/nf_conntrack_sane.c b/net/netfilter/nf_conntrack_sane.c
index 13dc421fc4f5..9cf4a22eca4a 100644
--- a/net/netfilter/nf_conntrack_sane.c
+++ b/net/netfilter/nf_conntrack_sane.c
@@ -74,6 +74,9 @@ static int help(struct sk_buff *skb,
 		struct sane_reply_net_start repl;
 	} buf;
 
+	if (!ct_sane_info)
+		return NF_DROP;
+
 	/* Until there's been traffic both ways, don't look in packets. */
 	if (ctinfo != IP_CT_ESTABLISHED &&
 	    ctinfo != IP_CT_ESTABLISHED_REPLY)
diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index e69941f1a101..2f90f2c54708 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -1227,6 +1227,9 @@ static int process_invite_response(struct sk_buff *skb, unsigned int protoff,
 	struct nf_conn *ct = nf_ct_get(skb, &ctinfo);
 	struct nf_ct_sip_master *ct_sip_info = nfct_help_data(ct);
 
+	if (!ct_sip_info)
+		return NF_DROP;
+
 	if ((code >= 100 && code <= 199) ||
 	    (code >= 200 && code <= 299))
 		return process_sdp(skb, protoff, dataoff, dptr, datalen, cseq);
@@ -1244,6 +1247,9 @@ static int process_update_response(struct sk_buff *skb, unsigned int protoff,
 	struct nf_conn *ct = nf_ct_get(skb, &ctinfo);
 	struct nf_ct_sip_master *ct_sip_info = nfct_help_data(ct);
 
+	if (!ct_sip_info)
+		return NF_DROP;
+
 	if ((code >= 100 && code <= 199) ||
 	    (code >= 200 && code <= 299))
 		return process_sdp(skb, protoff, dataoff, dptr, datalen, cseq);
@@ -1261,6 +1267,9 @@ static int process_prack_response(struct sk_buff *skb, unsigned int protoff,
 	struct nf_conn *ct = nf_ct_get(skb, &ctinfo);
 	struct nf_ct_sip_master *ct_sip_info = nfct_help_data(ct);
 
+	if (!ct_sip_info)
+		return NF_DROP;
+
 	if ((code >= 100 && code <= 199) ||
 	    (code >= 200 && code <= 299))
 		return process_sdp(skb, protoff, dataoff, dptr, datalen, cseq);
@@ -1279,6 +1288,9 @@ static int process_invite_request(struct sk_buff *skb, unsigned int protoff,
 	struct nf_ct_sip_master *ct_sip_info = nfct_help_data(ct);
 	unsigned int ret;
 
+	if (!ct_sip_info)
+		return NF_DROP;
+
 	flush_expectations(ct, true);
 	ret = process_sdp(skb, protoff, dataoff, dptr, datalen, cseq);
 	if (ret == NF_ACCEPT)
@@ -1321,6 +1333,9 @@ static int process_register_request(struct sk_buff *skb, unsigned int protoff,
 	unsigned int expires = 0;
 	int ret;
 
+	if (!ct_sip_info)
+		return NF_DROP;
+
 	/* Expected connections can not register again. */
 	if (ct->status & IPS_EXPECTED)
 		return NF_ACCEPT;
@@ -1421,6 +1436,9 @@ static int process_register_response(struct sk_buff *skb, unsigned int protoff,
 	unsigned int expires = 0;
 	int in_contact = 0, ret;
 
+	if (!ct_sip_info)
+		return NF_DROP;
+
 	/* According to RFC 3261, "UAs MUST NOT send a new registration until
 	 * they have received a final response from the registrar for the
 	 * previous one or the previous REGISTER request has timed out".
@@ -1550,6 +1568,9 @@ static int process_sip_request(struct sk_buff *skb, unsigned int protoff,
 	union nf_inet_addr addr;
 	__be16 port;
 
+	if (!ct_sip_info)
+		return NF_DROP;
+
 	/* Many Cisco IP phones use a high source port for SIP requests, but
 	 * listen for the response on port 5060.  If we are the local
 	 * router for one of these phones, save the port number from the
diff --git a/net/netfilter/nf_nat_sip.c b/net/netfilter/nf_nat_sip.c
index 9fbfc6bff0c2..b1931202825b 100644
--- a/net/netfilter/nf_nat_sip.c
+++ b/net/netfilter/nf_nat_sip.c
@@ -106,6 +106,9 @@ static int map_addr(struct sk_buff *skb, unsigned int protoff,
 	union nf_inet_addr newaddr;
 	__be16 newport;
 
+	if (!ct_sip_info)
+		return 0;
+
 	if (nf_inet_addr_cmp(&ct->tuplehash[dir].tuple.src.u3, addr) &&
 	    ct->tuplehash[dir].tuple.src.u.udp.port == port) {
 		newaddr = ct->tuplehash[!dir].tuple.dst.u3;
@@ -158,6 +161,9 @@ static unsigned int nf_nat_sip(struct sk_buff *skb, unsigned int protoff,
 	__be16 port;
 	int request, in_header;
 
+	if (!ct_sip_info)
+		return NF_DROP;
+
 	/* Basic rules: requests and responses. */
 	if (strncasecmp(*dptr, "SIP/2.0", strlen("SIP/2.0")) != 0) {
 		if (ct_sip_parse_request(ct, *dptr, *datalen,
@@ -390,6 +396,9 @@ static unsigned int nf_nat_sip_expect(struct sk_buff *skb, unsigned int protoff,
 	char buffer[INET6_ADDRSTRLEN + sizeof("[]:nnnnn")];
 	unsigned int buflen;
 
+	if (!ct_sip_info)
+		return NF_DROP;
+
 	/* Connection will come from reply */
 	if (nf_inet_addr_cmp(&ct->tuplehash[dir].tuple.src.u3,
 			     &ct->tuplehash[!dir].tuple.dst.u3))
-- 
2.47.3

[PATCH nf 4/7] netfilter: conntrack: add null check in nfct_help() calls

[Pablo Neira Ayuso]

When helper is removed, nf_ct_iterate_destroy() unhelps the conntrack
entries. Then, the nf_ct_ext_find() might return NULL if the extension
is stale for unconfirmed conntracks if the genid validation fails.

Add the null check to nfct_help() and helpers that call this function
since packet path could be walking over helper while it is being
removed.

Fixes: c56716c69ce1 ("netfilter: extensions: introduce extension genid count").
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_conntrack_broadcast.c |  3 +++
 net/netfilter/nf_conntrack_expect.c    | 31 +++++++++++++++++---------
 net/netfilter/nf_conntrack_sip.c       | 20 +++++++++++++++--
 net/netfilter/nf_nat_sip.c             |  3 +++
 net/netfilter/nfnetlink_cthelper.c     |  6 +++++
 net/netfilter/xt_CT.c                  |  1 +
 6 files changed, 51 insertions(+), 13 deletions(-)

diff --git a/net/netfilter/nf_conntrack_broadcast.c b/net/netfilter/nf_conntrack_broadcast.c
index 75e53fde6b29..400119b6320e 100644
--- a/net/netfilter/nf_conntrack_broadcast.c
+++ b/net/netfilter/nf_conntrack_broadcast.c
@@ -29,6 +29,9 @@ int nf_conntrack_broadcast_help(struct sk_buff *skb,
 	struct nf_conn_help *help = nfct_help(ct);
 	__be32 mask = 0;
 
+	if (!help)
+		goto out;
+
 	/* we're only interested in locally generated packets */
 	if (skb->sk == NULL || !net_eq(nf_ct_net(ct), sock_net(skb->sk)))
 		goto out;
diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
index 8e943efbdf0a..09d0eff47658 100644
--- a/net/netfilter/nf_conntrack_expect.c
+++ b/net/netfilter/nf_conntrack_expect.c
@@ -60,8 +60,14 @@ void nf_ct_unlink_expect_report(struct nf_conntrack_expect *exp,
 	cnet = nf_ct_pernet(net);
 	cnet->expect_count--;
 
-	hlist_del_rcu(&exp->lnode);
-	master_help->expecting[exp->class]--;
+	/* This master conntrack has been unhelped, it is not reachable
+	 * anymore, skip expectation removal from local master conntrack
+	 * list and the expecting counter update.
+	 */
+	if (master_help) {
+		hlist_del_rcu(&exp->lnode);
+		master_help->expecting[exp->class]--;
+	}
 
 	nf_ct_expect_event_report(IPEXP_DESTROY, exp, portid, report);
 	nf_ct_expect_put(exp);
@@ -405,10 +411,10 @@ void nf_ct_expect_put(struct nf_conntrack_expect *exp)
 }
 EXPORT_SYMBOL_GPL(nf_ct_expect_put);
 
-static void nf_ct_expect_insert(struct nf_conntrack_expect *exp)
+static void nf_ct_expect_insert(struct nf_conntrack_expect *exp,
+				struct nf_conn_help *master_help)
 {
 	struct nf_conntrack_net *cnet;
-	struct nf_conn_help *master_help = nfct_help(exp->master);
 	struct nf_conntrack_helper *helper;
 	struct net *net = nf_ct_exp_net(exp);
 	unsigned int h = nf_ct_expect_dst_hash(net, &exp->tuple);
@@ -452,13 +458,13 @@ static void evict_oldest_expect(struct nf_conn *master,
 }
 
 static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect,
+				       struct nf_conn_help *master_help,
 				       unsigned int flags)
 {
 	const struct nf_conntrack_expect_policy *p;
 	struct nf_conntrack_expect *i;
 	struct nf_conntrack_net *cnet;
 	struct nf_conn *master = expect->master;
-	struct nf_conn_help *master_help = nfct_help(master);
 	struct nf_conntrack_helper *helper;
 	struct net *net = nf_ct_exp_net(expect);
 	struct hlist_node *next;
@@ -467,10 +473,6 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect,
 
 	lockdep_nfct_expect_lock_held();
 
-	if (!master_help) {
-		ret = -ESHUTDOWN;
-		goto out;
-	}
 	h = nf_ct_expect_dst_hash(net, &expect->tuple);
 	hlist_for_each_entry_safe(i, next, &nf_ct_expect_hash[h], hnode) {
 		if (master_matches(i, expect, flags) &&
@@ -514,14 +516,21 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect,
 int nf_ct_expect_related_report(struct nf_conntrack_expect *expect,
 				u32 portid, int report, unsigned int flags)
 {
+	struct nf_conn_help *master_help;
 	int ret;
 
 	spin_lock_bh(&nf_conntrack_expect_lock);
-	ret = __nf_ct_expect_check(expect, flags);
+	master_help = nfct_help(expect->master);
+	if (!master_help) {
+		ret = -ESHUTDOWN;
+		goto out;
+	}
+
+	ret = __nf_ct_expect_check(expect, master_help, flags);
 	if (ret < 0)
 		goto out;
 
-	nf_ct_expect_insert(expect);
+	nf_ct_expect_insert(expect, master_help);
 
 	nf_ct_expect_event_report(IPEXP_NEW, expect, portid, report);
 	spin_unlock_bh(&nf_conntrack_expect_lock);
diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index 2f90f2c54708..adc5562dcf11 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -887,6 +887,9 @@ static int refresh_signalling_expectation(struct nf_conn *ct,
 	struct hlist_node *next;
 	int found = 0;
 
+	if (!help)
+		return 0;
+
 	spin_lock_bh(&nf_conntrack_expect_lock);
 	hlist_for_each_entry_safe(exp, next, &help->expectations, lnode) {
 		if (exp->class != SIP_EXPECT_SIGNALLING ||
@@ -910,6 +913,9 @@ static void flush_expectations(struct nf_conn *ct, bool media)
 	struct nf_conntrack_expect *exp;
 	struct hlist_node *next;
 
+	if (!help)
+		return;
+
 	spin_lock_bh(&nf_conntrack_expect_lock);
 	hlist_for_each_entry_safe(exp, next, &help->expectations, lnode) {
 		if ((exp->class != SIP_EXPECT_SIGNALLING) ^ media)
@@ -940,6 +946,11 @@ static int set_expected_rtp_rtcp(struct sk_buff *skb, unsigned int protoff,
 	u_int16_t base_port;
 	__be16 rtp_port, rtcp_port;
 	const struct nf_nat_sip_hooks *hooks;
+	struct nf_conn_help *help;
+
+	help = nfct_help(ct);
+	if (!help)
+		return NF_DROP;
 
 	saddr = NULL;
 	if (sip_direct_media) {
@@ -1002,7 +1013,7 @@ static int set_expected_rtp_rtcp(struct sk_buff *skb, unsigned int protoff,
 		exp = __nf_ct_expect_find(net, nf_ct_zone(ct), &tuple);
 
 		if (!exp || exp->master == ct ||
-		    exp->helper != nfct_help(ct)->helper ||
+		    exp->helper != help->helper ||
 		    exp->class != class)
 			break;
 #if IS_ENABLED(CONFIG_NF_NAT)
@@ -1328,6 +1339,7 @@ static int process_register_request(struct sk_buff *skb, unsigned int protoff,
 	union nf_inet_addr *saddr, daddr;
 	const struct nf_nat_sip_hooks *hooks;
 	struct nf_conntrack_helper *helper;
+	struct nf_conn_help *help;
 	__be16 port;
 	u8 proto;
 	unsigned int expires = 0;
@@ -1381,7 +1393,11 @@ static int process_register_request(struct sk_buff *skb, unsigned int protoff,
 		goto store_cseq;
 	}
 
-	helper = rcu_dereference(nfct_help(ct)->helper);
+	help = nfct_help(ct);
+	if (!help)
+		return NF_DROP;
+
+	helper = rcu_dereference(help->helper);
 	if (!helper)
 		return NF_DROP;
 
diff --git a/net/netfilter/nf_nat_sip.c b/net/netfilter/nf_nat_sip.c
index b1931202825b..7f29a6785327 100644
--- a/net/netfilter/nf_nat_sip.c
+++ b/net/netfilter/nf_nat_sip.c
@@ -332,6 +332,9 @@ static void nf_nat_sip_expected(struct nf_conn *ct,
 	int range_set_for_snat = 0;
 	struct nf_nat_range2 range;
 
+	if (!help)
+		return;
+
 	/* This must be a fresh one. */
 	BUG_ON(ct->status & IPS_NAT_DONE_MASK);
 
diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
index 61a2407b53bd..203bf5cf1f29 100644
--- a/net/netfilter/nfnetlink_cthelper.c
+++ b/net/netfilter/nfnetlink_cthelper.c
@@ -101,6 +101,9 @@ nfnl_cthelper_from_nlattr(struct nlattr *attr, struct nf_conn *ct)
 	struct nf_conn_help *help = nfct_help(ct);
 	const struct nf_conntrack_helper *helper;
 
+	if (!help)
+		return -EINVAL;
+
 	if (attr == NULL)
 		return -EINVAL;
 
@@ -118,6 +121,9 @@ nfnl_cthelper_to_nlattr(struct sk_buff *skb, const struct nf_conn *ct)
 	const struct nf_conn_help *help = nfct_help(ct);
 	const struct nf_conntrack_helper *helper;
 
+	if (!help)
+		return 0;
+
 	helper = rcu_dereference(help->helper);
 	if (helper && helper->data_len &&
 	    nla_put(skb, CTA_HELP_INFO, helper->data_len, &help->data))
diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
index d2aeacf94230..827e45f5d5ee 100644
--- a/net/netfilter/xt_CT.c
+++ b/net/netfilter/xt_CT.c
@@ -223,6 +223,7 @@ static int xt_ct_tg_check(const struct xt_tgchk_param *par,
 
 err4:
 	help = nfct_help(ct);
+	WARN_ON_ONCE(help);
 	xt_ct_put_helper(help);
 err3:
 	nf_ct_tmpl_free(ct);
-- 
2.47.3

[PATCH nf 5/7] netfilter: conntrack: add nf_ct_iterate_destroy_net()

[Pablo Neira Ayuso]

This calls nf_queue_nf_hook_drop(), bump generation id to invalidate
ct extension and finally nf_ct_iterate_cleanup().

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/netfilter/nf_conntrack.h |  4 ++
 net/netfilter/nf_conntrack_core.c    | 71 ++++++++++++++++++++--------
 2 files changed, 56 insertions(+), 19 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index bc42dd0e10e6..4803e43677b9 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -245,6 +245,10 @@ void nf_ct_iterate_cleanup_net(int (*iter)(struct nf_conn *i, void *data),
 /* also set unconfirmed conntracks as dying. Only use in module exit path. */
 void nf_ct_iterate_destroy(int (*iter)(struct nf_conn *i, void *data),
 			   void *data);
+/* same as previous function, but for one specific netns. */
+void nf_ct_iterate_destroy_net(struct net *net,
+			       int (*iter)(struct nf_conn *i, void *data),
+			       void *data);
 
 struct nf_conntrack_zone;
 
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 1c04ef9dd17c..59656b7de654 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -2388,6 +2388,54 @@ void nf_ct_iterate_cleanup_net(int (*iter)(struct nf_conn *i, void *data),
 }
 EXPORT_SYMBOL_GPL(nf_ct_iterate_cleanup_net);
 
+static void
+nf_ct_iterate_destroy_finish(int (*iter)(struct nf_conn *i, void *data),
+			     struct nf_ct_iter_data *iter_data)
+{
+	/* a skb w. unconfirmed conntrack could have been reinjected just
+	 * before we called nf_queue_nf_hook_drop().
+	 *
+	 * This makes sure its inserted into conntrack table.
+	 */
+	synchronize_net();
+
+	nf_ct_ext_bump_genid();
+	nf_ct_iterate_cleanup(iter, iter_data);
+
+	/* Another cpu might be in a rcu read section with
+	 * rcu protected pointer cleared in iter callback
+	 * or hidden via nf_ct_ext_bump_genid() above.
+	 *
+	 * Wait until those are done.
+	 */
+	synchronize_rcu();
+}
+
+/**
+ * nf_ct_iterate_destroy_net - destroy unconfirmed conntracks and iterate table in netns
+ * @iter: callback to invoke for each conntrack
+ * @data: data to pass to @iter
+ *
+ * Like nf_ct_iterate_cleanup, but first marks conntracks on the
+ * unconfirmed list as dying (so they will not be inserted into
+ * main table).
+ *
+ * Can only be called for netns.
+ */
+void
+nf_ct_iterate_destroy_net(struct net *net,
+			  int (*iter)(struct nf_conn *i, void *data), void *data)
+{
+	struct nf_ct_iter_data iter_data = {
+		.net	= net,
+		.data	= data,
+	};
+
+	nf_queue_nf_hook_drop(net);
+
+	nf_ct_iterate_destroy_finish(iter, &iter_data);
+}
+
 /**
  * nf_ct_iterate_destroy - destroy unconfirmed conntracks and iterate table
  * @iter: callback to invoke for each conntrack
@@ -2402,7 +2450,9 @@ EXPORT_SYMBOL_GPL(nf_ct_iterate_cleanup_net);
 void
 nf_ct_iterate_destroy(int (*iter)(struct nf_conn *i, void *data), void *data)
 {
-	struct nf_ct_iter_data iter_data = {};
+	struct nf_ct_iter_data iter_data = {
+		.data	= data,
+	};
 	struct net *net;
 
 	down_read(&net_rwsem);
@@ -2422,24 +2472,7 @@ nf_ct_iterate_destroy(int (*iter)(struct nf_conn *i, void *data), void *data)
 	 */
 	net_ns_barrier();
 
-	/* a skb w. unconfirmed conntrack could have been reinjected just
-	 * before we called nf_queue_nf_hook_drop().
-	 *
-	 * This makes sure its inserted into conntrack table.
-	 */
-	synchronize_net();
-
-	nf_ct_ext_bump_genid();
-	iter_data.data = data;
-	nf_ct_iterate_cleanup(iter, &iter_data);
-
-	/* Another cpu might be in a rcu read section with
-	 * rcu protected pointer cleared in iter callback
-	 * or hidden via nf_ct_ext_bump_genid() above.
-	 *
-	 * Wait until those are done.
-	 */
-	synchronize_rcu();
+	nf_ct_iterate_destroy_finish(iter, &iter_data);
 }
 EXPORT_SYMBOL_GPL(nf_ct_iterate_destroy);
 
-- 
2.47.3

[PATCH nf 6/7] netfilter: nf_conntrack_timeout: use nf_ct_iterate_destroy() to cleanup timeout going away

[Pablo Neira Ayuso]

nf_ct_iterate_cleanup_net() is not sufficient, this is just a subset of
nf_ct_iterate_destroy_net(), which deals with flushing skb with
unconfirmed conntrack entries sitting in nfqueue, and it also
invalidates the timeout extension.

Fixes: 34158151d2aa ("netfilter: cttimeout: use nf_ct_iterate_cleanup_net to unlink timeout objs")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_conntrack_timeout.c | 7 +------
 net/netfilter/nft_ct.c               | 1 -
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/net/netfilter/nf_conntrack_timeout.c b/net/netfilter/nf_conntrack_timeout.c
index 0cc584d3dbb1..006e8bcf129b 100644
--- a/net/netfilter/nf_conntrack_timeout.c
+++ b/net/netfilter/nf_conntrack_timeout.c
@@ -44,12 +44,7 @@ static int untimeout(struct nf_conn *ct, void *timeout)
 
 void nf_ct_untimeout(struct net *net, struct nf_ct_timeout *timeout)
 {
-	struct nf_ct_iter_data iter_data = {
-		.net	= net,
-		.data	= timeout,
-	};
-
-	nf_ct_iterate_cleanup_net(untimeout, &iter_data);
+	nf_ct_iterate_destroy_net(net, untimeout, timeout);
 }
 EXPORT_SYMBOL_GPL(nf_ct_untimeout);
 
diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
index fa2cc556331c..3fdee729149b 100644
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@ -971,7 +971,6 @@ static void nft_ct_timeout_obj_destroy(const struct nft_ctx *ctx,
 	struct nft_ct_timeout_obj *priv = nft_obj_data(obj);
 	struct nf_ct_timeout *timeout = priv->timeout;
 
-	nf_queue_nf_hook_drop(ctx->net);
 	nf_ct_untimeout(ctx->net, timeout);
 	nf_ct_netns_put(ctx->net, ctx->family);
 	kfree_rcu(priv->timeout, rcu);
-- 
2.47.3

[PATCH nf 7/7] netfilter: xt_CT: fix race with rule removal and nfnetlink_queue

[Pablo Neira Ayuso]

         CPU0                           CPU1
                               xt_CT attaches tmpl_ct
  remove rule with xt_CT
  ...                          ...
  nf_queue_nf_hook_drop()
                               nfqueue's enqueue_skb

While skb is flying to be enqueued by nfqueue, the rule and the
helper/timeout can be removed, leaving a dangling pointer in the ct
extensions.

Set on dying bit in the template and handle this from nfnetlink_queue.

Fixes: 84f3bb9ae9db ("netfilter: xtables: add CT target")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nfnetlink_queue.c | 20 ++++++++++++++++++++
 net/netfilter/xt_CT.c           |  2 ++
 2 files changed, 22 insertions(+)

diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 984a0eb9e149..c682adc34dcf 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -412,6 +412,20 @@ static void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
 	nf_queue_entry_free(entry);
 }
 
+static bool nf_ct_drop_template(const struct nf_queue_entry *entry)
+{
+#if IS_ENABLED(CONFIG_NF_CONNTRACK)
+	struct nf_conn *ct = (void *)skb_nfct(entry->skb);
+
+	if (!ct || !nf_ct_is_template(ct))
+		return false;
+
+	if (nf_ct_is_dying(ct))
+		return true;
+#endif
+	return false;
+}
+
 /* return true if the entry has an unconfirmed conntrack attached that isn't owned by us
  * exclusively.
  */
@@ -468,6 +482,9 @@ static void nfqnl_reinject(struct nf_queue_entry *entry, unsigned int verdict)
 		}
 	}
 
+	if (nf_ct_drop_template(entry))
+		verdict = NF_DROP;
+
 	if (verdict != NF_DROP && entry->nf_ct_is_unconfirmed) {
 		/* If first queued segment was already reinjected then
 		 * there is a good chance the ct entry is now confirmed.
@@ -1077,6 +1094,9 @@ nfqnl_enqueue_packet(struct nf_queue_entry *entry, unsigned int queuenum)
 		break;
 	}
 
+	if (nf_ct_drop_template(entry))
+		return -EINVAL;
+
 	/* Check if someone already holds another reference to
 	 * unconfirmed ct.  If so, we cannot queue the skb:
 	 * concurrent modifications of nf_conn->ext are not
diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
index 827e45f5d5ee..9c4e8e3f6d25 100644
--- a/net/netfilter/xt_CT.c
+++ b/net/netfilter/xt_CT.c
@@ -285,6 +285,8 @@ static void xt_ct_tg_destroy(const struct xt_tgdtor_param *par,
 	struct nf_conn_help *help;
 
 	if (ct) {
+		set_bit(IPS_DYING_BIT, &ct->status);
+
 		if (info->helper[0] || info->timeout[0])
 			nf_queue_nf_hook_drop(par->net);
 
-- 
2.47.3

[syzbot ci] Re: netfilter: nfnetlink_cthelper: use {READ,WRITE}_ONCE for accessing helper flags

[syzbot ci]

syzbot ci has tested the following series

[v1] netfilter: nfnetlink_cthelper: use {READ,WRITE}_ONCE for accessing helper flags
https://lore.kernel.org/all/20260519213826.1181661-1-pablo@netfilter.org
* [PATCH nf 1/7] netfilter: nfnetlink_cthelper: use {READ,WRITE}_ONCE for accessing helper flags
* [PATCH nf 2/7] netfilter: conntrack: add dead flag to helpers
* [PATCH nf 3/7] netfilter: nf_conntrack_helper: add null check in nfct_help_data() calls
* [PATCH nf 4/7] netfilter: conntrack: add null check in nfct_help() calls
* [PATCH nf 5/7] netfilter: conntrack: add nf_ct_iterate_destroy_net()
* [PATCH nf 6/7] netfilter: nf_conntrack_timeout: use nf_ct_iterate_destroy() to cleanup timeout going away
* [PATCH nf 7/7] netfilter: xt_CT: fix race with rule removal and nfnetlink_queue

and found the following issue:
WARNING in xt_ct_tg_check

Full report is available here:
https://ci.syzbot.org/series/c356956d-b1f6-4d7e-be26-6cf68d49814e

***

WARNING in xt_ct_tg_check

tree:      nf
URL:       https://kernel.googlesource.com/pub/scm/linux/kernel/git/netfilter/nf.git
base:      2beba18b0160446463bf1dbd749324846db98493
arch:      amd64
compiler:  Debian clang version 21.1.8 (++20251221033036+2078da43e25a-1~exp1~20251221153213.50), Debian LLD 21.1.8
config:    https://ci.syzbot.org/builds/45c49e4e-439a-4d11-bc9a-3c3a5077f679/config
syz repro: https://ci.syzbot.org/findings/9cfb9381-576b-4a17-a156-68641410fec2/syz_repro

No such timeout policy "syz1"
------------[ cut here ]------------
help
WARNING: net/netfilter/xt_CT.c:226 at xt_ct_tg_check+0x814/0xa90 net/netfilter/xt_CT.c:226, CPU#1: syz.0.17/5870
Modules linked in:
CPU: 1 UID: 0 PID: 5870 Comm: syz.0.17 Not tainted syzkaller #0 PREEMPT(full) 
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
RIP: 0010:xt_ct_tg_check+0x814/0xa90 net/netfilter/xt_CT.c:226
Code: c7 c7 c0 a7 e6 8c e8 eb 33 3e f7 e9 12 ff ff ff e8 01 4b dc f7 48 c7 c7 40 a8 e6 8c 4c 89 ee e8 d2 33 3e f7 e9 f9 fe ff ff 90 <0f> 0b 90 4c 89 e0 48 c1 e8 03 42 80 3c 38 00 74 08 4c 89 e7 e8 73
RSP: 0018:ffffc900036ef6e0 EFLAGS: 00010282
RAX: ffff88812063bd10 RBX: 1ffff920006ddee4 RCX: 0000000000000010
RDX: ffff88812063bd00 RSI: 0000000000000002 RDI: 0000000000000002
RBP: ffffc900036ef7b0 R08: ffffffff90316c23 R09: 1ffffffff2062d84
R10: dffffc0000000000 R11: fffffbfff2062d85 R12: ffff88812063bd10
R13: 00000000fffffffe R14: ffff888113ee1800 R15: dffffc0000000000
FS:  00007fd41ea436c0(0000) GS:ffff8882a928a000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fd41da72780 CR3: 0000000175cc8000 CR4: 00000000000006f0
Call Trace:
 <TASK>
 xt_checkentry_target net/netfilter/x_tables.c:1115 [inline]
 xt_check_target+0x61a/0xca0 net/netfilter/x_tables.c:1138
 check_target net/ipv4/netfilter/ip_tables.c:510 [inline]
 find_check_entry net/ipv4/netfilter/ip_tables.c:552 [inline]
 translate_table+0x1881/0x2110 net/ipv4/netfilter/ip_tables.c:716
 do_replace net/ipv4/netfilter/ip_tables.c:1137 [inline]
 do_ipt_set_ctl+0x9f5/0xe00 net/ipv4/netfilter/ip_tables.c:1635
 nf_setsockopt+0x26f/0x290 net/netfilter/nf_sockopt.c:101
 do_sock_setsockopt+0x17c/0x1b0 net/socket.c:2381
 __sys_setsockopt net/socket.c:2406 [inline]
 __do_sys_setsockopt net/socket.c:2412 [inline]
 __se_sys_setsockopt net/socket.c:2409 [inline]
 __x64_sys_setsockopt+0x13d/0x1b0 net/socket.c:2409
 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
 do_syscall_64+0x15f/0xf80 arch/x86/entry/syscall_64.c:94
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7fd41db9ce59
Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 e8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fd41ea43028 EFLAGS: 00000246 ORIG_RAX: 0000000000000036
RAX: ffffffffffffffda RBX: 00007fd41de15fa0 RCX: 00007fd41db9ce59
RDX: 0000000000000040 RSI: 8001000000000000 RDI: 0000000000000003
RBP: 00007fd41dc32d6f R08: 00000000000002a8 R09: 0000000000000000
R10: 0000200000001500 R11: 0000000000000246 R12: 0000000000000000
R13: 00007fd41de16038 R14: 00007fd41de15fa0 R15: 00007ffdb7f510f8
 </TASK>


***

If these findings have caused you to resend the series or submit a
separate fix, please add the following tag to your commit message:
  Tested-by: syzbot@syzkaller.appspotmail.com

---
This report is generated by a bot. It may contain errors.
syzbot ci engineers can be reached at syzkaller@googlegroups.com.

To test a patch for this bug, please reply with `#syz test`
(should be on a separate line).

The patch should be attached to the email.
Note: arguments like custom git repos and branches are not supported.

Re: [syzbot ci] Re: netfilter: nfnetlink_cthelper: use {READ,WRITE}_ONCE for accessing helper flags

[Pablo Neira Ayuso]

Hi,

On Tue, May 19, 2026 at 11:23:57PM -0700, syzbot ci wrote:
> syzbot ci has tested the following series
> 
> [v1] netfilter: nfnetlink_cthelper: use {READ,WRITE}_ONCE for accessing helper flags
> https://lore.kernel.org/all/20260519213826.1181661-1-pablo@netfilter.org
> * [PATCH nf 1/7] netfilter: nfnetlink_cthelper: use {READ,WRITE}_ONCE for accessing helper flags
> * [PATCH nf 2/7] netfilter: conntrack: add dead flag to helpers
> * [PATCH nf 3/7] netfilter: nf_conntrack_helper: add null check in nfct_help_data() calls
> * [PATCH nf 4/7] netfilter: conntrack: add null check in nfct_help() calls
> * [PATCH nf 5/7] netfilter: conntrack: add nf_ct_iterate_destroy_net()
> * [PATCH nf 6/7] netfilter: nf_conntrack_timeout: use nf_ct_iterate_destroy() to cleanup timeout going away
> * [PATCH nf 7/7] netfilter: xt_CT: fix race with rule removal and nfnetlink_queue
> 
> and found the following issue:
> WARNING in xt_ct_tg_check

I added:

WARN_ON_ONCE(help)

instead of:

WARN_ON_ONCE(!help)

I will fix in the next spin.

> 
> Full report is available here:
> https://ci.syzbot.org/series/c356956d-b1f6-4d7e-be26-6cf68d49814e
> 
> ***
> 
> WARNING in xt_ct_tg_check
> 
> tree:      nf
> URL:       https://kernel.googlesource.com/pub/scm/linux/kernel/git/netfilter/nf.git
> base:      2beba18b0160446463bf1dbd749324846db98493
> arch:      amd64
> compiler:  Debian clang version 21.1.8 (++20251221033036+2078da43e25a-1~exp1~20251221153213.50), Debian LLD 21.1.8
> config:    https://ci.syzbot.org/builds/45c49e4e-439a-4d11-bc9a-3c3a5077f679/config
> syz repro: https://ci.syzbot.org/findings/9cfb9381-576b-4a17-a156-68641410fec2/syz_repro
> 
> No such timeout policy "syz1"
> ------------[ cut here ]------------
> help
> WARNING: net/netfilter/xt_CT.c:226 at xt_ct_tg_check+0x814/0xa90 net/netfilter/xt_CT.c:226, CPU#1: syz.0.17/5870
> Modules linked in:
> CPU: 1 UID: 0 PID: 5870 Comm: syz.0.17 Not tainted syzkaller #0 PREEMPT(full) 
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
> RIP: 0010:xt_ct_tg_check+0x814/0xa90 net/netfilter/xt_CT.c:226
> Code: c7 c7 c0 a7 e6 8c e8 eb 33 3e f7 e9 12 ff ff ff e8 01 4b dc f7 48 c7 c7 40 a8 e6 8c 4c 89 ee e8 d2 33 3e f7 e9 f9 fe ff ff 90 <0f> 0b 90 4c 89 e0 48 c1 e8 03 42 80 3c 38 00 74 08 4c 89 e7 e8 73
> RSP: 0018:ffffc900036ef6e0 EFLAGS: 00010282
> RAX: ffff88812063bd10 RBX: 1ffff920006ddee4 RCX: 0000000000000010
> RDX: ffff88812063bd00 RSI: 0000000000000002 RDI: 0000000000000002
> RBP: ffffc900036ef7b0 R08: ffffffff90316c23 R09: 1ffffffff2062d84
> R10: dffffc0000000000 R11: fffffbfff2062d85 R12: ffff88812063bd10
> R13: 00000000fffffffe R14: ffff888113ee1800 R15: dffffc0000000000
> FS:  00007fd41ea436c0(0000) GS:ffff8882a928a000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fd41da72780 CR3: 0000000175cc8000 CR4: 00000000000006f0
> Call Trace:
>  <TASK>
>  xt_checkentry_target net/netfilter/x_tables.c:1115 [inline]
>  xt_check_target+0x61a/0xca0 net/netfilter/x_tables.c:1138
>  check_target net/ipv4/netfilter/ip_tables.c:510 [inline]
>  find_check_entry net/ipv4/netfilter/ip_tables.c:552 [inline]
>  translate_table+0x1881/0x2110 net/ipv4/netfilter/ip_tables.c:716
>  do_replace net/ipv4/netfilter/ip_tables.c:1137 [inline]
>  do_ipt_set_ctl+0x9f5/0xe00 net/ipv4/netfilter/ip_tables.c:1635
>  nf_setsockopt+0x26f/0x290 net/netfilter/nf_sockopt.c:101
>  do_sock_setsockopt+0x17c/0x1b0 net/socket.c:2381
>  __sys_setsockopt net/socket.c:2406 [inline]
>  __do_sys_setsockopt net/socket.c:2412 [inline]
>  __se_sys_setsockopt net/socket.c:2409 [inline]
>  __x64_sys_setsockopt+0x13d/0x1b0 net/socket.c:2409
>  do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
>  do_syscall_64+0x15f/0xf80 arch/x86/entry/syscall_64.c:94
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7fd41db9ce59
> Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 e8 ff ff ff f7 d8 64 89 01 48
> RSP: 002b:00007fd41ea43028 EFLAGS: 00000246 ORIG_RAX: 0000000000000036
> RAX: ffffffffffffffda RBX: 00007fd41de15fa0 RCX: 00007fd41db9ce59
> RDX: 0000000000000040 RSI: 8001000000000000 RDI: 0000000000000003
> RBP: 00007fd41dc32d6f R08: 00000000000002a8 R09: 0000000000000000
> R10: 0000200000001500 R11: 0000000000000246 R12: 0000000000000000
> R13: 00007fd41de16038 R14: 00007fd41de15fa0 R15: 00007ffdb7f510f8
>  </TASK>
> 
> 
> ***
> 
> If these findings have caused you to resend the series or submit a
> separate fix, please add the following tag to your commit message:
>   Tested-by: syzbot@syzkaller.appspotmail.com
> 
> ---
> This report is generated by a bot. It may contain errors.
> syzbot ci engineers can be reached at syzkaller@googlegroups.com.
> 
> To test a patch for this bug, please reply with `#syz test`
> (should be on a separate line).
> 
> The patch should be attached to the email.
> Note: arguments like custom git repos and branches are not supported.