From 2a50d805e59ed18265fca44825719f35927af8af Mon Sep 17 00:00:00 2001 From: Pablo Neira Ayuso Date: Mon, 6 Jan 2014 13:54:30 +0100 Subject: [PATCH 1/3] Revert "netfilter: avoid get_random_bytes calls" 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 --- 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 7d4254b0dc6b..3c4b69e5fe17 100644 --- a/net/netfilter/nfnetlink_log.c +++ b/net/netfilter/nfnetlink_log.c @@ -28,6 +28,8 @@ #include #include #include +#include +#include #include #include #include @@ -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 6aae699aeb46..3d3f8fce10a5 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 190854be7629..370adf622cef 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 7671e8214919..c40b2695633b 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 d819f62b3b7c..a3910fc2122b 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 bfdc29f1a04a..1e657cf715c4 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) { From 138aef7dca831403e50a719d1be18dfd52027ea2 Mon Sep 17 00:00:00 2001 From: Daniel Borkmann Date: Mon, 6 Jan 2014 01:04:16 +0100 Subject: [PATCH 2/3] netfilter: nf_conntrack_dccp: use %s format string for buffer 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 Signed-off-by: Pablo Neira Ayuso --- 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 a99b6c3427b0..38412684a882 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; } From b22f5126a24b3b2f15448c3f2a254fc10cbc2b92 Mon Sep 17 00:00:00 2001 From: Daniel Borkmann Date: Mon, 6 Jan 2014 00:57:54 +0100 Subject: [PATCH 3/3] netfilter: nf_conntrack_dccp: fix skb_header_pointer API usages 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 Signed-off-by: Pablo Neira Ayuso --- 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 38412684a882..cb372f96f10d 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;