[PATCH nf 0/5] netfilter: nfnl_cthelper: fix some bugs

[PATCH nf 0/5] netfilter: nfnl_cthelper: fix some bugs

[Liping Zhang]

From: Liping Zhang <zlpnobody@gmail.com>

This patch set aims to fix some bugs related to nfnetlink_cthelper.
They are:
1. if NFCTH_PRIV_DATA_LEN attr is empty, we cannot create a cthelper
   via nfnetlink
2. helper->expect_class_max is incorrect
3. when update cthelper via nfnetlink, memory leak will happen. It's
   ok to modify the nf_conntrack_expect_policy directly when do update,
   but drop the const qualifier is required
4. introduce a dummy list to fix a race when operate nfnl_cthelper

Note, the patch set is based on these three patches which have not
been applied or showed in nf picktree:
http://patchwork.ozlabs.org/patch/740302/
http://patchwork.ozlabs.org/patch/740300/
http://patchwork.ozlabs.org/patch/739509/

Liping Zhang (5):
  netfilter: nfnl_cthelper: don't report error if NFCTH_PRIV_DATA_LEN is
    empty
  netfilter: nfnl_cthelper: fix incorrect helper->expect_class_max
  netfilter: drop const qualifier from struct nf_conntrack_expect_policy
  netfilter: nfnl_cthelper: fix memory leak when do update
  netfilter: nfnl_cthelper: fix a race when walk the nf_ct_helper_hash
    table

 include/net/netfilter/nf_conntrack_helper.h |   4 +-
 net/ipv4/netfilter/nf_nat_snmp_basic.c      |   2 +-
 net/netfilter/nf_conntrack_amanda.c         |   2 +-
 net/netfilter/nf_conntrack_expect.c         |   2 +-
 net/netfilter/nf_conntrack_ftp.c            |   2 +-
 net/netfilter/nf_conntrack_h323_main.c      |   6 +-
 net/netfilter/nf_conntrack_helper.c         |   2 +-
 net/netfilter/nf_conntrack_pptp.c           |   2 +-
 net/netfilter/nf_conntrack_sane.c           |   2 +-
 net/netfilter/nf_conntrack_sip.c            |   2 +-
 net/netfilter/nf_conntrack_tftp.c           |   2 +-
 net/netfilter/nfnetlink_cthelper.c          | 226 +++++++++++++++-------------
 12 files changed, 135 insertions(+), 119 deletions(-)

-- 
2.5.5

[PATCH nf 1/5] netfilter: nfnl_cthelper: don't report error if NFCTH_PRIV_DATA_LEN is empty

[Liping Zhang]

From: Liping Zhang <zlpnobody@gmail.com>

Currently, when we create cthelper via nfnetlink, -EINVAL will be
returned if the NFCTH_PRIV_DATA_LEN attribute is empty.

But enforcing the user to specify the NFCTH_PRIV_DATA_LEN attr seems
unnecessary, so it's better to set the helper->data_len to zero if
the NFCTH_PRIV_DATA_LEN attribute is empty.

Found by running example program from libnetfilter_cthelper:
  # ./libnetfilter_cthelper/examples/nfct-helper-add test 1
  error: Invalid argument

Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
---
 net/netfilter/nfnetlink_cthelper.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
index 2defe73..9c24301 100644
--- a/net/netfilter/nfnetlink_cthelper.c
+++ b/net/netfilter/nfnetlink_cthelper.c
@@ -205,7 +205,7 @@ nfnl_cthelper_create(const struct nlattr * const tb[],
 	struct nf_conntrack_helper *helper;
 	int ret;
 
-	if (!tb[NFCTH_TUPLE] || !tb[NFCTH_POLICY] || !tb[NFCTH_PRIV_DATA_LEN])
+	if (!tb[NFCTH_TUPLE] || !tb[NFCTH_POLICY])
 		return -EINVAL;
 
 	helper = kzalloc(sizeof(struct nf_conntrack_helper), GFP_KERNEL);
@@ -217,7 +217,8 @@ nfnl_cthelper_create(const struct nlattr * const tb[],
 		goto err1;
 
 	strncpy(helper->name, nla_data(tb[NFCTH_NAME]), NF_CT_HELPER_NAME_LEN);
-	helper->data_len = ntohl(nla_get_be32(tb[NFCTH_PRIV_DATA_LEN]));
+	if (tb[NFCTH_PRIV_DATA_LEN])
+		helper->data_len = ntohl(nla_get_be32(tb[NFCTH_PRIV_DATA_LEN]));
 	helper->flags |= NF_CT_HELPER_F_USERSPACE;
 	memcpy(&helper->tuple, tuple, sizeof(struct nf_conntrack_tuple));
 
-- 
2.5.5

[PATCH nf 2/5] netfilter: nfnl_cthelper: fix incorrect helper->expect_class_max

[Liping Zhang]

From: Liping Zhang <zlpnobody@gmail.com>

The helper->expect_class_max must be set to the total number of
expect_policy minus 1, since we will use the statement "if (class >
helper->expect_class_max)" to validate the CTA_EXPECT_CLASS attr in
ctnetlink_alloc_expect.

So for compatibility, set the helper->expect_class_max to the
NFCTH_POLICY_SET_NUM attr's value minus 1.

Also: it's invalid when the NFCTH_POLICY_SET_NUM attr's value is zero.
1. this will result "expect_policy = kzalloc(0, GFP_KERNEL);";
2. we cannot set the helper->expect_class_max to a proper value.

So if nla_get_be32(tb[NFCTH_POLICY_SET_NUM]) is zero, report -EINVAL to
the userspace.

Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
---
 net/netfilter/nfnetlink_cthelper.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
index 9c24301..cc70dd5 100644
--- a/net/netfilter/nfnetlink_cthelper.c
+++ b/net/netfilter/nfnetlink_cthelper.c
@@ -161,6 +161,7 @@ nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
 	int i, ret;
 	struct nf_conntrack_expect_policy *expect_policy;
 	struct nlattr *tb[NFCTH_POLICY_SET_MAX+1];
+	unsigned int class_max;
 
 	ret = nla_parse_nested(tb, NFCTH_POLICY_SET_MAX, attr,
 			       nfnl_cthelper_expect_policy_set);
@@ -170,19 +171,18 @@ nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
 	if (!tb[NFCTH_POLICY_SET_NUM])
 		return -EINVAL;
 
-	helper->expect_class_max =
-		ntohl(nla_get_be32(tb[NFCTH_POLICY_SET_NUM]));
-
-	if (helper->expect_class_max != 0 &&
-	    helper->expect_class_max > NF_CT_MAX_EXPECT_CLASSES)
+	class_max = ntohl(nla_get_be32(tb[NFCTH_POLICY_SET_NUM]));
+	if (class_max == 0)
+		return -EINVAL;
+	if (class_max > NF_CT_MAX_EXPECT_CLASSES)
 		return -EOVERFLOW;
 
 	expect_policy = kzalloc(sizeof(struct nf_conntrack_expect_policy) *
-				helper->expect_class_max, GFP_KERNEL);
+				class_max, GFP_KERNEL);
 	if (expect_policy == NULL)
 		return -ENOMEM;
 
