[PATCH 0/3] Netfilter updates for net-next

[PATCH 0/3] Netfilter updates for net-next

[Pablo Neira Ayuso]

Hi David,

The following patchset contains three Netfilter updates, they are:

* Fix wrong usage of skb_header_pointer in the DCCP protocol helper that
  has been there for quite some time. It was resulting in copying the dccp
  header to a pointer allocated in the stack. Fortunately, this pointer
  provides room for the dccp header is 4 bytes long, so no crashes have been
  reported so far. From Daniel Borkmann.

* Use format string to print in the invocation of nf_log_packet(), again
  in the DCCP helper. Also from Daniel Borkmann.

* Revert "netfilter: avoid get_random_bytes call" as prandom32 does not
  guarantee enough entropy when being calling this at boot time, that may
  happen when reloading the rule.

You can pull these changes from:

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

Thanks!

P.S: I still have a pending pull request that should land anytime soon with
several nftables updates from Patrick, will send them asap to reach your merge
window.

----------------------------------------------------------------

The following changes since commit b912b2f8fc71df4c3ffa7a9fe2c2227e8bcdaa07:

  net/mlx4_core: Warn if device doesn't have enough PCI bandwidth (2014-01-05 20:37:05 -0500)

are available in the git repository at:

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

for you to fetch changes up to b22f5126a24b3b2f15448c3f2a254fc10cbc2b92:

  netfilter: nf_conntrack_dccp: fix skb_header_pointer API usages (2014-01-06 17:40:02 +0100)

----------------------------------------------------------------
Daniel Borkmann (2):
      netfilter: nf_conntrack_dccp: use %s format string for buffer
      netfilter: nf_conntrack_dccp: fix skb_header_pointer API usages

Pablo Neira Ayuso (1):
      Revert "netfilter: avoid get_random_bytes calls"

 net/netfilter/nf_conntrack_proto_dccp.c |   10 +++++-----
 net/netfilter/nfnetlink_log.c           |    8 ++++++++
 net/netfilter/nft_hash.c                |    2 +-
 net/netfilter/xt_RATEEST.c              |    2 +-
 net/netfilter/xt_connlimit.c            |    2 +-
 net/netfilter/xt_hashlimit.c            |    2 +-
 net/netfilter/xt_recent.c               |    2 +-
 7 files changed, 18 insertions(+), 10 deletions(-)

-- 
1.7.10.4

[PATCH 1/3] Revert "netfilter: avoid get_random_bytes calls"

[Pablo Neira Ayuso]

This reverts commit a42b99a6e329654d376b330de057eff87686d890.

Hannes Frederic Sowa reported some problems with this patch, more specifically
that prandom_u32() may not be ready at boot time, see:

http://marc.info/?l=linux-netdev&m=138896532403533&w=2

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nfnetlink_log.c |    8 ++++++++
 net/netfilter/nft_hash.c      |    2 +-
 net/netfilter/xt_RATEEST.c    |    2 +-
 net/netfilter/xt_connlimit.c  |    2 +-
 net/netfilter/xt_hashlimit.c  |    2 +-
 net/netfilter/xt_recent.c     |    2 +-
 6 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index 7d4254b..3c4b69e 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -28,6 +28,8 @@
 #include <linux/proc_fs.h>
 #include <linux/security.h>
 #include <linux/list.h>
+#include <linux/jhash.h>
+#include <linux/random.h>
 #include <linux/slab.h>
 #include <net/sock.h>
 #include <net/netfilter/nf_log.h>
@@ -73,6 +75,7 @@ struct nfulnl_instance {
 };
 
 #define INSTANCE_BUCKETS	16
+static unsigned int hash_init;
 
 static int nfnl_log_net_id __read_mostly;
 
@@ -1063,6 +1066,11 @@ static int __init nfnetlink_log_init(void)
 {
 	int status = -ENOMEM;
 
+	/* it's not really all that important to have a random value, so
+	 * we can do this from the init function, even if there hasn't
+	 * been that much entropy yet */
+	get_random_bytes(&hash_init, sizeof(hash_init));
+
 	netlink_register_notifier(&nfulnl_rtnl_notifier);
 	status = nfnetlink_subsys_register(&nfulnl_subsys);
 	if (status < 0) {
diff --git a/net/netfilter/nft_hash.c b/net/netfilter/nft_hash.c
index 6aae699..3d3f8fc 100644
--- a/net/netfilter/nft_hash.c
+++ b/net/netfilter/nft_hash.c
@@ -164,7 +164,7 @@ static int nft_hash_init(const struct nft_set *set,
 	unsigned int cnt, i;
 
 	if (unlikely(!nft_hash_rnd_initted)) {
-		nft_hash_rnd = prandom_u32();
+		get_random_bytes(&nft_hash_rnd, 4);
 		nft_hash_rnd_initted = true;
 	}
 
diff --git a/net/netfilter/xt_RATEEST.c b/net/netfilter/xt_RATEEST.c
index 190854b..370adf6 100644
--- a/net/netfilter/xt_RATEEST.c
+++ b/net/netfilter/xt_RATEEST.c
@@ -100,7 +100,7 @@ static int xt_rateest_tg_checkentry(const struct xt_tgchk_param *par)
 	int ret;
 
 	if (unlikely(!rnd_inited)) {
-		jhash_rnd = prandom_u32();
+		get_random_bytes(&jhash_rnd, sizeof(jhash_rnd));
 		rnd_inited = true;
 	}
 
diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
index 7671e82..c40b269 100644
--- a/net/netfilter/xt_connlimit.c
+++ b/net/netfilter/xt_connlimit.c
@@ -229,7 +229,7 @@ static int connlimit_mt_check(const struct xt_mtchk_param *par)
 		u_int32_t rand;
 
 		do {
-			rand = prandom_u32();
+			get_random_bytes(&rand, sizeof(rand));
 		} while (!rand);
 		cmpxchg(&connlimit_rnd, 0, rand);
 	}
diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index d819f62..a3910fc 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -177,7 +177,7 @@ dsthash_alloc_init(struct xt_hashlimit_htable *ht,
 	/* initialize hash with random val at the time we allocate
 	 * the first hashtable entry */
 	if (unlikely(!ht->rnd_initialized)) {
-		ht->rnd = prandom_u32();
+		get_random_bytes(&ht->rnd, sizeof(ht->rnd));
 		ht->rnd_initialized = true;
 	}
 
diff --git a/net/netfilter/xt_recent.c b/net/netfilter/xt_recent.c
index bfdc29f..1e657cf 100644
--- a/net/netfilter/xt_recent.c
+++ b/net/netfilter/xt_recent.c
@@ -334,7 +334,7 @@ static int recent_mt_check(const struct xt_mtchk_param *par,
 	size_t sz;
 
 	if (unlikely(!hash_rnd_inited)) {
-		hash_rnd = prandom_u32();
+		get_random_bytes(&hash_rnd, sizeof(hash_rnd));
 		hash_rnd_inited = true;
 	}
 	if (info->check_set & ~XT_RECENT_VALID_FLAGS) {
-- 
1.7.10.4

[PATCH 2/3] netfilter: nf_conntrack_dccp: use %s format string for buffer

[Pablo Neira Ayuso]

From: Daniel Borkmann <dborkman@redhat.com>

Some invocations of nf_log_packet() use arg buffer directly instead of
"%s" format string with follow-up buffer pointer. Currently, these two
usages are not really critical, but we should fix this up nevertheless
so that we don't run into trouble if that changes one day.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_conntrack_proto_dccp.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto_dccp.c b/net/netfilter/nf_conntrack_proto_dccp.c
index a99b6c3..3841268 100644
--- a/net/netfilter/nf_conntrack_proto_dccp.c
+++ b/net/netfilter/nf_conntrack_proto_dccp.c
@@ -457,7 +457,7 @@ static bool dccp_new(struct nf_conn *ct, const struct sk_buff *skb,
 out_invalid:
 	if (LOG_INVALID(net, IPPROTO_DCCP))
 		nf_log_packet(net, nf_ct_l3num(ct), 0, skb, NULL, NULL,
-			      NULL, msg);
+			      NULL, "%s", msg);
 	return false;
 }
 
@@ -614,7 +614,7 @@ static int dccp_error(struct net *net, struct nf_conn *tmpl,
 
 out_invalid:
 	if (LOG_INVALID(net, IPPROTO_DCCP))
-		nf_log_packet(net, pf, 0, skb, NULL, NULL, NULL, msg);
+		nf_log_packet(net, pf, 0, skb, NULL, NULL, NULL, "%s", msg);
 	return -NF_ACCEPT;
 }
 
-- 
1.7.10.4

[PATCH 3/3] netfilter: nf_conntrack_dccp: fix skb_header_pointer API usages

[Pablo Neira Ayuso]

From: Daniel Borkmann <dborkman@redhat.com>

Some occurences in the netfilter tree use skb_header_pointer() in
the following way ...

  struct dccp_hdr _dh, *dh;
  ...
  skb_header_pointer(skb, dataoff, sizeof(_dh), &dh);

... where dh itself is a pointer that is being passed as the copy
buffer. Instead, we need to use &_dh as the forth argument so that
we're copying the data into an actual buffer that sits on the stack.

Currently, we probably could overwrite memory on the stack (e.g.
with a possibly mal-formed DCCP packet), but unintentionally, as
we only want the buffer to be placed into _dh variable.

Fixes: 2bc780499aa3 ("[NETFILTER]: nf_conntrack: add DCCP protocol support")
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_conntrack_proto_dccp.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto_dccp.c b/net/netfilter/nf_conntrack_proto_dccp.c
index 3841268..cb372f9 100644
--- a/net/netfilter/nf_conntrack_proto_dccp.c
+++ b/net/netfilter/nf_conntrack_proto_dccp.c
@@ -428,7 +428,7 @@ static bool dccp_new(struct nf_conn *ct, const struct sk_buff *skb,
 	const char *msg;
 	u_int8_t state;
 
-	dh = skb_header_pointer(skb, dataoff, sizeof(_dh), &dh);
+	dh = skb_header_pointer(skb, dataoff, sizeof(_dh), &_dh);
 	BUG_ON(dh == NULL);
 
 	state = dccp_state_table[CT_DCCP_ROLE_CLIENT][dh->dccph_type][CT_DCCP_NONE];
@@ -486,7 +486,7 @@ static int dccp_packet(struct nf_conn *ct, const struct sk_buff *skb,
 	u_int8_t type, old_state, new_state;
 	enum ct_dccp_roles role;
 
-	dh = skb_header_pointer(skb, dataoff, sizeof(_dh), &dh);
+	dh = skb_header_pointer(skb, dataoff, sizeof(_dh), &_dh);
 	BUG_ON(dh == NULL);
 	type = dh->dccph_type;
 
@@ -577,7 +577,7 @@ static int dccp_error(struct net *net, struct nf_conn *tmpl,
 	unsigned int cscov;
 	const char *msg;
 
-	dh = skb_header_pointer(skb, dataoff, sizeof(_dh), &dh);
+	dh = skb_header_pointer(skb, dataoff, sizeof(_dh), &_dh);
 	if (dh == NULL) {
 		msg = "nf_ct_dccp: short packet ";
 		goto out_invalid;
-- 
1.7.10.4

Re: [PATCH 0/3] Netfilter updates for net-next

[David Miller]

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Wed,  8 Jan 2014 20:13:21 +0100

> The following patchset contains three Netfilter updates, they are:
> 
> * Fix wrong usage of skb_header_pointer in the DCCP protocol helper that
>   has been there for quite some time. It was resulting in copying the dccp
>   header to a pointer allocated in the stack. Fortunately, this pointer
>   provides room for the dccp header is 4 bytes long, so no crashes have been
>   reported so far. From Daniel Borkmann.
> 
> * Use format string to print in the invocation of nf_log_packet(), again
>   in the DCCP helper. Also from Daniel Borkmann.
> 
> * Revert "netfilter: avoid get_random_bytes call" as prandom32 does not
>   guarantee enough entropy when being calling this at boot time, that may
>   happen when reloading the rule.
> 
> You can pull these changes from:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next.git master

Pulled, thanks Pablo.