[PATCH 0/7] netfilter updates for net-next (batch 3)

[PATCH 0/7] netfilter updates for net-next (batch 3)

[pablo]

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

Hi David,

The following patchset contains small updates for net-next, more relevantly:

* One fix for potential NULL dereference in xt_HMARK by Dan Carpenter.

* Conversion to use _ALL macro in xt_hashlimit as you suggested by
  Florian Westphal.

* One fix for timeout overflow from Jozsef Kadlecsik.

* Replace usage of modulus for hash calculation in xt_HMARK as you suggested
  from myself.

You can pull these changes from:

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

Thanks!

Dan Carpenter (1):
  netfilter: xt_HMARK: potential NULL dereference in get_inner_hdr()

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

Florian Westphal (1):
  netfilter: xt_hashlimit: use _ALL macro to reject unknown flag bits

Jozsef Kadlecsik (1):
  netfilter: ipset: fix timeout value overflow bug

Pablo Neira Ayuso (3):
  netfilter: xt_HMARK: modulus is expensive for hash calculation
  netfilter: nf_ct_tcp: extend log message for invalid ignored packets
  netfilter: nf_ct_h323: fix usage of MODULE_ALIAS_NFCT_HELPER

 include/linux/netfilter/ipset/ip_set_timeout.h |    4 ++++
 include/linux/netfilter/xt_hashlimit.h         |    6 ++++--
 net/netfilter/nf_conntrack_h323_main.c         |    4 +++-
 net/netfilter/nf_conntrack_proto_tcp.c         |    3 ++-
 net/netfilter/xt_CT.c                          |    1 -
 net/netfilter/xt_HMARK.c                       |    4 ++--
 net/netfilter/xt_hashlimit.c                   |    2 +-
 net/netfilter/xt_set.c                         |   15 +++++++++++++--
 8 files changed, 29 insertions(+), 10 deletions(-)

-- 
1.7.10

[PATCH 1/7] netfilter: xt_hashlimit: use _ALL macro to reject unknown flag bits

[pablo]

From: Florian Westphal <fw@strlen.de>

David Miller says:
     The canonical way to validate if the set bits are in a valid
     range is to have a "_ALL" macro, and test:
     if (val & ~XT_HASHLIMIT_ALL)
         goto err;"

make it so.

Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/linux/netfilter/xt_hashlimit.h |    6 ++++--
 net/netfilter/xt_hashlimit.c           |    2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/linux/netfilter/xt_hashlimit.h b/include/linux/netfilter/xt_hashlimit.h
index 05fe799..c42e52f 100644
--- a/include/linux/netfilter/xt_hashlimit.h
+++ b/include/linux/netfilter/xt_hashlimit.h
@@ -22,10 +22,12 @@ enum {
 	XT_HASHLIMIT_HASH_SPT = 1 << 3,
 	XT_HASHLIMIT_INVERT   = 1 << 4,
 	XT_HASHLIMIT_BYTES    = 1 << 5,
+};
 #ifdef __KERNEL__
-	XT_HASHLIMIT_MAX      = 1 << 6,
+#define XT_HASHLIMIT_ALL (XT_HASHLIMIT_HASH_DIP | XT_HASHLIMIT_HASH_DPT | \
+			  XT_HASHLIMIT_HASH_SIP | XT_HASHLIMIT_HASH_SPT | \
+			  XT_HASHLIMIT_INVERT | XT_HASHLIMIT_BYTES)
 #endif
-};
 
 struct hashlimit_cfg {
 	__u32 mode;	  /* bitmask of XT_HASHLIMIT_HASH_* */
diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index 5d5af1d..26a668a 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -647,7 +647,7 @@ static int hashlimit_mt_check(const struct xt_mtchk_param *par)
 			return -EINVAL;
 	}
 
-	if (info->cfg.mode >= XT_HASHLIMIT_MAX) {
+	if (info->cfg.mode & ~XT_HASHLIMIT_ALL) {
 		pr_info("Unknown mode mask %X, kernel too old?\n",
 						info->cfg.mode);
 		return -EINVAL;
-- 
1.7.10

[PATCH 2/7] netfilter: xt_HMARK: potential NULL dereference in get_inner_hdr()

[pablo]

From: Dan Carpenter <dan.carpenter@oracle.com>

There is a typo in the error checking and "&&" was used instead of "||".
If skb_header_pointer() returns NULL then it leads to a NULL
dereference.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Hans Schillstrom <hans.schillstrom@ericsson.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/xt_HMARK.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/xt_HMARK.c b/net/netfilter/xt_HMARK.c
index 32fbd73..5817d03 100644
--- a/net/netfilter/xt_HMARK.c
+++ b/net/netfilter/xt_HMARK.c
@@ -223,7 +223,7 @@ static int get_inner_hdr(const struct sk_buff *skb, int iphsz, int *nhoff)
 
 	/* Not enough header? */
 	icmph = skb_header_pointer(skb, *nhoff + iphsz, sizeof(_ih), &_ih);
-	if (icmph == NULL && icmph->type > NR_ICMP_TYPES)
+	if (icmph == NULL || icmph->type > NR_ICMP_TYPES)
 		return 0;
 
 	/* Error message? */
-- 
1.7.10

[PATCH 3/7] netfilter: xt_HMARK: modulus is expensive for hash calculation

[pablo]

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

Use:

((u64)(HASH_VAL * HASH_SIZE)) >> 32

as suggested by David S. Miller.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/xt_HMARK.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/xt_HMARK.c b/net/netfilter/xt_HMARK.c
index 5817d03..0a96a43 100644
--- a/net/netfilter/xt_HMARK.c
+++ b/net/netfilter/xt_HMARK.c
@@ -109,7 +109,7 @@ hmark_hash(struct hmark_tuple *t, const struct xt_hmark_info *info)
 	hash = jhash_3words(t->src, t->dst, t->uports.v32, info->hashrnd);
 	hash = hash ^ (t->proto & info->proto_mask);
 
-	return (hash % info->hmodulus) + info->hoffset;
+	return (((u64)hash * info->hmodulus) >> 32) + info->hoffset;
 }
 
 static void
-- 
1.7.10

[PATCH 4/7] netfilter: nf_ct_tcp: extend log message for invalid ignored packets

[pablo]

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

Extend log message if packets are ignored to include the TCP state, ie.
replace:

[ 3968.070196] nf_ct_tcp: invalid packet ignored IN= OUT= SRC=...

by:

[ 3968.070196] nf_ct_tcp: invalid packet ignored in state ESTABLISHED IN= OUT= SRC=...

This information is useful to know in what state we were while ignoring the
packet.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Acked-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
---
 net/netfilter/nf_conntrack_proto_tcp.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 4dfbfa8..21ff1a9 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -952,7 +952,8 @@ static int tcp_packet(struct nf_conn *ct,
 		spin_unlock_bh(&ct->lock);
 		if (LOG_INVALID(net, IPPROTO_TCP))
 			nf_log_packet(pf, 0, skb, NULL, NULL, NULL,
-				  "nf_ct_tcp: invalid packet ignored ");
+				  "nf_ct_tcp: invalid packet ignored in "
+				  "state %s ", tcp_conntrack_names[old_state]);
 		return NF_ACCEPT;
 	case TCP_CONNTRACK_MAX:
 		/* Invalid packet */
-- 
1.7.10

[PATCH 5/7] 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)

