[PATCH 0/4] netfilter fixes for 3.4-rc7

From: Pablo Neira Ayuso <pablo@netfilter.org>

Hi David,

The following patchset contains several fixes for Netfilter 3.4-rc7:

* One fix for possible timeout overflow for ipset, from Jozsef
  Kadlecsik.

* One fix to ensure that hash size is correct, again for ipset
  from Jozsef Kadlecsik.

* Removal of redundant include in xt_CT from Eldad Zack.

* Fix for wrong usage of MODULE_ALIAS_NFCT_HELPER in nf_ct_h323
  helper from myself.

You can pull these changes from:

git://1984.lsi.us.es/net master

Thanks!

Eldad Zack (1):
  netfilter: xt_CT: remove redundant header include

Jozsef Kadlecsik (2):
  netfilter: ipset: fix timeout value overflow bug
  netfilter: ipset: fix hash size checking in kernel

Pablo Neira Ayuso (1):
  netfilter: nf_ct_h323: fix usage of MODULE_ALIAS_NFCT_HELPER

 include/linux/netfilter/ipset/ip_set_ahash.h   |   16 ++++++++++++++++
 include/linux/netfilter/ipset/ip_set_timeout.h |    4 ++++
 net/netfilter/ipset/ip_set_hash_ip.c           |   10 +++++++---
 net/netfilter/ipset/ip_set_hash_ipport.c       |   10 +++++++---
 net/netfilter/ipset/ip_set_hash_ipportip.c     |   10 +++++++---
 net/netfilter/ipset/ip_set_hash_ipportnet.c    |   10 +++++++---
 net/netfilter/ipset/ip_set_hash_net.c          |   10 +++++++---
 net/netfilter/ipset/ip_set_hash_netiface.c     |   10 +++++++---
 net/netfilter/ipset/ip_set_hash_netport.c      |   10 +++++++---
 net/netfilter/nf_conntrack_h323_main.c         |    4 +++-
 net/netfilter/xt_CT.c                          |    1 -
 net/netfilter/xt_set.c                         |   15 +++++++++++++--
 12 files changed, 85 insertions(+), 25 deletions(-)

-- 
1.7.10

From: Pablo Neira Ayuso <pablo@netfilter.org>

ctnetlink uses the aliases that are created by MODULE_ALIAS_NFCT_HELPER
to auto-load the module based on the helper name. Thus, we have to use
RAS, Q.931 and H.245, not H.323.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_conntrack_h323_main.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
index 722291f..b7bf187 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c

@@ -1833,4 +1833,6 @@ MODULE_AUTHOR("Jing Min Zhao <zhaojingmin@users.sourceforge.net>");
 MODULE_DESCRIPTION("H.323 connection tracking helper");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("ip_conntrack_h323");
-MODULE_ALIAS_NFCT_HELPER("h323");
+MODULE_ALIAS_NFCT_HELPER("RAS");
+MODULE_ALIAS_NFCT_HELPER("Q.931");
+MODULE_ALIAS_NFCT_HELPER("H.245");

-- 
1.7.10

From: Jozsef Kadlecsik <redacted>

Large timeout parameters could result wrong timeout values due to
an overflow at msec to jiffies conversion (reported by Andreas Herz)

Signed-off-by: Jozsef Kadlecsik <redacted>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/linux/netfilter/ipset/ip_set_timeout.h |    4 ++++
 net/netfilter/xt_set.c                         |   15 +++++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/include/linux/netfilter/ipset/ip_set_timeout.h b/include/linux/netfilter/ipset/ip_set_timeout.h
index 4792320..9fba34f 100644
--- a/include/linux/netfilter/ipset/ip_set_timeout.h
+++ b/include/linux/netfilter/ipset/ip_set_timeout.h

@@ -30,6 +30,10 @@ ip_set_timeout_uget(struct nlattr *tb)
 {
 	unsigned int timeout = ip_set_get_h32(tb);
 
+	/* Normalize to fit into jiffies */
+	if (timeout > UINT_MAX/1000)
+		timeout = UINT_MAX/1000;
+
 	/* Userspace supplied TIMEOUT parameter: adjust crazy size */
 	return timeout == IPSET_NO_TIMEOUT ? IPSET_NO_TIMEOUT - 1 : timeout;
 }

