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

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

[pablo]

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

[PATCH 1/4] netfilter: ipset: fix timeout value overflow bug

[pablo]

From: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>

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 <kadlec@blackhole.kfki.hu>
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

[PATCH 2/4] netfilter: ipset: fix hash size checking in kernel

[pablo]

From: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>

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 <kadlec@blackhole.kfki.hu>
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

[PATCH 3/4] netfilter: xt_CT: remove redundant header include

[pablo]

From: Eldad Zack <eldad@fogrefinery.com>

nf_conntrack_l4proto.h is included twice.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
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

[PATCH 4/4] netfilter: nf_ct_h323: fix usage of MODULE_ALIAS_NFCT_HELPER

[pablo]

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

RE: [PATCH 1/4] netfilter: ipset: fix timeout value overflow bug

[David Laight]

 
> --- 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 ?

	David
 

Re: [PATCH 1/4] netfilter: ipset: fix timeout value overflow bug

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 782 bytes --]

On Mon, May 14, 2012 at 03:19:49PM +0100, David Laight wrote:
>  
> > --- 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 ?

[-- Attachment #2: 0001-netfilter-ipset-fix-timeout-value-overflow-bug.patch --]
[-- Type: text/x-diff, Size: 2645 bytes --]

>From bcb0e955ae5ea5acb1b59fb59e4fcb1c8364994d Mon Sep 17 00:00:00 2001
From: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Date: Mon, 7 May 2012 02:35:44 +0000
Subject: [PATCH] netfilter: ipset: fix timeout value overflow bug

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

[ This patch was mangled by Pablo Neira Ayuso since David Laight notices
  that we were using hardcode 1000 instead of HZ to calculate the timeout ]

Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
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..40a85b1 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/HZ)
+		timeout = UINT_MAX/HZ;
+
 	/* 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..15275e9 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/HZ)
+		add_opt.timeout = UINT_MAX/HZ;
 	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

Re: [PATCH 1/4] netfilter: ipset: fix timeout value overflow bug

[Eric Dumazet]

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:
> >  
> > > --- 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)

Re: [PATCH 1/4] netfilter: ipset: fix timeout value overflow bug

[Jozsef Kadlecsik]

On Mon, 14 May 2012, Eric Dumazet wrote:

> 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:
> > >  
> > > > --- 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

Re: [PATCH 1/4] netfilter: ipset: fix timeout value overflow bug

[Pablo Neira Ayuso]

On Mon, May 14, 2012 at 07:45:07PM +0200, Jozsef Kadlecsik wrote:
> On Mon, 14 May 2012, Eric Dumazet wrote:
> 
> > 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:
> > > >  
> > > > > --- 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.

Re: [PATCH 1/4] netfilter: ipset: fix timeout value overflow bug

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1062 bytes --]

On Mon, May 14, 2012 at 07:45:07PM +0200, Jozsef Kadlecsik wrote:
> On Mon, 14 May 2012, Eric Dumazet wrote:
> 
> > 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:
> > > >  
> > > > > --- 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.

[-- Attachment #2: 0001-netfilter-ipset-fix-timeout-value-overflow-bug.patch --]
[-- Type: text/x-diff, Size: 2715 bytes --]

>From 9fdc90b2d00ecf98797c147d58cfe324fc92c9ed Mon Sep 17 00:00:00 2001
From: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Date: Mon, 7 May 2012 02:35:44 +0000
Subject: [PATCH] netfilter: ipset: fix timeout value overflow bug

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

[ This patch was mangled by Pablo Neira Ayuso since David Laight and
  Eric Dumazet noticed that we were using hardcoded 1000 instead of
  MSEC_PER_SEC to calculate the timeout ]

Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
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..41d9cfa 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/MSEC_PER_SEC)
+		timeout = UINT_MAX/MSEC_PER_SEC;
+
 	/* 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..035960e 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/MSEC_PER_SEC)
+		add_opt.timeout = UINT_MAX/MSEC_PER_SEC;
 	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

Re: [PATCH 1/4] netfilter: ipset: fix timeout value overflow bug

[Jozsef Kadlecsik]

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

> On Mon, May 14, 2012 at 07:45:07PM +0200, Jozsef Kadlecsik wrote:
> > On Mon, 14 May 2012, Eric Dumazet wrote:
> > 
> > > 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:
> > > > >  
> > > > > > --- 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

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

[David Miller]

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.

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

[Pablo Neira Ayuso]

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
> 
> > * 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.

RE: [PATCH 1/4] netfilter: ipset: fix timeout value overflow bug

[David Laight]

 

> -----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:
> > On Mon, May 14, 2012 at 03:19:49PM +0100, David Laight wrote:
> > >  
> > > > --- 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

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

[Jozsef Kadlecsik]

Hi Dave,

On Mon, 14 May 2012, David Miller wrote:

> 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.

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

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

[David Miller]

From: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
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.

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

[Jozsef Kadlecsik]

On Wed, 16 May 2012, David Miller wrote:

> > 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

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

[David Miller]

From: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Date: Wed, 16 May 2012 21:34:55 +0200 (CEST)

> On Wed, 16 May 2012, David Miller wrote:
> 
>> > 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'.

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

[Jozsef Kadlecsik]

On Wed, 16 May 2012, David Miller wrote:

> From: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
> Date: Wed, 16 May 2012 21:34:55 +0200 (CEST)
> 
> > On Wed, 16 May 2012, David Miller wrote:
> > 
> >> > 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