[ 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

[PATCH 6/7] 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 7/7] 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 93c13eb..46d69d7 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c
@@ -1830,4 +1830,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 0/7] netfilter updates for net-next (batch 3)

[David Miller]

From: pablo@netfilter.org
Date: Thu, 17 May 2012 01:06:37 +0200

> The following patchset contains small updates for net-next, more relevantly:
> 
> * One fix for potential NULL dereference in xt_HMARK by Dan Carpenter.
> 
> * Conversion to use _ALL macro in xt_hashlimit as you suggested by
>   Florian Westphal.
> 
> * One fix for timeout overflow from Jozsef Kadlecsik.
> 
> * Replace usage of modulus for hash calculation in xt_HMARK as you suggested
>   from myself.
> 
> You can pull these changes from:
> 
> git://1984.lsi.us.es/net-next master

Pulled, thanks a lot!

RE: [PATCH 3/7] netfilter: xt_HMARK: modulus is expensive for hash calculation

[David Laight]

 
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> 
> Use:
> 
> ((u64)(HASH_VAL * HASH_SIZE)) >> 32
> 
> as suggested by David S. Miller.

That (u64) cast is very unlikely to have any effect.
If you want a 64 bit result from the product of two
32 bit values, you have to cast one of the 32 bit values
prior to the multiply - as in the patch below.

	David

> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  net/netfilter/xt_HMARK.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/xt_HMARK.c b/net/netfilter/xt_HMARK.c
> index 5817d03..0a96a43 100644
> --- a/net/netfilter/xt_HMARK.c
> +++ b/net/netfilter/xt_HMARK.c
> @@ -109,7 +109,7 @@ hmark_hash(struct hmark_tuple *t, const 
> struct xt_hmark_info *info)
>  	hash = jhash_3words(t->src, t->dst, t->uports.v32, 
> info->hashrnd);
>  	hash = hash ^ (t->proto & info->proto_mask);
>  
> -	return (hash % info->hmodulus) + info->hoffset;
> +	return (((u64)hash * info->hmodulus) >> 32) + info->hoffset;
>  }
>  
>  static void
> -- 
> 1.7.10
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

RE: [PATCH 3/7] netfilter: xt_HMARK: modulus is expensive for hash calculation

[Eric Dumazet]

On Thu, 2012-05-17 at 09:16 +0100, David Laight wrote:
> > From: Pablo Neira Ayuso <pablo@netfilter.org>
> > 
> > Use:
> > 
> > ((u64)(HASH_VAL * HASH_SIZE)) >> 32
> > 
> > as suggested by David S. Miller.
> 
> That (u64) cast is very unlikely to have any effect.
> If you want a 64 bit result from the product of two
> 32 bit values, you have to cast one of the 32 bit values
> prior to the multiply - as in the patch below.

Hey, Changelog is a bit wrong (for several reasons) but code is correct.

return (((u64)hash * info->hmodulus) >> 32) + info->hoffset;

Re: [PATCH 3/7] netfilter: xt_HMARK: modulus is expensive for hash calculation

[Pablo Neira Ayuso]

On Thu, May 17, 2012 at 10:39:28AM +0200, Eric Dumazet wrote:
> On Thu, 2012-05-17 at 09:16 +0100, David Laight wrote:
> > > From: Pablo Neira Ayuso <pablo@netfilter.org>
> > > 
> > > Use:
> > > 
> > > ((u64)(HASH_VAL * HASH_SIZE)) >> 32
> > > 
> > > as suggested by David S. Miller.
> > 
> > That (u64) cast is very unlikely to have any effect.
> > If you want a 64 bit result from the product of two
> > 32 bit values, you have to cast one of the 32 bit values
> > prior to the multiply - as in the patch below.
> 
> Hey, Changelog is a bit wrong (for several reasons) but code is correct.
> 
> return (((u64)hash * info->hmodulus) >> 32) + info->hoffset;

Sorry, for the mistake in the changelog. I copied & pasted it from the
mailing list discussion.