diff --git a/net/netfilter/xt_set.c b/net/netfilter/xt_set.c
index 0ec8138..e97a31b 100644
--- a/net/netfilter/xt_set.c
+++ b/net/netfilter/xt_set.c

@@ -44,6 +44,14 @@ const struct ip_set_adt_opt n = {	\
 	.cmdflags = cfs,		\
 	.timeout = t,			\
 }
+#define ADT_MOPT(n, f, d, fs, cfs, t)	\
+struct ip_set_adt_opt n = {		\
+	.family	= f,			\
+	.dim = d,			\
+	.flags = fs,			\
+	.cmdflags = cfs,		\
+	.timeout = t,			\
+}
 
 /* Revision 0 interface: backward compatible with netfilter/iptables */
 

@@ -296,11 +304,14 @@ static unsigned int
 set_target_v2(struct sk_buff *skb, const struct xt_action_param *par)
 {
 	const struct xt_set_info_target_v2 *info = par->targinfo;
-	ADT_OPT(add_opt, par->family, info->add_set.dim,
-		info->add_set.flags, info->flags, info->timeout);
+	ADT_MOPT(add_opt, par->family, info->add_set.dim,
+		 info->add_set.flags, info->flags, info->timeout);
 	ADT_OPT(del_opt, par->family, info->del_set.dim,
 		info->del_set.flags, 0, UINT_MAX);
 
+	/* Normalize to fit into jiffies */
+	if (add_opt.timeout > UINT_MAX/1000)
+		add_opt.timeout = UINT_MAX/1000;
 	if (info->add_set.index != IPSET_INVALID_ID)
 		ip_set_add(info->add_set.index, skb, par, &add_opt);
 	if (info->del_set.index != IPSET_INVALID_ID)

-- 
1.7.10

From: Eldad Zack <redacted>

nf_conntrack_l4proto.h is included twice.

Signed-off-by: Eldad Zack <redacted>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/xt_CT.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
index 3746d8b..a51de9b 100644
--- a/net/netfilter/xt_CT.c
+++ b/net/netfilter/xt_CT.c

@@ -17,7 +17,6 @@
 #include <net/netfilter/nf_conntrack_l4proto.h>
 #include <net/netfilter/nf_conntrack_helper.h>
 #include <net/netfilter/nf_conntrack_ecache.h>
-#include <net/netfilter/nf_conntrack_l4proto.h>
 #include <net/netfilter/nf_conntrack_timeout.h>
 #include <net/netfilter/nf_conntrack_zones.h>
 

-- 
1.7.10

From: Jozsef Kadlecsik <redacted>

The hash size must fit both into u32 (jhash) and the max value of
size_t. The missing checking could lead to kernel crash, bug reported
by Seblu.

Signed-off-by: Jozsef Kadlecsik <redacted>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/linux/netfilter/ipset/ip_set_ahash.h |   16 ++++++++++++++++
 net/netfilter/ipset/ip_set_hash_ip.c         |   10 +++++++---
 net/netfilter/ipset/ip_set_hash_ipport.c     |   10 +++++++---
 net/netfilter/ipset/ip_set_hash_ipportip.c   |   10 +++++++---
 net/netfilter/ipset/ip_set_hash_ipportnet.c  |   10 +++++++---
 net/netfilter/ipset/ip_set_hash_net.c        |   10 +++++++---
 net/netfilter/ipset/ip_set_hash_netiface.c   |   10 +++++++---
 net/netfilter/ipset/ip_set_hash_netport.c    |   10 +++++++---
 8 files changed, 65 insertions(+), 21 deletions(-)

diff --git a/include/linux/netfilter/ipset/ip_set_ahash.h b/include/linux/netfilter/ipset/ip_set_ahash.h
index 05a5d72..230a290 100644
--- a/include/linux/netfilter/ipset/ip_set_ahash.h
+++ b/include/linux/netfilter/ipset/ip_set_ahash.h

@@ -99,6 +99,22 @@ struct ip_set_hash {
 #endif
 };
 
+static size_t
+htable_size(u8 hbits)
+{
+	size_t hsize;
+
+	/* We must fit both into u32 in jhash and size_t */
+	if (hbits > 31)
+		return 0;
+	hsize = jhash_size(hbits);
+	if ((((size_t)-1) - sizeof(struct htable))/sizeof(struct hbucket)
+	    < hsize)
+		return 0;
+
+	return hsize * sizeof(struct hbucket) + sizeof(struct htable);
+}
+
 /* Compute htable_bits from the user input parameter hashsize */
 static u8
 htable_bits(u32 hashsize)

diff --git a/net/netfilter/ipset/ip_set_hash_ip.c b/net/netfilter/ipset/ip_set_hash_ip.c
index 5139dea..828ce46 100644
--- a/net/netfilter/ipset/ip_set_hash_ip.c
+++ b/net/netfilter/ipset/ip_set_hash_ip.c

@@ -364,6 +364,7 @@ hash_ip_create(struct ip_set *set, struct nlattr *tb[], u32 flags)
 {
 	u32 hashsize = IPSET_DEFAULT_HASHSIZE, maxelem = IPSET_DEFAULT_MAXELEM;
 	u8 netmask, hbits;
+	size_t hsize;
 	struct ip_set_hash *h;
 
 	if (!(set->family == NFPROTO_IPV4 || set->family == NFPROTO_IPV6))

@@ -405,9 +406,12 @@ hash_ip_create(struct ip_set *set, struct nlattr *tb[], u32 flags)
 	h->timeout = IPSET_NO_TIMEOUT;
 
 	hbits = htable_bits(hashsize);
-	h->table = ip_set_alloc(
-			sizeof(struct htable)
-			+ jhash_size(hbits) * sizeof(struct hbucket));
+	hsize = htable_size(hbits);
+	if (hsize == 0) {
+		kfree(h);
+		return -ENOMEM;
+	}
+	h->table = ip_set_alloc(hsize);
 	if (!h->table) {
 		kfree(h);
 		return -ENOMEM;

diff --git a/net/netfilter/ipset/ip_set_hash_ipport.c b/net/netfilter/ipset/ip_set_hash_ipport.c
index 9c27e24..e8dbb49 100644
--- a/net/netfilter/ipset/ip_set_hash_ipport.c
+++ b/net/netfilter/ipset/ip_set_hash_ipport.c

@@ -449,6 +449,7 @@ hash_ipport_create(struct ip_set *set, struct nlattr *tb[], u32 flags)
 	struct ip_set_hash *h;
 	u32 hashsize = IPSET_DEFAULT_HASHSIZE, maxelem = IPSET_DEFAULT_MAXELEM;
 	u8 hbits;
+	size_t hsize;
 
 	if (!(set->family == NFPROTO_IPV4 || set->family == NFPROTO_IPV6))
 		return -IPSET_ERR_INVALID_FAMILY;

@@ -476,9 +477,12 @@ hash_ipport_create(struct ip_set *set, struct nlattr *tb[], u32 flags)
 	h->timeout = IPSET_NO_TIMEOUT;
 
 	hbits = htable_bits(hashsize);
-	h->table = ip_set_alloc(
-			sizeof(struct htable)
-			+ jhash_size(hbits) * sizeof(struct hbucket));
+	hsize = htable_size(hbits);
+	if (hsize == 0) {
+		kfree(h);
+		return -ENOMEM;
+	}
+	h->table = ip_set_alloc(hsize);
 	if (!h->table) {
 		kfree(h);
 		return -ENOMEM;

diff --git a/net/netfilter/ipset/ip_set_hash_ipportip.c b/net/netfilter/ipset/ip_set_hash_ipportip.c
index 9134057..52f79d8 100644
--- a/net/netfilter/ipset/ip_set_hash_ipportip.c
+++ b/net/netfilter/ipset/ip_set_hash_ipportip.c

@@ -467,6 +467,7 @@ hash_ipportip_create(struct ip_set *set, struct nlattr *tb[], u32 flags)
 	struct ip_set_hash *h;
 	u32 hashsize = IPSET_DEFAULT_HASHSIZE, maxelem = IPSET_DEFAULT_MAXELEM;
 	u8 hbits;
+	size_t hsize;
 
 	if (!(set->family == NFPROTO_IPV4 || set->family == NFPROTO_IPV6))
 		return -IPSET_ERR_INVALID_FAMILY;

@@ -494,9 +495,12 @@ hash_ipportip_create(struct ip_set *set, struct nlattr *tb[], u32 flags)
 	h->timeout = IPSET_NO_TIMEOUT;
 
 	hbits = htable_bits(hashsize);
-	h->table = ip_set_alloc(
-			sizeof(struct htable)
-			+ jhash_size(hbits) * sizeof(struct hbucket));
+	hsize = htable_size(hbits);
+	if (hsize == 0) {
+		kfree(h);
+		return -ENOMEM;
+	}
+	h->table = ip_set_alloc(hsize);
 	if (!h->table) {
 		kfree(h);
 		return -ENOMEM;

diff --git a/net/netfilter/ipset/ip_set_hash_ipportnet.c b/net/netfilter/ipset/ip_set_hash_ipportnet.c
index 5d05e69..97583f5 100644
--- a/net/netfilter/ipset/ip_set_hash_ipportnet.c
+++ b/net/netfilter/ipset/ip_set_hash_ipportnet.c

@@ -616,6 +616,7 @@ hash_ipportnet_create(struct ip_set *set, struct nlattr *tb[], u32 flags)
 	struct ip_set_hash *h;
 	u32 hashsize = IPSET_DEFAULT_HASHSIZE, maxelem = IPSET_DEFAULT_MAXELEM;
 	u8 hbits;
+	size_t hsize;
 
 	if (!(set->family == NFPROTO_IPV4 || set->family == NFPROTO_IPV6))
 		return -IPSET_ERR_INVALID_FAMILY;

@@ -645,9 +646,12 @@ hash_ipportnet_create(struct ip_set *set, struct nlattr *tb[], u32 flags)
 	h->timeout = IPSET_NO_TIMEOUT;
 
 	hbits = htable_bits(hashsize);
-	h->table = ip_set_alloc(
-			sizeof(struct htable)
-			+ jhash_size(hbits) * sizeof(struct hbucket));
+	hsize = htable_size(hbits);
+	if (hsize == 0) {
+		kfree(h);
+		return -ENOMEM;
+	}
+	h->table = ip_set_alloc(hsize);
 	if (!h->table) {
 		kfree(h);
 		return -ENOMEM;

diff --git a/net/netfilter/ipset/ip_set_hash_net.c b/net/netfilter/ipset/ip_set_hash_net.c
index 7c3d945..1721cde 100644
--- a/net/netfilter/ipset/ip_set_hash_net.c
+++ b/net/netfilter/ipset/ip_set_hash_net.c

@@ -460,6 +460,7 @@ hash_net_create(struct ip_set *set, struct nlattr *tb[], u32 flags)
 	u32 hashsize = IPSET_DEFAULT_HASHSIZE, maxelem = IPSET_DEFAULT_MAXELEM;
 	struct ip_set_hash *h;
 	u8 hbits;
+	size_t hsize;
 
 	if (!(set->family == NFPROTO_IPV4 || set->family == NFPROTO_IPV6))
 		return -IPSET_ERR_INVALID_FAMILY;

@@ -489,9 +490,12 @@ hash_net_create(struct ip_set *set, struct nlattr *tb[], u32 flags)
 	h->timeout = IPSET_NO_TIMEOUT;
 
 	hbits = htable_bits(hashsize);
-	h->table = ip_set_alloc(
-			sizeof(struct htable)
-			+ jhash_size(hbits) * sizeof(struct hbucket));
+	hsize = htable_size(hbits);
+	if (hsize == 0) {
+		kfree(h);
+		return -ENOMEM;
+	}
+	h->table = ip_set_alloc(hsize);
 	if (!h->table) {
 		kfree(h);
 		return -ENOMEM;

diff --git a/net/netfilter/ipset/ip_set_hash_netiface.c b/net/netfilter/ipset/ip_set_hash_netiface.c
index f24037f..33bafc9 100644
--- a/net/netfilter/ipset/ip_set_hash_netiface.c
+++ b/net/netfilter/ipset/ip_set_hash_netiface.c

@@ -722,6 +722,7 @@ hash_netiface_create(struct ip_set *set, struct nlattr *tb[], u32 flags)
 	struct ip_set_hash *h;
 	u32 hashsize = IPSET_DEFAULT_HASHSIZE, maxelem = IPSET_DEFAULT_MAXELEM;
 	u8 hbits;
+	size_t hsize;
 
 	if (!(set->family == NFPROTO_IPV4 || set->family == NFPROTO_IPV6))
 		return -IPSET_ERR_INVALID_FAMILY;

@@ -752,9 +753,12 @@ hash_netiface_create(struct ip_set *set, struct nlattr *tb[], u32 flags)
 	h->ahash_max = AHASH_MAX_SIZE;
 
 	hbits = htable_bits(hashsize);
-	h->table = ip_set_alloc(
-			sizeof(struct htable)
-			+ jhash_size(hbits) * sizeof(struct hbucket));
+	hsize = htable_size(hbits);
+	if (hsize == 0) {
+		kfree(h);
+		return -ENOMEM;
+	}
+	h->table = ip_set_alloc(hsize);
 	if (!h->table) {
 		kfree(h);
 		return -ENOMEM;

diff --git a/net/netfilter/ipset/ip_set_hash_netport.c b/net/netfilter/ipset/ip_set_hash_netport.c
index ce2e771..3a5e198 100644
--- a/net/netfilter/ipset/ip_set_hash_netport.c
+++ b/net/netfilter/ipset/ip_set_hash_netport.c

@@ -572,6 +572,7 @@ hash_netport_create(struct ip_set *set, struct nlattr *tb[], u32 flags)
 	struct ip_set_hash *h;
 	u32 hashsize = IPSET_DEFAULT_HASHSIZE, maxelem = IPSET_DEFAULT_MAXELEM;
 	u8 hbits;
+	size_t hsize;
 
 	if (!(set->family == NFPROTO_IPV4 || set->family == NFPROTO_IPV6))
 		return -IPSET_ERR_INVALID_FAMILY;

@@ -601,9 +602,12 @@ hash_netport_create(struct ip_set *set, struct nlattr *tb[], u32 flags)
 	h->timeout = IPSET_NO_TIMEOUT;
 
 	hbits = htable_bits(hashsize);
-	h->table = ip_set_alloc(
-			sizeof(struct htable)
-			+ jhash_size(hbits) * sizeof(struct hbucket));
+	hsize = htable_size(hbits);
+	if (hsize == 0) {
+		kfree(h);
+		return -ENOMEM;
+	}
+	h->table = ip_set_alloc(hsize);
 	if (!h->table) {
 		kfree(h);
 		return -ENOMEM;

-- 
1.7.10

Doesn't that rather assume that HZ is 1000 ?

	David
 

On Mon, May 14, 2012 at 03:19:49PM +0100, David Laight wrote:

quoted

--- a/include/linux/netfilter/ipset/ip_set_timeout.h
+++ b/include/linux/netfilter/ipset/ip_set_timeout.h

@@ -30,6 +30,10 @@ ip_set_timeout_uget(struct nlattr *tb)
 {
 	unsigned int timeout = ip_set_get_h32(tb);
 
+	/* Normalize to fit into jiffies */
+	if (timeout > UINT_MAX/1000)
+		timeout = UINT_MAX/1000;
+

Doesn't that rather assume that HZ is 1000 ?

Indeed. I overlooked that. Thanks David.

New patch attached fixing this. I've rebased my tree.

@Jozsef: BTW, why do we have

include/linux/netfilter/ipset/ip_set_timeout.h

living under include/linux ?

All definitions are private to the kernel. Why not moving that header
(and other similar) to include/net ?

On Mon, 2012-05-14 at 16:36 +0200, Pablo Neira Ayuso wrote:

On Mon, May 14, 2012 at 03:19:49PM +0100, David Laight wrote:

quoted

quoted

--- a/include/linux/netfilter/ipset/ip_set_timeout.h
+++ b/include/linux/netfilter/ipset/ip_set_timeout.h

@@ -30,6 +30,10 @@ ip_set_timeout_uget(struct nlattr *tb)
 {
 	unsigned int timeout = ip_set_get_h32(tb);
 
+	/* Normalize to fit into jiffies */
+	if (timeout > UINT_MAX/1000)
+		timeout = UINT_MAX/1000;
+

Doesn't that rather assume that HZ is 1000 ?

Indeed. I overlooked that. Thanks David.

I dont think so.

1000 here is really MSEC_PER_SEC

(All occurrences should be changed in this file)

On Mon, 14 May 2012, Eric Dumazet wrote:

On Mon, 2012-05-14 at 16:36 +0200, Pablo Neira Ayuso wrote:

quoted

On Mon, May 14, 2012 at 03:19:49PM +0100, David Laight wrote:

quoted

quoted

--- a/include/linux/netfilter/ipset/ip_set_timeout.h
+++ b/include/linux/netfilter/ipset/ip_set_timeout.h

@@ -30,6 +30,10 @@ ip_set_timeout_uget(struct nlattr *tb)
 {
 	unsigned int timeout = ip_set_get_h32(tb);
 
+	/* Normalize to fit into jiffies */
+	if (timeout > UINT_MAX/1000)
+		timeout = UINT_MAX/1000;
+

Doesn't that rather assume that HZ is 1000 ?

Indeed. I overlooked that. Thanks David.

I dont think so.

1000 here is really MSEC_PER_SEC

(All occurrences should be changed in this file)

Yes, exactly. Pablo, shall I produce a little patch or could you change 
1000 to MSEC_PER_SEC?

Best regards, 
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

On Mon, May 14, 2012 at 07:45:07PM +0200, Jozsef Kadlecsik wrote:

On Mon, 14 May 2012, Eric Dumazet wrote:

quoted

On Mon, 2012-05-14 at 16:36 +0200, Pablo Neira Ayuso wrote:

quoted

On Mon, May 14, 2012 at 03:19:49PM +0100, David Laight wrote:

quoted

quoted

--- a/include/linux/netfilter/ipset/ip_set_timeout.h
+++ b/include/linux/netfilter/ipset/ip_set_timeout.h

@@ -30,6 +30,10 @@ ip_set_timeout_uget(struct nlattr *tb)
 {
 	unsigned int timeout = ip_set_get_h32(tb);
 
+	/* Normalize to fit into jiffies */
+	if (timeout > UINT_MAX/1000)
+		timeout = UINT_MAX/1000;
+

Doesn't that rather assume that HZ is 1000 ?

Indeed. I overlooked that. Thanks David.

I dont think so.

1000 here is really MSEC_PER_SEC

(All occurrences should be changed in this file)

Yes, exactly. Pablo, shall I produce a little patch or could you change 
1000 to MSEC_PER_SEC?

I'll mangle this myself. No need to send a new patch.

On Mon, May 14, 2012 at 07:45:07PM +0200, Jozsef Kadlecsik wrote:

On Mon, 14 May 2012, Eric Dumazet wrote:

quoted

On Mon, 2012-05-14 at 16:36 +0200, Pablo Neira Ayuso wrote:

quoted

On Mon, May 14, 2012 at 03:19:49PM +0100, David Laight wrote:

quoted

quoted

--- a/include/linux/netfilter/ipset/ip_set_timeout.h
+++ b/include/linux/netfilter/ipset/ip_set_timeout.h

@@ -30,6 +30,10 @@ ip_set_timeout_uget(struct nlattr *tb)
 {
 	unsigned int timeout = ip_set_get_h32(tb);
 
+	/* Normalize to fit into jiffies */
+	if (timeout > UINT_MAX/1000)
+		timeout = UINT_MAX/1000;
+

Doesn't that rather assume that HZ is 1000 ?

Indeed. I overlooked that. Thanks David.

I dont think so.

1000 here is really MSEC_PER_SEC

(All occurrences should be changed in this file)

Yes, exactly. Pablo, shall I produce a little patch or could you change 
1000 to MSEC_PER_SEC?

New patch attached.

I have rebase my tree again.

On Mon, 14 May 2012, Pablo Neira Ayuso wrote:

On Mon, May 14, 2012 at 07:45:07PM +0200, Jozsef Kadlecsik wrote:

quoted

On Mon, 14 May 2012, Eric Dumazet wrote:

quoted

On Mon, 2012-05-14 at 16:36 +0200, Pablo Neira Ayuso wrote:

quoted

On Mon, May 14, 2012 at 03:19:49PM +0100, David Laight wrote:

quoted

quoted

--- a/include/linux/netfilter/ipset/ip_set_timeout.h
+++ b/include/linux/netfilter/ipset/ip_set_timeout.h

@@ -30,6 +30,10 @@ ip_set_timeout_uget(struct nlattr *tb)
 {
 	unsigned int timeout = ip_set_get_h32(tb);
 
+	/* Normalize to fit into jiffies */
+	if (timeout > UINT_MAX/1000)
+		timeout = UINT_MAX/1000;
+

Doesn't that rather assume that HZ is 1000 ?

Indeed. I overlooked that. Thanks David.

I dont think so.

1000 here is really MSEC_PER_SEC

(All occurrences should be changed in this file)

Yes, exactly. Pablo, shall I produce a little patch or could you change 
1000 to MSEC_PER_SEC?

New patch attached.

I have rebase my tree again.

Thanks Pablo, indeed!

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

From: pablo@netfilter.org
Date: Mon, 14 May 2012 13:46:59 +0200

* One fix for possible timeout overflow for ipset, from Jozsef
  Kadlecsik.

* One fix to ensure that hash size is correct, again for ipset
  from Jozsef Kadlecsik.

* Removal of redundant include in xt_CT from Eldad Zack.

* Fix for wrong usage of MODULE_ALIAS_NFCT_HELPER in nf_ct_h323
  helper from myself.

I don't consider any of these appropriate this late in the -RC
series.

They don't fix major regressions seen by many users.

And the duplicate header include is extremely out-of-scope and
inappropriate.

Sorry.

On Mon, May 14, 2012 at 06:56:07PM -0400, David Miller wrote:

From: pablo@netfilter.org
Date: Mon, 14 May 2012 13:46:59 +0200

quoted

* One fix for possible timeout overflow for ipset, from Jozsef
  Kadlecsik.

* One fix to ensure that hash size is correct, again for ipset
  from Jozsef Kadlecsik.

* Removal of redundant include in xt_CT from Eldad Zack.

* Fix for wrong usage of MODULE_ALIAS_NFCT_HELPER in nf_ct_h323
  helper from myself.

I don't consider any of these appropriate this late in the -RC
series.

They don't fix major regressions seen by many users.

And the duplicate header include is extremely out-of-scope and
inappropriate.

Sorry.

No problem, I'll schedule those for net-next.

-----Original Message-----
From: Eric Dumazet [mailto:eric.dumazet@gmail.com] 
Sent: 14 May 2012 15:47
To: Pablo Neira Ayuso
Cc: David Laight; netfilter-devel@vger.kernel.org; 
davem@davemloft.net; netdev@vger.kernel.org; Jozsef Kadlecsik
Subject: Re: [PATCH 1/4] netfilter: ipset: fix timeout value 
overflow bug

On Mon, 2012-05-14 at 16:36 +0200, Pablo Neira Ayuso wrote:

quoted

On Mon, May 14, 2012 at 03:19:49PM +0100, David Laight wrote:

quoted

quoted

--- a/include/linux/netfilter/ipset/ip_set_timeout.h
+++ b/include/linux/netfilter/ipset/ip_set_timeout.h

@@ -30,6 +30,10 @@ ip_set_timeout_uget(struct nlattr *tb)
 {
 	unsigned int timeout = ip_set_get_h32(tb);
 
+	/* Normalize to fit into jiffies */
+	if (timeout > UINT_MAX/1000)
+		timeout = UINT_MAX/1000;
+

Doesn't that rather assume that HZ is 1000 ?

Indeed. I overlooked that. Thanks David.

I dont think so.

1000 here is really MSEC_PER_SEC

I was just reading the comment above - which seemed to imply
the purpose of the code was to ensure the timeout wouldn't
exceeded 2^32 jiffies.

I tend to use variable names for timeouts/timestamps that
include the units - can make it more obvious when a divisor
is absent - and makes it more obvious what the literal
constants are converying beween.
After all, the number of milliseconds in a second isn't
subject to change :-)

	David

Hi Dave,

On Mon, 14 May 2012, David Miller wrote:

From: pablo@netfilter.org
Date: Mon, 14 May 2012 13:46:59 +0200

quoted

* One fix for possible timeout overflow for ipset, from Jozsef
  Kadlecsik.

* One fix to ensure that hash size is correct, again for ipset
  from Jozsef Kadlecsik.

* Removal of redundant include in xt_CT from Eldad Zack.

* Fix for wrong usage of MODULE_ALIAS_NFCT_HELPER in nf_ct_h323
  helper from myself.

I don't consider any of these appropriate this late in the -RC
series.

They don't fix major regressions seen by many users.

Could at least the patch with the subject

   netfilter: ipset: fix hash size checking in kernel

   The hash size must fit both into u32 (jhash) and the max value of
   size_t. The missing checking could lead to kernel crash, bug reported
   by Seblu.

be submitted into 3.4-rc7? Any non most-recent ipset package compiled with 
gcc-4.7 or above can trigger the bug.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

From: Jozsef Kadlecsik <redacted>
Date: Wed, 16 May 2012 20:41:51 +0200 (CEST)

Could at least the patch with the subject

   netfilter: ipset: fix hash size checking in kernel

   The hash size must fit both into u32 (jhash) and the max value of
   size_t. The missing checking could lead to kernel crash, bug reported
   by Seblu.

be submitted into 3.4-rc7? Any non most-recent ipset package compiled with 
gcc-4.7 or above can trigger the bug.

And only root can trigger it if he gives bogus parameters right?

If that's the case, the exposure is to privileged users committing an
operator error, so I don't see it as so important.

On Wed, 16 May 2012, David Miller wrote:

quoted

Could at least the patch with the subject

   netfilter: ipset: fix hash size checking in kernel

   The hash size must fit both into u32 (jhash) and the max value of
   size_t. The missing checking could lead to kernel crash, bug reported
   by Seblu.

be submitted into 3.4-rc7? Any non most-recent ipset package compiled with 
gcc-4.7 or above can trigger the bug.

And only root can trigger it if he gives bogus parameters right?

Yes, root can trigger it only, however it's a two sided bug: ipset < 6.12 
compiled with gcc >= 4.7 is broken and sends bogus data to the kernel, 
even when normal input parameters are supplied. And unpatched kernel can 
crash when acting on the bogus data. :-(
 

If that's the case, the exposure is to privileged users committing an
operator error, so I don't see it as so important.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

From: Jozsef Kadlecsik <redacted>
Date: Wed, 16 May 2012 21:34:55 +0200 (CEST)

On Wed, 16 May 2012, David Miller wrote:

quoted

quoted

Could at least the patch with the subject

   netfilter: ipset: fix hash size checking in kernel

   The hash size must fit both into u32 (jhash) and the max value of
   size_t. The missing checking could lead to kernel crash, bug reported
   by Seblu.

be submitted into 3.4-rc7? Any non most-recent ipset package compiled with 
gcc-4.7 or above can trigger the bug.

And only root can trigger it if he gives bogus parameters right?

Yes, root can trigger it only, however it's a two sided bug: ipset < 6.12 
compiled with gcc >= 4.7 is broken and sends bogus data to the kernel, 
even when normal input parameters are supplied. And unpatched kernel can 
crash when acting on the bogus data. :-(

Ok, that's a more convincing argument.

I've applied the hash size validation patch to 'net'.

On Wed, 16 May 2012, David Miller wrote:

From: Jozsef Kadlecsik <redacted>
Date: Wed, 16 May 2012 21:34:55 +0200 (CEST)

quoted

On Wed, 16 May 2012, David Miller wrote:

quoted

quoted

Could at least the patch with the subject

   netfilter: ipset: fix hash size checking in kernel

   The hash size must fit both into u32 (jhash) and the max value of
   size_t. The missing checking could lead to kernel crash, bug reported
   by Seblu.

be submitted into 3.4-rc7? Any non most-recent ipset package compiled with 
gcc-4.7 or above can trigger the bug.

And only root can trigger it if he gives bogus parameters right?

Yes, root can trigger it only, however it's a two sided bug: ipset < 6.12 
compiled with gcc >= 4.7 is broken and sends bogus data to the kernel, 
even when normal input parameters are supplied. And unpatched kernel can 
crash when acting on the bogus data. :-(

Ok, that's a more convincing argument.

I've applied the hash size validation patch to 'net'.

Thank you indeed - I received multiple reports of the crash, so the 
conditions are not uncommon.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary