netfilter 00/07: netfilter fixes

netfilter 00/07: netfilter fixes

[Patrick McHardy]

Hi Dave,

the following patches fix a number of netfilter bugs:

- a conntrack race condition in death_by_timeout() when moving dying entries
  to the dying list. Fix from Eric Dumazet.

- a conntrack confirmation race condition that might lead to new conntrack
  entries becoming visible before they are fully set up.

- a conntrack lookup race condition that might lead to deleted conntrack
  entries being returned

- some sparse endianess fixes

- a fix for the nf_log proc handler, which is accessing userspace memory directly

- an incomplete initialization in the quota match, fix from Jan Engelhardt

- an incorrect comparison in the rateest match

Please apply or pull from:

git://git.kernel.org/pub/scm/linux/kernel/git/kaber/nf-next-2.6.git master

Thanks!


 net/netfilter/nf_conntrack_core.c |   25 ++++++++++++++++++++-----
 net/netfilter/nf_log.c            |   16 +++++++++++-----
 net/netfilter/xt_NFQUEUE.c        |    8 ++++----
 net/netfilter/xt_cluster.c        |    8 ++++----
 net/netfilter/xt_quota.c          |    1 +
 net/netfilter/xt_rateest.c        |    2 +-
 6 files changed, 41 insertions(+), 19 deletions(-)

Eric Dumazet (1):
      netfilter: nf_conntrack: death_by_timeout() fix

Jan Engelhardt (1):
      netfilter: xt_quota: fix incomplete initialization

Patrick McHardy (5):
      netfilter: nf_conntrack: fix confirmation race condition
      netfilter: nf_conntrack: fix conntrack lookup race
      netfilter: fix some sparse endianess warnings
      netfilter: nf_log: fix direct userspace memory access in proc handler
      netfilter: xt_rateest: fix comparison with self

netfilter 01/07: nf_conntrack: death_by_timeout() fix

[Patrick McHardy]

commit 8cc20198cfccd06cef705c14fd50bde603e2e306
Author: Eric Dumazet <eric.dumazet@gmail.com>
Date:   Mon Jun 22 14:13:55 2009 +0200

    netfilter: nf_conntrack: death_by_timeout() fix
    
    death_by_timeout() might delete a conntrack from hash list
    and insert it in dying list.
    
     nf_ct_delete_from_lists(ct);
     nf_ct_insert_dying_list(ct);
    
    I believe a (lockless) reader could *catch* ct while doing a lookup
    and miss the end of its chain.
    (nulls lookup algo must check the null value at the end of lookup and
    should restart if the null value is not the expected one.
    cf Documentation/RCU/rculist_nulls.txt for details)
    
    We need to change nf_conntrack_init_net() and use a different "null" value,
    guaranteed not being used in regular lists. Choose very large values, since
    hash table uses [0..size-1] null values.
    
    Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
    Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 5f72b94..5276a2d 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1267,13 +1267,19 @@ err_cache:
 	return ret;
 }
 
+/*
+ * We need to use special "null" values, not used in hash table
+ */
+#define UNCONFIRMED_NULLS_VAL	((1<<30)+0)
+#define DYING_NULLS_VAL		((1<<30)+1)
+
 static int nf_conntrack_init_net(struct net *net)
 {
 	int ret;
 
 	atomic_set(&net->ct.count, 0);
-	INIT_HLIST_NULLS_HEAD(&net->ct.unconfirmed, 0);
-	INIT_HLIST_NULLS_HEAD(&net->ct.dying, 0);
+	INIT_HLIST_NULLS_HEAD(&net->ct.unconfirmed, UNCONFIRMED_NULLS_VAL);
+	INIT_HLIST_NULLS_HEAD(&net->ct.dying, DYING_NULLS_VAL);
 	net->ct.stat = alloc_percpu(struct ip_conntrack_stat);
 	if (!net->ct.stat) {
 		ret = -ENOMEM;

netfilter 02/07: nf_conntrack: fix confirmation race condition

[Patrick McHardy]

commit 5c8ec910e789a92229978d8fd1fce7b62e8ac711
Author: Patrick McHardy <kaber@trash.net>
Date:   Mon Jun 22 14:14:16 2009 +0200

    netfilter: nf_conntrack: fix confirmation race condition
    
    New connection tracking entries are inserted into the hash before they
    are fully set up, namely the CONFIRMED bit is not set and the timer not
    started yet. This can theoretically lead to a race with timer, which
    would set the timeout value to a relative value, most likely already in
    the past.
    
    Perform hash insertion as the final step to fix this.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 5276a2d..b0b06c7 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -425,7 +425,6 @@ __nf_conntrack_confirm(struct sk_buff *skb)
 	/* Remove from unconfirmed list */
 	hlist_nulls_del_rcu(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode);
 
-	__nf_conntrack_hash_insert(ct, hash, repl_hash);
 	/* Timer relative to confirmation time, not original
 	   setting time, otherwise we'd get timer wrap in
 	   weird delay cases. */
@@ -433,8 +432,16 @@ __nf_conntrack_confirm(struct sk_buff *skb)
 	add_timer(&ct->timeout);
 	atomic_inc(&ct->ct_general.use);
 	set_bit(IPS_CONFIRMED_BIT, &ct->status);
+
+	/* Since the lookup is lockless, hash insertion must be done after
+	 * starting the timer and setting the CONFIRMED bit. The RCU barriers
+	 * guarantee that no other CPU can find the conntrack before the above
+	 * stores are visible.
+	 */
+	__nf_conntrack_hash_insert(ct, hash, repl_hash);
 	NF_CT_STAT_INC(net, insert);
 	spin_unlock_bh(&nf_conntrack_lock);
+
 	help = nfct_help(ct);
 	if (help && help->helper)
 		nf_conntrack_event_cache(IPCT_HELPER, ct);

netfilter 03/07: nf_conntrack: fix conntrack lookup race

[Patrick McHardy]

commit 8d8890b7751387f58ce0a6428773de2fbc0fd596
Author: Patrick McHardy <kaber@trash.net>
Date:   Mon Jun 22 14:14:41 2009 +0200

    netfilter: nf_conntrack: fix conntrack lookup race
    
    The RCU protected conntrack hash lookup only checks whether the entry
    has a refcount of zero to decide whether it is stale. This is not
    sufficient, entries are explicitly removed while there is at least
    one reference left, possibly more. Explicitly check whether the entry
    has been marked as dying to fix this.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index b0b06c7..7508f11 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -335,7 +335,8 @@ begin:
 	h = __nf_conntrack_find(net, tuple);
 	if (h) {
 		ct = nf_ct_tuplehash_to_ctrack(h);
-		if (unlikely(!atomic_inc_not_zero(&ct->ct_general.use)))
+		if (unlikely(nf_ct_is_dying(ct) ||
+			     !atomic_inc_not_zero(&ct->ct_general.use)))
 			h = NULL;
 		else {
 			if (unlikely(!nf_ct_tuple_equal(tuple, &h->tuple))) {
@@ -510,7 +511,8 @@ static noinline int early_drop(struct net *net, unsigned int hash)
 			cnt++;
 		}
 
-		if (ct && unlikely(!atomic_inc_not_zero(&ct->ct_general.use)))
+		if (ct && unlikely(nf_ct_is_dying(ct) ||
+				   !atomic_inc_not_zero(&ct->ct_general.use)))
 			ct = NULL;
 		if (ct || cnt >= NF_CT_EVICTION_RANGE)
 			break;

netfilter 04/07: fix some sparse endianess warnings

[Patrick McHardy]

commit f9ffc31251c2caa11962c9b74ce650e2167fa8d1
Author: Patrick McHardy <kaber@trash.net>
Date:   Mon Jun 22 14:15:02 2009 +0200

    netfilter: fix some sparse endianess warnings
    
    net/netfilter/xt_NFQUEUE.c:46:9: warning: incorrect type in assignment (different base types)
    net/netfilter/xt_NFQUEUE.c:46:9:    expected unsigned int [unsigned] [usertype] ipaddr
    net/netfilter/xt_NFQUEUE.c:46:9:    got restricted unsigned int
    net/netfilter/xt_NFQUEUE.c:68:10: warning: incorrect type in assignment (different base types)
    net/netfilter/xt_NFQUEUE.c:68:10:    expected unsigned int [unsigned] <noident>
    net/netfilter/xt_NFQUEUE.c:68:10:    got restricted unsigned int
    net/netfilter/xt_NFQUEUE.c:69:10: warning: incorrect type in assignment (different base types)
    net/netfilter/xt_NFQUEUE.c:69:10:    expected unsigned int [unsigned] <noident>
    net/netfilter/xt_NFQUEUE.c:69:10:    got restricted unsigned int
    net/netfilter/xt_NFQUEUE.c:70:10: warning: incorrect type in assignment (different base types)
    net/netfilter/xt_NFQUEUE.c:70:10:    expected unsigned int [unsigned] <noident>
    net/netfilter/xt_NFQUEUE.c:70:10:    got restricted unsigned int
    net/netfilter/xt_NFQUEUE.c:71:10: warning: incorrect type in assignment (different base types)
    net/netfilter/xt_NFQUEUE.c:71:10:    expected unsigned int [unsigned] <noident>
    net/netfilter/xt_NFQUEUE.c:71:10:    got restricted unsigned int
    
    net/netfilter/xt_cluster.c:20:55: warning: incorrect type in return expression (different base types)
    net/netfilter/xt_cluster.c:20:55:    expected unsigned int
    net/netfilter/xt_cluster.c:20:55:    got restricted unsigned int const [usertype] ip
    net/netfilter/xt_cluster.c:20:55: warning: incorrect type in return expression (different base types)
    net/netfilter/xt_cluster.c:20:55:    expected unsigned int
    net/netfilter/xt_cluster.c:20:55:    got restricted unsigned int const [usertype] ip
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/xt_NFQUEUE.c b/net/netfilter/xt_NFQUEUE.c
index 498b451..f28f6a5 100644
--- a/net/netfilter/xt_NFQUEUE.c
+++ b/net/netfilter/xt_NFQUEUE.c
@@ -40,12 +40,12 @@ nfqueue_tg(struct sk_buff *skb, const struct xt_target_param *par)
 static u32 hash_v4(const struct sk_buff *skb)
 {
 	const struct iphdr *iph = ip_hdr(skb);
-	u32 ipaddr;
+	__be32 ipaddr;
 
 	/* packets in either direction go into same queue */
 	ipaddr = iph->saddr ^ iph->daddr;
 
-	return jhash_2words(ipaddr, iph->protocol, jhash_initval);
+	return jhash_2words((__force u32)ipaddr, iph->protocol, jhash_initval);
 }
 
 static unsigned int
@@ -63,14 +63,14 @@ nfqueue_tg4_v1(struct sk_buff *skb, const struct xt_target_param *par)
 static u32 hash_v6(const struct sk_buff *skb)
 {
 	const struct ipv6hdr *ip6h = ipv6_hdr(skb);
-	u32 addr[4];
+	__be32 addr[4];
 
 	addr[0] = ip6h->saddr.s6_addr32[0] ^ ip6h->daddr.s6_addr32[0];
 	addr[1] = ip6h->saddr.s6_addr32[1] ^ ip6h->daddr.s6_addr32[1];
 	addr[2] = ip6h->saddr.s6_addr32[2] ^ ip6h->daddr.s6_addr32[2];
 	addr[3] = ip6h->saddr.s6_addr32[3] ^ ip6h->daddr.s6_addr32[3];
 
-	return jhash2(addr, ARRAY_SIZE(addr), jhash_initval);
+	return jhash2((__force u32 *)addr, ARRAY_SIZE(addr), jhash_initval);
 }
 
 static unsigned int
diff --git a/net/netfilter/xt_cluster.c b/net/netfilter/xt_cluster.c
index 69a639f..225ee3e 100644
--- a/net/netfilter/xt_cluster.c
+++ b/net/netfilter/xt_cluster.c
@@ -15,14 +15,14 @@
 #include <net/netfilter/nf_conntrack.h>
 #include <linux/netfilter/xt_cluster.h>
 
-static inline u_int32_t nf_ct_orig_ipv4_src(const struct nf_conn *ct)
+static inline u32 nf_ct_orig_ipv4_src(const struct nf_conn *ct)
 {
-	return ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u3.ip;
+	return (__force u32)ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u3.ip;
 }
 
-static inline const void *nf_ct_orig_ipv6_src(const struct nf_conn *ct)
+static inline const u32 *nf_ct_orig_ipv6_src(const struct nf_conn *ct)
 {
-	return ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u3.ip6;
+	return (__force u32 *)ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u3.ip6;
 }
 
 static inline u_int32_t

netfilter 05/07: nf_log: fix direct userspace memory access in proc handler

[Patrick McHardy]

commit 249556192859490b6280552d4b877064f9f5ee48
Author: Patrick McHardy <kaber@trash.net>
Date:   Mon Jun 22 14:15:30 2009 +0200

    netfilter: nf_log: fix direct userspace memory access in proc handler
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 2fefe14..4e62030 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -47,7 +47,6 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 	mutex_lock(&nf_log_mutex);
 
 	if (pf == NFPROTO_UNSPEC) {
-		int i;
 		for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++)
 			list_add_tail(&(logger->list[i]), &(nf_loggers_l[i]));
 	} else {
@@ -216,7 +215,7 @@ static const struct file_operations nflog_file_ops = {
 #endif /* PROC_FS */
 
 #ifdef CONFIG_SYSCTL
-struct ctl_path nf_log_sysctl_path[] = {
+static struct ctl_path nf_log_sysctl_path[] = {
 	{ .procname = "net", .ctl_name = CTL_NET, },
 	{ .procname = "netfilter", .ctl_name = NET_NETFILTER, },
 	{ .procname = "nf_log", .ctl_name = CTL_UNNUMBERED, },
@@ -228,19 +227,26 @@ static struct ctl_table nf_log_sysctl_table[NFPROTO_NUMPROTO+1];
 static struct ctl_table_header *nf_log_dir_header;
 
 static int nf_log_proc_dostring(ctl_table *table, int write, struct file *filp,
-			 void *buffer, size_t *lenp, loff_t *ppos)
+			 void __user *buffer, size_t *lenp, loff_t *ppos)
 {
 	const struct nf_logger *logger;
+	char buf[NFLOGGER_NAME_LEN];
+	size_t size = *lenp;
 	int r = 0;
 	int tindex = (unsigned long)table->extra1;
 
 	if (write) {
-		if (!strcmp(buffer, "NONE")) {
+		if (size > sizeof(buf))
+			size = sizeof(buf);
+		if (copy_from_user(buf, buffer, size))
+			return -EFAULT;
+
+		if (!strcmp(buf, "NONE")) {
 			nf_log_unbind_pf(tindex);
 			return 0;
 		}
 		mutex_lock(&nf_log_mutex);
-		logger = __find_logger(tindex, buffer);
+		logger = __find_logger(tindex, buf);
 		if (logger == NULL) {
 			mutex_unlock(&nf_log_mutex);
 			return -ENOENT;

netfilter 06/07: xt_quota: fix incomplete initialization

[Patrick McHardy]

commit 6d62182fea6cc6bbc8d82a691ad0608d68a54aeb
Author: Jan Engelhardt <jengelh@medozas.de>
Date:   Mon Jun 22 14:16:45 2009 +0200

    netfilter: xt_quota: fix incomplete initialization
    
    Commit v2.6.29-rc5-872-gacc738f ("xtables: avoid pointer to self")
    forgot to copy the initial quota value supplied by iptables into the
    private structure, thus counting from whatever was in the memory
    kmalloc returned.
    
    Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/xt_quota.c b/net/netfilter/xt_quota.c
index 01dd07b..98fc190 100644
--- a/net/netfilter/xt_quota.c
+++ b/net/netfilter/xt_quota.c
@@ -54,6 +54,7 @@ static bool quota_mt_check(const struct xt_mtchk_param *par)
 	if (q->master == NULL)
 		return -ENOMEM;
 
+	q->master->quota = q->quota;
 	return true;
 }
 

netfilter 07/07: xt_rateest: fix comparison with self

[Patrick McHardy]

commit 4d900f9df5f0569c2dc536701e2c11b6d50ebebf
Author: Patrick McHardy <kaber@trash.net>
Date:   Mon Jun 22 14:17:12 2009 +0200

    netfilter: xt_rateest: fix comparison with self
    
    As noticed by Török Edwin <edwintorok@gmail.com>:
    
    Compiling the kernel with clang has shown this warning:
    
    net/netfilter/xt_rateest.c:69:16: warning: self-comparison always results in a
    constant value
                            ret &= pps2 == pps2;
                                        ^
    Looking at the code:
    if (info->flags & XT_RATEEST_MATCH_BPS)
                ret &= bps1 == bps2;
            if (info->flags & XT_RATEEST_MATCH_PPS)
                ret &= pps2 == pps2;
    
    Judging from the MATCH_BPS case it seems to be a typo, with the intention of
    comparing pps1 with pps2.
    
    http://bugzilla.kernel.org/show_bug.cgi?id=13535
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/xt_rateest.c b/net/netfilter/xt_rateest.c
index 220a1d5..4fc6a91 100644
--- a/net/netfilter/xt_rateest.c
+++ b/net/netfilter/xt_rateest.c
@@ -66,7 +66,7 @@ xt_rateest_mt(const struct sk_buff *skb, const struct xt_match_param *par)
 		if (info->flags & XT_RATEEST_MATCH_BPS)
 			ret &= bps1 == bps2;
 		if (info->flags & XT_RATEEST_MATCH_PPS)
-			ret &= pps2 == pps2;
+			ret &= pps1 == pps2;
 		break;
 	}
 

Re: netfilter 00/07: netfilter fixes

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Mon, 22 Jun 2009 14:53:49 +0200 (MEST)

> the following patches fix a number of netfilter bugs:
 ...
> Please apply or pull from:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/kaber/nf-next-2.6.git master

Looks great, pulled, thanks a lot!