-	for (i=0; i<helper->expect_class_max; i++) {
+	for (i = 0; i < class_max; i++) {
 		if (!tb[NFCTH_POLICY_SET+i])
 			goto err;
 
@@ -191,6 +191,8 @@ nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
 		if (ret < 0)
 			goto err;
 	}
+
+	helper->expect_class_max = class_max - 1;
 	helper->expect_policy = expect_policy;
 	return 0;
 err:
@@ -375,10 +377,10 @@ nfnl_cthelper_dump_policy(struct sk_buff *skb,
 		goto nla_put_failure;
 
 	if (nla_put_be32(skb, NFCTH_POLICY_SET_NUM,
-			 htonl(helper->expect_class_max)))
+			 htonl(helper->expect_class_max + 1)))
 		goto nla_put_failure;
 
-	for (i=0; i<helper->expect_class_max; i++) {
+	for (i = 0; i < helper->expect_class_max + 1; i++) {
 		nest_parms2 = nla_nest_start(skb,
 				(NFCTH_POLICY_SET+i) | NLA_F_NESTED);
 		if (nest_parms2 == NULL)
-- 
2.5.5

[PATCH nf 3/5] netfilter: drop const qualifier from struct nf_conntrack_expect_policy

[Liping Zhang]

From: Liping Zhang <zlpnobody@gmail.com>

So we can modify the nf_conntrack_expect_policy directly, the next patch
will need this.

Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
---
 include/net/netfilter/nf_conntrack_helper.h | 4 ++--
 net/ipv4/netfilter/nf_nat_snmp_basic.c      | 2 +-
 net/netfilter/nf_conntrack_amanda.c         | 2 +-
 net/netfilter/nf_conntrack_expect.c         | 2 +-
 net/netfilter/nf_conntrack_ftp.c            | 2 +-
 net/netfilter/nf_conntrack_h323_main.c      | 6 +++---
 net/netfilter/nf_conntrack_helper.c         | 2 +-
 net/netfilter/nf_conntrack_pptp.c           | 2 +-
 net/netfilter/nf_conntrack_sane.c           | 2 +-
 net/netfilter/nf_conntrack_sip.c            | 2 +-
 net/netfilter/nf_conntrack_tftp.c           | 2 +-
 11 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
index 1eaac1f..621a404 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -27,7 +27,7 @@ struct nf_conntrack_helper {
 
 	char name[NF_CT_HELPER_NAME_LEN]; /* name of the module */
 	struct module *me;		/* pointer to self */
-	const struct nf_conntrack_expect_policy *expect_policy;
+	struct nf_conntrack_expect_policy *expect_policy;
 
 	/* length of internal data, ie. sizeof(struct nf_ct_*_master) */
 	size_t data_len;
@@ -61,7 +61,7 @@ struct nf_conntrack_helper *nf_conntrack_helper_try_module_get(const char *name,
 void nf_ct_helper_init(struct nf_conntrack_helper *helper,
 		       u16 l3num, u16 protonum, const char *name,
 		       u16 default_port, u16 spec_port, u32 id,
-		       const struct nf_conntrack_expect_policy *exp_pol,
+		       struct nf_conntrack_expect_policy *exp_pol,
 		       u32 expect_class_max, u32 data_len,
 		       int (*help)(struct sk_buff *skb, unsigned int protoff,
 				   struct nf_conn *ct,
diff --git a/net/ipv4/netfilter/nf_nat_snmp_basic.c b/net/ipv4/netfilter/nf_nat_snmp_basic.c
index c9b52c3..5f32532 100644
--- a/net/ipv4/netfilter/nf_nat_snmp_basic.c
+++ b/net/ipv4/netfilter/nf_nat_snmp_basic.c
@@ -1255,7 +1255,7 @@ static int help(struct sk_buff *skb, unsigned int protoff,
 	return ret;
 }
 
-static const struct nf_conntrack_expect_policy snmp_exp_policy = {
+static struct nf_conntrack_expect_policy snmp_exp_policy = {
 	.max_expected	= 0,
 	.timeout	= 180,
 };
diff --git a/net/netfilter/nf_conntrack_amanda.c b/net/netfilter/nf_conntrack_amanda.c
index 57a26cc..fbf9f59 100644
--- a/net/netfilter/nf_conntrack_amanda.c
+++ b/net/netfilter/nf_conntrack_amanda.c
@@ -167,7 +167,7 @@ static int amanda_help(struct sk_buff *skb,
 	return ret;
 }
 
-static const struct nf_conntrack_expect_policy amanda_exp_policy = {
+static struct nf_conntrack_expect_policy amanda_exp_policy = {
 	.max_expected		= 3,
 	.timeout		= 180,
 };
diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
index 4b2e1fb..2d81122 100644
--- a/net/netfilter/nf_conntrack_expect.c
+++ b/net/netfilter/nf_conntrack_expect.c
@@ -402,7 +402,7 @@ static void evict_oldest_expect(struct nf_conn *master,
 
 static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
 {
-	const struct nf_conntrack_expect_policy *p;
+	struct nf_conntrack_expect_policy *p;
 	struct nf_conntrack_expect *i;
 	struct nf_conn *master = expect->master;
 	struct nf_conn_help *master_help = nfct_help(master);
diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
index 4aecef4..ad51dd3 100644
--- a/net/netfilter/nf_conntrack_ftp.c
+++ b/net/netfilter/nf_conntrack_ftp.c
@@ -561,7 +561,7 @@ static int nf_ct_ftp_from_nlattr(struct nlattr *attr, struct nf_conn *ct)
 
 static struct nf_conntrack_helper ftp[MAX_PORTS * 2] __read_mostly;
 
-static const struct nf_conntrack_expect_policy ftp_exp_policy = {
+static struct nf_conntrack_expect_policy ftp_exp_policy = {
 	.max_expected	= 1,
 	.timeout	= 5 * 60,
 };
diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
index f65d936..2223c57 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c
@@ -629,7 +629,7 @@ static int h245_help(struct sk_buff *skb, unsigned int protoff,
 }
 
 /****************************************************************************/
-static const struct nf_conntrack_expect_policy h245_exp_policy = {
+static struct nf_conntrack_expect_policy h245_exp_policy = {
 	.max_expected	= H323_RTP_CHANNEL_MAX * 4 + 2 /* T.120 */,
 	.timeout	= 240,
 };
@@ -1205,7 +1205,7 @@ static int q931_help(struct sk_buff *skb, unsigned int protoff,
 }
 
 /****************************************************************************/
-static const struct nf_conntrack_expect_policy q931_exp_policy = {
+static struct nf_conntrack_expect_policy q931_exp_policy = {
 	/* T.120 and H.245 */
 	.max_expected		= H323_RTP_CHANNEL_MAX * 4 + 4,
 	.timeout		= 240,
@@ -1791,7 +1791,7 @@ static int ras_help(struct sk_buff *skb, unsigned int protoff,
 }
 
 /****************************************************************************/
-static const struct nf_conntrack_expect_policy ras_exp_policy = {
+static struct nf_conntrack_expect_policy ras_exp_policy = {
 	.max_expected		= 32,
 	.timeout		= 240,
 };
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 6dc44d9..b8591fb 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -483,7 +483,7 @@ EXPORT_SYMBOL_GPL(nf_conntrack_helper_unregister);
 void nf_ct_helper_init(struct nf_conntrack_helper *helper,
 		       u16 l3num, u16 protonum, const char *name,
 		       u16 default_port, u16 spec_port, u32 id,
-		       const struct nf_conntrack_expect_policy *exp_pol,
+		       struct nf_conntrack_expect_policy *exp_pol,
 		       u32 expect_class_max, u32 data_len,
 		       int (*help)(struct sk_buff *skb, unsigned int protoff,
 				   struct nf_conn *ct,
diff --git a/net/netfilter/nf_conntrack_pptp.c b/net/netfilter/nf_conntrack_pptp.c
index f60a475..1f20d6a 100644
--- a/net/netfilter/nf_conntrack_pptp.c
+++ b/net/netfilter/nf_conntrack_pptp.c
@@ -587,7 +587,7 @@ conntrack_pptp_help(struct sk_buff *skb, unsigned int protoff,
 	return ret;
 }
 
-static const struct nf_conntrack_expect_policy pptp_exp_policy = {
+static struct nf_conntrack_expect_policy pptp_exp_policy = {
 	.max_expected	= 2,
 	.timeout	= 5 * 60,
 };
diff --git a/net/netfilter/nf_conntrack_sane.c b/net/netfilter/nf_conntrack_sane.c
index 9dcb9ee..d54cfec 100644
--- a/net/netfilter/nf_conntrack_sane.c
+++ b/net/netfilter/nf_conntrack_sane.c
@@ -168,7 +168,7 @@ static int help(struct sk_buff *skb,
 
 static struct nf_conntrack_helper sane[MAX_PORTS * 2] __read_mostly;
 
-static const struct nf_conntrack_expect_policy sane_exp_policy = {
+static struct nf_conntrack_expect_policy sane_exp_policy = {
 	.max_expected	= 1,
 	.timeout	= 5 * 60,
 };
diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index 0d17894..3ec7b734 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -1592,7 +1592,7 @@ static int sip_help_udp(struct sk_buff *skb, unsigned int protoff,
 
 static struct nf_conntrack_helper sip[MAX_PORTS * 4] __read_mostly;
 
-static const struct nf_conntrack_expect_policy sip_exp_policy[SIP_EXPECT_MAX + 1] = {
+static struct nf_conntrack_expect_policy sip_exp_policy[SIP_EXPECT_MAX + 1] = {
 	[SIP_EXPECT_SIGNALLING] = {
 		.name		= "signalling",
 		.max_expected	= 1,
diff --git a/net/netfilter/nf_conntrack_tftp.c b/net/netfilter/nf_conntrack_tftp.c
index b1227dc..fcc4965 100644
--- a/net/netfilter/nf_conntrack_tftp.c
+++ b/net/netfilter/nf_conntrack_tftp.c
@@ -99,7 +99,7 @@ static int tftp_help(struct sk_buff *skb,
 
 static struct nf_conntrack_helper tftp[MAX_PORTS * 2] __read_mostly;
 
-static const struct nf_conntrack_expect_policy tftp_exp_policy = {
+static struct nf_conntrack_expect_policy tftp_exp_policy = {
 	.max_expected	= 1,
 	.timeout	= 5 * 60,
 };
-- 
2.5.5

[PATCH nf 4/5] netfilter: nfnl_cthelper: fix memory leak when do update

[Liping Zhang]

From: Liping Zhang <zlpnobody@gmail.com>

When invoke nfnl_cthelper_update, we will malloc a new expect_policy,
then only point the helper->expect_policy to the new one but ignore
the old one, so it will be leaked forever.

Another issue is that the user can modify the expect_class_max to a
new value, for example, decrease the expect_class_max from 3 to 0.
Then, exp->class created by ctnetlink_alloc_expect may become invalid,
and out of bound access will happen.

So keep it simple, when update the cthelper, reject to modify the
expect_class_max, and we can modify the struct nf_conntrack_expect_policy
directly instead of allocing a new object, then memory leak does not
exist anymore.

Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
---
 net/netfilter/nfnetlink_cthelper.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
index cc70dd5..fc4733f 100644
--- a/net/netfilter/nfnetlink_cthelper.c
+++ b/net/netfilter/nfnetlink_cthelper.c
@@ -156,7 +156,7 @@ nfnl_cthelper_expect_policy_set[NFCTH_POLICY_SET_MAX+1] = {
 
 static int
 nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
-				  const struct nlattr *attr)
+				  const struct nlattr *attr, bool update)
 {
 	int i, ret;
 	struct nf_conntrack_expect_policy *expect_policy;
@@ -172,15 +172,23 @@ nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
 		return -EINVAL;
 
 	class_max = ntohl(nla_get_be32(tb[NFCTH_POLICY_SET_NUM]));
-	if (class_max == 0)
-		return -EINVAL;
-	if (class_max > NF_CT_MAX_EXPECT_CLASSES)
-		return -EOVERFLOW;
 
-	expect_policy = kzalloc(sizeof(struct nf_conntrack_expect_policy) *
-				class_max, GFP_KERNEL);
-	if (expect_policy == NULL)
-		return -ENOMEM;
+	if (update) {
+		if (helper->expect_class_max + 1 != class_max)
+			return -EINVAL;
+
+		expect_policy = helper->expect_policy;
+	} else {
+		if (class_max == 0)
+			return -EINVAL;
+		if (class_max > NF_CT_MAX_EXPECT_CLASSES)
+			return -EOVERFLOW;
+
+		expect_policy = kcalloc(class_max, sizeof(*expect_policy),
+					GFP_KERNEL);
+		if (expect_policy == NULL)
+			return -ENOMEM;
+	}
 
 	for (i = 0; i < class_max; i++) {
 		if (!tb[NFCTH_POLICY_SET+i])
@@ -196,7 +204,8 @@ nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
 	helper->expect_policy = expect_policy;
 	return 0;
 err:
-	kfree(expect_policy);
+	if (!update)
+		kfree(expect_policy);
 	return -EINVAL;
 }
 
@@ -214,7 +223,8 @@ nfnl_cthelper_create(const struct nlattr * const tb[],
 	if (helper == NULL)
 		return -ENOMEM;
 
-	ret = nfnl_cthelper_parse_expect_policy(helper, tb[NFCTH_POLICY]);
+	ret = nfnl_cthelper_parse_expect_policy(helper, tb[NFCTH_POLICY],
+						false);
 	if (ret < 0)
 		goto err1;
 
@@ -269,7 +279,8 @@ nfnl_cthelper_update(const struct nlattr * const tb[],
 
 	if (tb[NFCTH_POLICY]) {
 		ret = nfnl_cthelper_parse_expect_policy(helper,
-							tb[NFCTH_POLICY]);
+							tb[NFCTH_POLICY],
+							true);
 		if (ret < 0)
 			return ret;
 	}
-- 
2.5.5

[PATCH nf 5/5] netfilter: nfnl_cthelper: fix a race when walk the nf_ct_helper_hash table

[Liping Zhang]

From: Liping Zhang <zlpnobody@gmail.com>

The nf_ct_helper_hash table is protected by nf_ct_helper_mutex, while
nfct_helper operation is protected by nfnl_lock(NFNL_SUBSYS_CTHELPER).
So it's possible that one CPU is walking the nf_ct_helper_hash for
cthelper add/get/del, another cpu is doing nf_conntrack_helpers_unregister
at the same time. This is dangrous, and may cause use after free error.

Note, delete operation will flush all cthelpers added via nfnetlink, so
using rcu to do protect is not easy.

Now introduce a dummy list to record all the cthelpers added via
nfnetlink, then we can walk the dummy list instead of walking the
nf_ct_helper_hash. Also, keep nfnl_cthelper_dump_table unchanged, it
may be invoked without nfnl_lock(NFNL_SUBSYS_CTHELPER) held.

Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
---
 net/netfilter/nfnetlink_cthelper.c | 176 +++++++++++++++++++------------------
 1 file changed, 89 insertions(+), 87 deletions(-)

diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
index fc4733f..47424ec 100644
--- a/net/netfilter/nfnetlink_cthelper.c
+++ b/net/netfilter/nfnetlink_cthelper.c
@@ -32,6 +32,13 @@ MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Pablo Neira Ayuso <pablo@netfilter.org>");
 MODULE_DESCRIPTION("nfnl_cthelper: User-space connection tracking helpers");
 
+struct nfnl_cthelper {
+	struct list_head		list;
+	struct nf_conntrack_helper	*helper;
+};
+
+static LIST_HEAD(nfnl_cthelper_list);
+
 static int
 nfnl_userspace_cthelper(struct sk_buff *skb, unsigned int protoff,
 			struct nf_conn *ct, enum ip_conntrack_info ctinfo)
@@ -214,14 +221,21 @@ nfnl_cthelper_create(const struct nlattr * const tb[],
 		     struct nf_conntrack_tuple *tuple)
 {
 	struct nf_conntrack_helper *helper;
+	struct nfnl_cthelper *nfcth;
 	int ret;
 
 	if (!tb[NFCTH_TUPLE] || !tb[NFCTH_POLICY])
 		return -EINVAL;
 
+	nfcth = kmalloc(sizeof(struct nfnl_cthelper), GFP_KERNEL);
+	if (nfcth == NULL)
+		return -ENOMEM;
+
 	helper = kzalloc(sizeof(struct nf_conntrack_helper), GFP_KERNEL);
-	if (helper == NULL)
+	if (helper == NULL) {
+		kfree(nfcth);
 		return -ENOMEM;
+	}
 
 	ret = nfnl_cthelper_parse_expect_policy(helper, tb[NFCTH_POLICY],
 						false);
@@ -260,10 +274,13 @@ nfnl_cthelper_create(const struct nlattr * const tb[],
 	if (ret < 0)
 		goto err2;
 
+	nfcth->helper = helper;
+	list_add_tail(&nfcth->list, &nfnl_cthelper_list);
 	return 0;
 err2:
 	kfree(helper->expect_policy);
 err1:
+	kfree(nfcth);
 	kfree(helper);
 	return ret;
 }
@@ -309,7 +326,8 @@ static int nfnl_cthelper_new(struct net *net, struct sock *nfnl,
 	const char *helper_name;
 	struct nf_conntrack_helper *cur, *helper = NULL;
 	struct nf_conntrack_tuple tuple;
-	int ret = 0, i;
+	struct nfnl_cthelper *nlcth;
+	int ret = 0;
 
 	if (!tb[NFCTH_NAME] || !tb[NFCTH_TUPLE])
 		return -EINVAL;
@@ -320,28 +338,22 @@ static int nfnl_cthelper_new(struct net *net, struct sock *nfnl,
 	if (ret < 0)
 		return ret;
 
-	for (i = 0; i < nf_ct_helper_hsize && !helper; i++) {
-		hlist_for_each_entry(cur, &nf_ct_helper_hash[i], hnode) {
-
-			/* skip non-userspace conntrack helpers. */
-			if (!(cur->flags & NF_CT_HELPER_F_USERSPACE))
-				continue;
+	list_for_each_entry(nlcth, &nfnl_cthelper_list, list) {
+		cur = nlcth->helper;
 
-			if (strncmp(cur->name, helper_name,
-					NF_CT_HELPER_NAME_LEN) != 0)
-				continue;
+		if (strncmp(cur->name, helper_name, NF_CT_HELPER_NAME_LEN))
+			continue;
 
-			if ((tuple.src.l3num != cur->tuple.src.l3num ||
-			     tuple.dst.protonum != cur->tuple.dst.protonum))
-				continue;
+		if ((tuple.src.l3num != cur->tuple.src.l3num ||
+		     tuple.dst.protonum != cur->tuple.dst.protonum))
+			continue;
 
-			if (nlh->nlmsg_flags & NLM_F_EXCL) {
-				ret = -EEXIST;
-				goto err;
-			}
-			helper = cur;
-			break;
+		if (nlh->nlmsg_flags & NLM_F_EXCL) {
+			ret = -EEXIST;
+			goto err;
 		}
+		helper = cur;
+		break;
 	}
 
 	if (helper == NULL)
@@ -513,11 +525,12 @@ static int nfnl_cthelper_get(struct net *net, struct sock *nfnl,
 			     struct sk_buff *skb, const struct nlmsghdr *nlh,
 			     const struct nlattr * const tb[])
 {
-	int ret = -ENOENT, i;
+	int ret = -ENOENT;
 	struct nf_conntrack_helper *cur;
 	struct sk_buff *skb2;
 	char *helper_name = NULL;
 	struct nf_conntrack_tuple tuple;
+	struct nfnl_cthelper *nlcth;
 	bool tuple_set = false;
 
 	if (nlh->nlmsg_flags & NLM_F_DUMP) {
@@ -538,45 +551,39 @@ static int nfnl_cthelper_get(struct net *net, struct sock *nfnl,
 		tuple_set = true;
 	}
 
-	for (i = 0; i < nf_ct_helper_hsize; i++) {
-		hlist_for_each_entry_rcu(cur, &nf_ct_helper_hash[i], hnode) {
+	list_for_each_entry(nlcth, &nfnl_cthelper_list, list) {
+		cur = nlcth->helper;
+		if (helper_name &&
+		    strncmp(cur->name, helper_name, NF_CT_HELPER_NAME_LEN))
+			continue;
 
-			/* skip non-userspace conntrack helpers. */
-			if (!(cur->flags & NF_CT_HELPER_F_USERSPACE))
-				continue;
-
-			if (helper_name && strncmp(cur->name, helper_name,
-						NF_CT_HELPER_NAME_LEN) != 0) {
-				continue;
-			}
-			if (tuple_set &&
-			    (tuple.src.l3num != cur->tuple.src.l3num ||
-			     tuple.dst.protonum != cur->tuple.dst.protonum))
-				continue;
+		if (tuple_set &&
+		    (tuple.src.l3num != cur->tuple.src.l3num ||
+		     tuple.dst.protonum != cur->tuple.dst.protonum))
+			continue;
 
-			skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
-			if (skb2 == NULL) {
-				ret = -ENOMEM;
-				break;
-			}
+		skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+		if (skb2 == NULL) {
+			ret = -ENOMEM;
+			break;
+		}
 
-			ret = nfnl_cthelper_fill_info(skb2, NETLINK_CB(skb).portid,
-						nlh->nlmsg_seq,
-						NFNL_MSG_TYPE(nlh->nlmsg_type),
-						NFNL_MSG_CTHELPER_NEW, cur);
-			if (ret <= 0) {
-				kfree_skb(skb2);
-				break;
-			}
+		ret = nfnl_cthelper_fill_info(skb2, NETLINK_CB(skb).portid,
+					      nlh->nlmsg_seq,
+					      NFNL_MSG_TYPE(nlh->nlmsg_type),
+					      NFNL_MSG_CTHELPER_NEW, cur);
+		if (ret <= 0) {
+			kfree_skb(skb2);
+			break;
+		}
 
-			ret = netlink_unicast(nfnl, skb2, NETLINK_CB(skb).portid,
-						MSG_DONTWAIT);
-			if (ret > 0)
-				ret = 0;
+		ret = netlink_unicast(nfnl, skb2, NETLINK_CB(skb).portid,
+				      MSG_DONTWAIT);
+		if (ret > 0)
+			ret = 0;
 
-			/* this avoids a loop in nfnetlink. */
-			return ret == -EAGAIN ? -ENOBUFS : ret;
-		}
+		/* this avoids a loop in nfnetlink. */
+		return ret == -EAGAIN ? -ENOBUFS : ret;
 	}
 	return ret;
 }
@@ -587,10 +594,10 @@ static int nfnl_cthelper_del(struct net *net, struct sock *nfnl,
 {
 	char *helper_name = NULL;
 	struct nf_conntrack_helper *cur;
-	struct hlist_node *tmp;
 	struct nf_conntrack_tuple tuple;
 	bool tuple_set = false, found = false;
-	int i, j = 0, ret;
+	struct nfnl_cthelper *nlcth, *n;
+	int j = 0, ret;
 
 	if (tb[NFCTH_NAME])
 		helper_name = nla_data(tb[NFCTH_NAME]);
@@ -603,30 +610,28 @@ static int nfnl_cthelper_del(struct net *net, struct sock *nfnl,
 		tuple_set = true;
 	}
 
-	for (i = 0; i < nf_ct_helper_hsize; i++) {
-		hlist_for_each_entry_safe(cur, tmp, &nf_ct_helper_hash[i],
-								hnode) {
-			/* skip non-userspace conntrack helpers. */
-			if (!(cur->flags & NF_CT_HELPER_F_USERSPACE))
-				continue;
+	list_for_each_entry_safe(nlcth, n, &nfnl_cthelper_list, list) {
+		cur = nlcth->helper;
+		j++;
 
-			j++;
+		if (helper_name &&
+		    strncmp(cur->name, helper_name, NF_CT_HELPER_NAME_LEN))
+			continue;
 
-			if (helper_name && strncmp(cur->name, helper_name,
-						NF_CT_HELPER_NAME_LEN) != 0) {
-				continue;
-			}
-			if (tuple_set &&
-			    (tuple.src.l3num != cur->tuple.src.l3num ||
-			     tuple.dst.protonum != cur->tuple.dst.protonum))
-				continue;
+		if (tuple_set &&
+		    (tuple.src.l3num != cur->tuple.src.l3num ||
+		     tuple.dst.protonum != cur->tuple.dst.protonum))
+			continue;
 
-			found = true;
-			nf_conntrack_helper_unregister(cur);
-			kfree(cur->expect_policy);
-			kfree(cur);
-		}
+		found = true;
+		nf_conntrack_helper_unregister(cur);
+		kfree(cur->expect_policy);
+		kfree(cur);
+
+		list_del(&nlcth->list);
+		kfree(nlcth);
 	}
+
 	/* Make sure we return success if we flush and there is no helpers */
 	return (found || j == 0) ? 0 : -ENOENT;
 }
@@ -675,20 +680,17 @@ static int __init nfnl_cthelper_init(void)
 static void __exit nfnl_cthelper_exit(void)
 {
 	struct nf_conntrack_helper *cur;
-	struct hlist_node *tmp;
-	int i;
+	struct nfnl_cthelper *nlcth, *n;
 
 	nfnetlink_subsys_unregister(&nfnl_cthelper_subsys);
 
-	for (i=0; i<nf_ct_helper_hsize; i++) {
-		hlist_for_each_entry_safe(cur, tmp, &nf_ct_helper_hash[i],
-									hnode) {
-			/* skip non-userspace conntrack helpers. */
-			if (!(cur->flags & NF_CT_HELPER_F_USERSPACE))
-				continue;
+	list_for_each_entry_safe(nlcth, n, &nfnl_cthelper_list, list) {
+		cur = nlcth->helper;
 
-			nf_conntrack_helper_unregister(cur);
-		}
+		nf_conntrack_helper_unregister(cur);
+		kfree(cur->expect_policy);
+		kfree(cur);
+		kfree(nlcth);
 	}
 }
 
-- 
2.5.5

Re: [PATCH nf 1/5] netfilter: nfnl_cthelper: don't report error if NFCTH_PRIV_DATA_LEN is empty

[Pablo Neira Ayuso]

On Sun, Mar 19, 2017 at 10:35:58PM +0800, Liping Zhang wrote:
> From: Liping Zhang <zlpnobody@gmail.com>
> 
> Currently, when we create cthelper via nfnetlink, -EINVAL will be
> returned if the NFCTH_PRIV_DATA_LEN attribute is empty.
> 
> But enforcing the user to specify the NFCTH_PRIV_DATA_LEN attr seems
> unnecessary, so it's better to set the helper->data_len to zero if
> the NFCTH_PRIV_DATA_LEN attribute is empty.
> 
> Found by running example program from libnetfilter_cthelper:
>   # ./libnetfilter_cthelper/examples/nfct-helper-add test 1
>   error: Invalid argument

I suggest you fix this userspace example instead, we should always
send NFCTH_PRIV_DATA_LEN. This is integral part of the helper
description.

NFCTH_ATTR_PRIV_DATA_LEN has been always set from the conntrack-tools,
so most likely this example just got outdated at some point of the
development and nobody noticed so far.

Thanks.

Re: [PATCH nf 2/5] netfilter: nfnl_cthelper: fix incorrect helper->expect_class_max

[Pablo Neira Ayuso]

On Sun, Mar 19, 2017 at 10:35:59PM +0800, Liping Zhang wrote:
> From: Liping Zhang <zlpnobody@gmail.com>
> 
> The helper->expect_class_max must be set to the total number of
> expect_policy minus 1, since we will use the statement "if (class >
> helper->expect_class_max)" to validate the CTA_EXPECT_CLASS attr in
> ctnetlink_alloc_expect.
> 
> So for compatibility, set the helper->expect_class_max to the
> NFCTH_POLICY_SET_NUM attr's value minus 1.
> 
> Also: it's invalid when the NFCTH_POLICY_SET_NUM attr's value is zero.
> 1. this will result "expect_policy = kzalloc(0, GFP_KERNEL);";
> 2. we cannot set the helper->expect_class_max to a proper value.
> 
> So if nla_get_be32(tb[NFCTH_POLICY_SET_NUM]) is zero, report -EINVAL to
> the userspace.
> 
> Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
> ---
>  net/netfilter/nfnetlink_cthelper.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
> index 9c24301..cc70dd5 100644
> --- a/net/netfilter/nfnetlink_cthelper.c
> +++ b/net/netfilter/nfnetlink_cthelper.c
> @@ -161,6 +161,7 @@ nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
>  	int i, ret;
>  	struct nf_conntrack_expect_policy *expect_policy;
>  	struct nlattr *tb[NFCTH_POLICY_SET_MAX+1];
> +	unsigned int class_max;
>  
>  	ret = nla_parse_nested(tb, NFCTH_POLICY_SET_MAX, attr,
>  			       nfnl_cthelper_expect_policy_set);
> @@ -170,19 +171,18 @@ nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
>  	if (!tb[NFCTH_POLICY_SET_NUM])
>  		return -EINVAL;
>  
> -	helper->expect_class_max =
> -		ntohl(nla_get_be32(tb[NFCTH_POLICY_SET_NUM]));
> -
> -	if (helper->expect_class_max != 0 &&
> -	    helper->expect_class_max > NF_CT_MAX_EXPECT_CLASSES)
> +	class_max = ntohl(nla_get_be32(tb[NFCTH_POLICY_SET_NUM]));
> +	if (class_max == 0)
> +		return -EINVAL;

I think this patch is just fixing up this case. We should always
provide a policy for the expectation.

> +	if (class_max > NF_CT_MAX_EXPECT_CLASSES)
>  		return -EOVERFLOW;
>  
>  	expect_policy = kzalloc(sizeof(struct nf_conntrack_expect_policy) *
> -				helper->expect_class_max, GFP_KERNEL);
> +				class_max, GFP_KERNEL);
>  	if (expect_policy == NULL)
>  		return -ENOMEM;
>  
> -	for (i=0; i<helper->expect_class_max; i++) {
> +	for (i = 0; i < class_max; i++) {
>  		if (!tb[NFCTH_POLICY_SET+i])
>  			goto err;
>  
> @@ -191,6 +191,8 @@ nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
>  		if (ret < 0)
>  			goto err;
>  	}
> +
> +	helper->expect_class_max = class_max - 1;

Why - 1 ?

class_max represents the array size, you can just keep this code the
way it is.

>  	helper->expect_policy = expect_policy;
>  	return 0;
>  err:
> @@ -375,10 +377,10 @@ nfnl_cthelper_dump_policy(struct sk_buff *skb,
>  		goto nla_put_failure;
>  
>  	if (nla_put_be32(skb, NFCTH_POLICY_SET_NUM,
> -			 htonl(helper->expect_class_max)))
> +			 htonl(helper->expect_class_max + 1)))
>  		goto nla_put_failure;
>  
> -	for (i=0; i<helper->expect_class_max; i++) {
> +	for (i = 0; i < helper->expect_class_max + 1; i++) {
>  		nest_parms2 = nla_nest_start(skb,
>  				(NFCTH_POLICY_SET+i) | NLA_F_NESTED);
>  		if (nest_parms2 == NULL)
> -- 
> 2.5.5
> 
> 

Re: [PATCH nf 4/5] netfilter: nfnl_cthelper: fix memory leak when do update

[Pablo Neira Ayuso]

On Sun, Mar 19, 2017 at 10:36:01PM +0800, Liping Zhang wrote:
> From: Liping Zhang <zlpnobody@gmail.com>
> 
> When invoke nfnl_cthelper_update, we will malloc a new expect_policy,
> then only point the helper->expect_policy to the new one but ignore
> the old one, so it will be leaked forever.
> 
> Another issue is that the user can modify the expect_class_max to a
> new value, for example, decrease the expect_class_max from 3 to 0.

If the code is allowing this, we should fix it since this is not
valid. We cannot change the number of classes once the helper has been
created.

Users may update the maximum number of expectations and its timeout
per policy, but not the number of classes once this has been created.

Re: [PATCH nf 5/5] netfilter: nfnl_cthelper: fix a race when walk the nf_ct_helper_hash table

[Pablo Neira Ayuso]

On Sun, Mar 19, 2017 at 10:36:02PM +0800, Liping Zhang wrote:
> From: Liping Zhang <zlpnobody@gmail.com>
> 
> The nf_ct_helper_hash table is protected by nf_ct_helper_mutex, while
> nfct_helper operation is protected by nfnl_lock(NFNL_SUBSYS_CTHELPER).
> So it's possible that one CPU is walking the nf_ct_helper_hash for
> cthelper add/get/del, another cpu is doing nf_conntrack_helpers_unregister
> at the same time. This is dangrous, and may cause use after free error.
> 
> Note, delete operation will flush all cthelpers added via nfnetlink, so
> using rcu to do protect is not easy.
> 
> Now introduce a dummy list to record all the cthelpers added via
> nfnetlink, then we can walk the dummy list instead of walking the
> nf_ct_helper_hash. Also, keep nfnl_cthelper_dump_table unchanged, it
> may be invoked without nfnl_lock(NFNL_SUBSYS_CTHELPER) held.
> 
> Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
> ---
>  net/netfilter/nfnetlink_cthelper.c | 176 +++++++++++++++++++------------------
>  1 file changed, 89 insertions(+), 87 deletions(-)
> 
> diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
> index fc4733f..47424ec 100644
> --- a/net/netfilter/nfnetlink_cthelper.c
> +++ b/net/netfilter/nfnetlink_cthelper.c
> @@ -32,6 +32,13 @@ MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Pablo Neira Ayuso <pablo@netfilter.org>");
>  MODULE_DESCRIPTION("nfnl_cthelper: User-space connection tracking helpers");
>  
> +struct nfnl_cthelper {
> +	struct list_head		list;
> +	struct nf_conntrack_helper	*helper;
> +};
> +
> +static LIST_HEAD(nfnl_cthelper_list);

We need a field possible_net_t so we can store what netns this helper
belongs to, thus in case of flush command, we just remove the helpers
that this netns owns.

Re: [PATCH nf 3/5] netfilter: drop const qualifier from struct nf_conntrack_expect_policy

[Pablo Neira Ayuso]

On Sun, Mar 19, 2017 at 10:36:00PM +0800, Liping Zhang wrote:
> From: Liping Zhang <zlpnobody@gmail.com>
> 
> So we can modify the nf_conntrack_expect_policy directly, the next patch
> will need this.

I'm missing why you need this.

Re: [PATCH nf 4/5] netfilter: nfnl_cthelper: fix memory leak when do update

[Pablo Neira Ayuso]

On Tue, Mar 21, 2017 at 11:32:08AM +0100, Pablo Neira Ayuso wrote:
> On Sun, Mar 19, 2017 at 10:36:01PM +0800, Liping Zhang wrote:
> > From: Liping Zhang <zlpnobody@gmail.com>
> > 
> > When invoke nfnl_cthelper_update, we will malloc a new expect_policy,
> > then only point the helper->expect_policy to the new one but ignore
> > the old one, so it will be leaked forever.
> > 
> > Another issue is that the user can modify the expect_class_max to a
> > new value, for example, decrease the expect_class_max from 3 to 0.
> 
> If the code is allowing this, we should fix it since this is not
> valid. We cannot change the number of classes once the helper has been
> created.
> 
> Users may update the maximum number of expectations and its timeout
> per policy, but not the number of classes once this has been created.

Just sent a patch to sort out this.

You can rebase on top of nf.git as soon as I get those patches pushed
out, will wait a bit to wait for review and give it some testing here.

Thanks.

Re: [PATCH nf 1/5] netfilter: nfnl_cthelper: don't report error if NFCTH_PRIV_DATA_LEN is empty

[Liping Zhang]

Hi Pablo,

2017-03-21 18:18 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> On Sun, Mar 19, 2017 at 10:35:58PM +0800, Liping Zhang wrote:
>> From: Liping Zhang <zlpnobody@gmail.com>
>>
>> Currently, when we create cthelper via nfnetlink, -EINVAL will be
>> returned if the NFCTH_PRIV_DATA_LEN attribute is empty.
>>
>> But enforcing the user to specify the NFCTH_PRIV_DATA_LEN attr seems
>> unnecessary, so it's better to set the helper->data_len to zero if
>> the NFCTH_PRIV_DATA_LEN attribute is empty.
>>
>> Found by running example program from libnetfilter_cthelper:
>>   # ./libnetfilter_cthelper/examples/nfct-helper-add test 1
>>   error: Invalid argument
>
> I suggest you fix this userspace example instead, we should always
> send NFCTH_PRIV_DATA_LEN. This is integral part of the helper
> description.
>
> NFCTH_ATTR_PRIV_DATA_LEN has been always set from the conntrack-tools,
> so most likely this example just got outdated at some point of the
> development and nobody noticed so far.

OK, get it. I will send a patch to fix the example codes.

>
> Thanks.

Re: [PATCH nf 2/5] netfilter: nfnl_cthelper: fix incorrect helper->expect_class_max

[Liping Zhang]

Hi Pablo,

2017-03-21 18:27 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
[...]
>> +     class_max = ntohl(nla_get_be32(tb[NFCTH_POLICY_SET_NUM]));
>> +     if (class_max == 0)
>> +             return -EINVAL;
>
> I think this patch is just fixing up this case. We should always
> provide a policy for the expectation.

No, see comments below.

>
>> +     if (class_max > NF_CT_MAX_EXPECT_CLASSES)
>>               return -EOVERFLOW;
>>
>>       expect_policy = kzalloc(sizeof(struct nf_conntrack_expect_policy) *
>> -                             helper->expect_class_max, GFP_KERNEL);
>> +                             class_max, GFP_KERNEL);
>>       if (expect_policy == NULL)
>>               return -ENOMEM;
>>
>> -     for (i=0; i<helper->expect_class_max; i++) {
>> +     for (i = 0; i < class_max; i++) {
>>               if (!tb[NFCTH_POLICY_SET+i])
>>                       goto err;
>>
>> @@ -191,6 +191,8 @@ nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
>>               if (ret < 0)
>>                       goto err;
>>       }
>> +
>> +     helper->expect_class_max = class_max - 1;
>
> Why - 1 ?
>
> class_max represents the array size, you can just keep this code the
> way it is.

Actually, expect_class_max represents the array size minus 1.

For sip, expect_class_max is set to SIP_EXPECT_MAX(3). For ftp,
expect_class_max is set to 0.

Also there's a "BUG_ON(me->expect_class_max >= NF_CT_MAX_EXPECT_CLASSES);"
in nf_conntrack_helper_register, so if we supply 4 expect_policys,
i.e. expect_class_max is 4, BUG_ON will happen.

Re: [PATCH nf 5/5] netfilter: nfnl_cthelper: fix a race when walk the nf_ct_helper_hash table

[Liping Zhang]

2017-03-21 18:33 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> On Sun, Mar 19, 2017 at 10:36:02PM +0800, Liping Zhang wrote:
>> From: Liping Zhang <zlpnobody@gmail.com>
>>
>> The nf_ct_helper_hash table is protected by nf_ct_helper_mutex, while
>> nfct_helper operation is protected by nfnl_lock(NFNL_SUBSYS_CTHELPER).
>> So it's possible that one CPU is walking the nf_ct_helper_hash for
>> cthelper add/get/del, another cpu is doing nf_conntrack_helpers_unregister
>> at the same time. This is dangrous, and may cause use after free error.
>>
>> Note, delete operation will flush all cthelpers added via nfnetlink, so
>> using rcu to do protect is not easy.
>>
>> Now introduce a dummy list to record all the cthelpers added via
>> nfnetlink, then we can walk the dummy list instead of walking the
>> nf_ct_helper_hash. Also, keep nfnl_cthelper_dump_table unchanged, it
>> may be invoked without nfnl_lock(NFNL_SUBSYS_CTHELPER) held.
>>
>> Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
>> ---
>>  net/netfilter/nfnetlink_cthelper.c | 176 +++++++++++++++++++------------------
>>  1 file changed, 89 insertions(+), 87 deletions(-)
>>
>> diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
>> index fc4733f..47424ec 100644
>> --- a/net/netfilter/nfnetlink_cthelper.c
>> +++ b/net/netfilter/nfnetlink_cthelper.c
>> @@ -32,6 +32,13 @@ MODULE_LICENSE("GPL");
>>  MODULE_AUTHOR("Pablo Neira Ayuso <pablo@netfilter.org>");
>>  MODULE_DESCRIPTION("nfnl_cthelper: User-space connection tracking helpers");
>>
>> +struct nfnl_cthelper {
>> +     struct list_head                list;
>> +     struct nf_conntrack_helper      *helper;
>> +};
>> +
>> +static LIST_HEAD(nfnl_cthelper_list);
>
> We need a field possible_net_t so we can store what netns this helper
> belongs to, thus in case of flush command, we just remove the helpers
> that this netns owns.

Yes, good point, I will send v2.

Thanks Pablo.

Re: [PATCH nf 2/5] netfilter: nfnl_cthelper: fix incorrect helper->expect_class_max

[Pablo Neira Ayuso]

On Tue, Mar 21, 2017 at 10:35:43PM +0800, Liping Zhang wrote:
> Hi Pablo,
> 
> 2017-03-21 18:27 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> [...]
> >> +     class_max = ntohl(nla_get_be32(tb[NFCTH_POLICY_SET_NUM]));
> >> +     if (class_max == 0)
> >> +             return -EINVAL;
> >
> > I think this patch is just fixing up this case. We should always
> > provide a policy for the expectation.
> 
> No, see comments below.
> 
> >
> >> +     if (class_max > NF_CT_MAX_EXPECT_CLASSES)
> >>               return -EOVERFLOW;
> >>
> >>       expect_policy = kzalloc(sizeof(struct nf_conntrack_expect_policy) *
> >> -                             helper->expect_class_max, GFP_KERNEL);
> >> +                             class_max, GFP_KERNEL);
> >>       if (expect_policy == NULL)
> >>               return -ENOMEM;
> >>
> >> -     for (i=0; i<helper->expect_class_max; i++) {
> >> +     for (i = 0; i < class_max; i++) {
> >>               if (!tb[NFCTH_POLICY_SET+i])
> >>                       goto err;
> >>
> >> @@ -191,6 +191,8 @@ nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
> >>               if (ret < 0)
> >>                       goto err;
> >>       }
> >> +
> >> +     helper->expect_class_max = class_max - 1;
> >
> > Why - 1 ?
> >
> > class_max represents the array size, you can just keep this code the
> > way it is.
> 
> Actually, expect_class_max represents the array size minus 1.
> 
> For sip, expect_class_max is set to SIP_EXPECT_MAX(3). For ftp,
> expect_class_max is set to 0.
> 
> Also there's a "BUG_ON(me->expect_class_max >= NF_CT_MAX_EXPECT_CLASSES);"
> in nf_conntrack_helper_register, so if we supply 4 expect_policys,
> i.e. expect_class_max is 4, BUG_ON will happen.

Right, from the kernel side, all helpers assume that
me->expect_class_max represents the array size - 1.

While cthelper takes this as the array size.

This expect_class_max semantics is counterintuitive to me.

Applied, thanks.

P.S: I'm going to send a new version of:

http://patchwork.ozlabs.org/patch/741528/

that applies on top of this one.

Re: [PATCH nf 3/5] netfilter: drop const qualifier from struct nf_conntrack_expect_policy

[Liping Zhang]

Hi Pablo,

2017-03-21 18:34 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> On Sun, Mar 19, 2017 at 10:36:00PM +0800, Liping Zhang wrote:
>> From: Liping Zhang <zlpnobody@gmail.com>
>>
>> So we can modify the nf_conntrack_expect_policy directly, the next patch
>> will need this.
>
> I'm missing why you need this.

Because I want to modify the nf_conntrack_expect_policy directly when
do update expect policy in nfct_helper.

I saw you make a cast type to do this:
  policy = (struct nf_conntrack_expect_policy *) &helper->expect_policy[i];
(refer to http://patchwork.ozlabs.org/patch/741528/)

But when I'm working on this, I'm not sure "cast type" is proper or not.
So I just remove the const qualifier.

Re: [PATCH nf 3/5] netfilter: drop const qualifier from struct nf_conntrack_expect_policy

[Pablo Neira Ayuso]

On Tue, Mar 21, 2017 at 11:00:02PM +0800, Liping Zhang wrote:
> Hi Pablo,
> 
> 2017-03-21 18:34 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> > On Sun, Mar 19, 2017 at 10:36:00PM +0800, Liping Zhang wrote:
> >> From: Liping Zhang <zlpnobody@gmail.com>
> >>
> >> So we can modify the nf_conntrack_expect_policy directly, the next patch
> >> will need this.
> >
> > I'm missing why you need this.
> 
> Because I want to modify the nf_conntrack_expect_policy directly when
> do update expect policy in nfct_helper.
> 
> I saw you make a cast type to do this:
>   policy = (struct nf_conntrack_expect_policy *) &helper->expect_policy[i];
> (refer to http://patchwork.ozlabs.org/patch/741528/)
> 
> But when I'm working on this, I'm not sure "cast type" is proper or not.
> So I just remove the const qualifier.

I see. I prefer we just cast away the type from the single spot in
nfnl_cthelper.

Re: [PATCH nf 5/5] netfilter: nfnl_cthelper: fix a race when walk the nf_ct_helper_hash table

[Liping Zhang]

Hi Pablo,

2017-03-21 22:48 GMT+08:00 Liping Zhang <zlpnobody@gmail.com>:
> 2017-03-21 18:33 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
>>> +struct nfnl_cthelper {
>>> +     struct list_head                list;
>>> +     struct nf_conntrack_helper      *helper;
>>> +};
>>> +
>>> +static LIST_HEAD(nfnl_cthelper_list);
>>
>> We need a field possible_net_t so we can store what netns this helper
>> belongs to, thus in case of flush command, we just remove the helpers
>> that this netns owns.

After I have a closer look, I find that we do not support netns for the
nfct_helper currently. So this possible_net_t field is not necessary for
the time being.

I have a quick glance look, supporting netns for helper need a lot works
to do. We need to both change the nfnetlink_cthelper, nf_conntrack_help
and so on.

But if you think it's worth to support netns for cthelper, I can finish it in my
spare time:)

>
> Yes, good point, I will send v2.
>
> Thanks Pablo.

Re: [PATCH nf 5/5] netfilter: nfnl_cthelper: fix a race when walk the nf_ct_helper_hash table

[Pablo Neira Ayuso]

On Tue, Mar 21, 2017 at 11:19:11PM +0800, Liping Zhang wrote:
> Hi Pablo,
> 
> 2017-03-21 22:48 GMT+08:00 Liping Zhang <zlpnobody@gmail.com>:
> > 2017-03-21 18:33 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> >>> +struct nfnl_cthelper {
> >>> +     struct list_head                list;
> >>> +     struct nf_conntrack_helper      *helper;
> >>> +};
> >>> +
> >>> +static LIST_HEAD(nfnl_cthelper_list);
> >>
> >> We need a field possible_net_t so we can store what netns this helper
> >> belongs to, thus in case of flush command, we just remove the helpers
> >> that this netns owns.
> 
> After I have a closer look, I find that we do not support netns for the
> nfct_helper currently. So this possible_net_t field is not necessary for
> the time being.

Oh, I see. This is probably one of the remaining subsystems not having
netns support.

> I have a quick glance look, supporting netns for helper need a lot works
> to do. We need to both change the nfnetlink_cthelper, nf_conntrack_help
> and so on.
> 
> But if you think it's worth to support netns for cthelper, I can
> finish it in my spare time:)

Let's focus on fixing up the existing issues. It would be great if you
can add that later on, once changes in nf.git propagate to
nf-next.git.

BTW, let me also pushed out what I have here into nf.git. I'd
appreciate if you can rebase this 5/5 patch on top of it.

Thanks!

Re: [PATCH nf 5/5] netfilter: nfnl_cthelper: fix a race when walk the nf_ct_helper_hash table

[Liping Zhang]

2017-03-21 23:26 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
[...]
>>
>> After I have a closer look, I find that we do not support netns for the
>> nfct_helper currently. So this possible_net_t field is not necessary for
>> the time being.
>
> Oh, I see. This is probably one of the remaining subsystems not having
> netns support.
>
>> I have a quick glance look, supporting netns for helper need a lot works
>> to do. We need to both change the nfnetlink_cthelper, nf_conntrack_help
>> and so on.
>>
>> But if you think it's worth to support netns for cthelper, I can
>> finish it in my spare time:)
>
> Let's focus on fixing up the existing issues. It would be great if you
> can add that later on, once changes in nf.git propagate to
> nf-next.git.

OK, I will keep follow up on it.

> BTW, let me also pushed out what I have here into nf.git. I'd
> appreciate if you can rebase this 5/5 patch on top of it.

No problem!