[PATCH -stable,4.x.y] netfilter: nf_nat_redirect: add missing NULL pointer check

[PATCH -stable,4.x.y] netfilter: nf_nat_redirect: add missing NULL pointer check

[Pablo Neira Ayuso]

From: Munehisa Kamata <kamatam@amazon.com>

[ upstream commit 94f9cd81436c85d8c3a318ba92e236ede73752fc ]

Commit 8b13eddfdf04cbfa561725cfc42d6868fe896f56 ("netfilter: refactor NAT
redirect IPv4 to use it from nf_tables") has introduced a trivial logic
change which can result in the following crash.

BUG: unable to handle kernel NULL pointer dereference at 0000000000000030
IP: [<ffffffffa033002d>] nf_nat_redirect_ipv4+0x2d/0xa0 [nf_nat_redirect]
PGD 3ba662067 PUD 3ba661067 PMD 0
Oops: 0000 [#1] SMP
Modules linked in: ipv6(E) xt_REDIRECT(E) nf_nat_redirect(E) xt_tcpudp(E) iptable_nat(E) nf_conntrack_ipv4(E) nf_defrag_ipv4(E) nf_nat_ipv4(E) nf_nat(E) nf_conntrack(E) ip_tables(E) x_tables(E) binfmt_misc(E) xfs(E) libcrc32c(E) evbug(E) evdev(E) psmouse(E) i2c_piix4(E) i2c_core(E) acpi_cpufreq(E) button(E) ext4(E) crc16(E) jbd2(E) mbcache(E) dm_mirror(E) dm_region_hash(E) dm_log(E) dm_mod(E)
CPU: 0 PID: 2536 Comm: ip Tainted: G            E   4.1.7-15.23.amzn1.x86_64 #1
Hardware name: Xen HVM domU, BIOS 4.2.amazon 05/06/2015
task: ffff8800eb438000 ti: ffff8803ba664000 task.ti: ffff8803ba664000
[...]
Call Trace:
 <IRQ>
 [<ffffffffa0334065>] redirect_tg4+0x15/0x20 [xt_REDIRECT]
 [<ffffffffa02e2e99>] ipt_do_table+0x2b9/0x5e1 [ip_tables]
 [<ffffffffa0328045>] iptable_nat_do_chain+0x25/0x30 [iptable_nat]
 [<ffffffffa031777d>] nf_nat_ipv4_fn+0x13d/0x1f0 [nf_nat_ipv4]
 [<ffffffffa0328020>] ? iptable_nat_ipv4_fn+0x20/0x20 [iptable_nat]
 [<ffffffffa031785e>] nf_nat_ipv4_in+0x2e/0x90 [nf_nat_ipv4]
 [<ffffffffa03280a5>] iptable_nat_ipv4_in+0x15/0x20 [iptable_nat]
 [<ffffffff81449137>] nf_iterate+0x57/0x80
 [<ffffffff814491f7>] nf_hook_slow+0x97/0x100
 [<ffffffff814504d4>] ip_rcv+0x314/0x400

unsigned int
nf_nat_redirect_ipv4(struct sk_buff *skb,
...
{
...
		rcu_read_lock();
		indev = __in_dev_get_rcu(skb->dev);
		if (indev != NULL) {
			ifa = indev->ifa_list;
			newdst = ifa->ifa_local; <---
		}
		rcu_read_unlock();
...
}

Before the commit, 'ifa' had been always checked before access. After the
commit, however, it could be accessed even if it's NULL. Interestingly,
this was once fixed in 2003.

http://marc.info/?l=netfilter-devel&m=106668497403047&w=2

In addition to the original one, we have seen the crash when packets that
need to be redirected somehow arrive on an interface which hasn't been
yet fully configured.

This change just reverts the logic to the old behavior to avoid the crash.

Fixes: 8b13eddfdf04 ("netfilter: refactor NAT redirect IPv4 to use it from nf_tables")
Cc: <stable@vger.kernel.org> # 4.1.x
Cc: <stable@vger.kernel.org> # 4.2.x
Cc: <stable@vger.kernel.org> # 4.3.x
Signed-off-by: Munehisa Kamata <kamatam@amazon.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_nat_redirect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nf_nat_redirect.c b/net/netfilter/nf_nat_redirect.c
index 97b75f9..d438698 100644
--- a/net/netfilter/nf_nat_redirect.c
+++ b/net/netfilter/nf_nat_redirect.c
@@ -55,7 +55,7 @@ nf_nat_redirect_ipv4(struct sk_buff *skb,
 
 		rcu_read_lock();
 		indev = __in_dev_get_rcu(skb->dev);
-		if (indev != NULL) {
+		if (indev && indev->ifa_list) {
 			ifa = indev->ifa_list;
 			newdst = ifa->ifa_local;
 		}
-- 
2.1.4

[PATCH -stable,4.3.y] netfilter: xt_TEE: fix NULL dereference

[Pablo Neira Ayuso]

From: Eric Dumazet <edumazet@google.com>

[ upstream commit 45efccdbec3cd465c4776ed9ca1d7b1bba1b7e34 ]

iptables -I INPUT ... -j TEE --gateway 10.1.2.3

<crash> because --oif was not specified

tee_tg_check() sets ->priv pointer to NULL in this case.

Fixes: bbde9fc1824a ("netfilter: factor out packet duplication for IPv4/IPv6")
Cc: <stable@vger.kernel.org> # 4.3.x
Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/xt_TEE.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/xt_TEE.c b/net/netfilter/xt_TEE.c
index fd980aa..c5fdea1 100644
--- a/net/netfilter/xt_TEE.c
+++ b/net/netfilter/xt_TEE.c
@@ -31,8 +31,9 @@ static unsigned int
 tee_tg4(struct sk_buff *skb, const struct xt_action_param *par)
 {
 	const struct xt_tee_tginfo *info = par->targinfo;
+	int oif = info->priv ? info->priv->oif : 0;
 
-	nf_dup_ipv4(skb, par->hooknum, &info->gw.in, info->priv->oif);
+	nf_dup_ipv4(skb, par->hooknum, &info->gw.in, oif);
 
 	return XT_CONTINUE;
 }
@@ -42,8 +43,9 @@ static unsigned int
 tee_tg6(struct sk_buff *skb, const struct xt_action_param *par)
 {
 	const struct xt_tee_tginfo *info = par->targinfo;
+	int oif = info->priv ? info->priv->oif : 0;
 
-	nf_dup_ipv6(skb, par->hooknum, &info->gw.in6, info->priv->oif);
+	nf_dup_ipv6(skb, par->hooknum, &info->gw.in6, oif);
 
 	return XT_CONTINUE;
 }
-- 
2.1.4

[PATCH -stable,3.4.y] netfilter: nf_conntrack: fix RCU race in nf_conntrack_find_get

[Pablo Neira Ayuso]

From: Andrey Vagin <avagin@openvz.org>

[ upstream commit 6aec24dfd7548dba0134f60f28f58580f07c540b ]

Lets look at destroy_conntrack:

hlist_nulls_del_rcu(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode);
...
nf_conntrack_free(ct)
	kmem_cache_free(net->ct.nf_conntrack_cachep, ct);

net->ct.nf_conntrack_cachep is created with SLAB_DESTROY_BY_RCU.

The hash is protected by rcu, so readers look up conntracks without
locks.
A conntrack is removed from the hash, but in this moment a few readers
still can use the conntrack. Then this conntrack is released and another
thread creates conntrack with the same address and the equal tuple.
After this a reader starts to validate the conntrack:
* It's not dying, because a new conntrack was created
* nf_ct_tuple_equal() returns true.

But this conntrack is not initialized yet, so it can not be used by two
threads concurrently. In this case BUG_ON may be triggered from
nf_nat_setup_info().

Florian Westphal suggested to check the confirm bit too. I think it's
right.

task 1			task 2			task 3
			nf_conntrack_find_get
			 ____nf_conntrack_find
destroy_conntrack
 hlist_nulls_del_rcu
 nf_conntrack_free
 kmem_cache_free
						__nf_conntrack_alloc
						 kmem_cache_alloc
						 memset(&ct->tuplehash[IP_CT_DIR_MAX],
			 if (nf_ct_is_dying(ct))
			 if (!nf_ct_tuple_equal()

I'm not sure, that I have ever seen this race condition in a real life.
Currently we are investigating a bug, which is reproduced on a few nodes.
In our case one conntrack is initialized from a few tasks concurrently,
we don't have any other explanation for this.

<2>[46267.083061] kernel BUG at net/ipv4/netfilter/nf_nat_core.c:322!
...
<4>[46267.083951] RIP: 0010:[<ffffffffa01e00a4>]  [<ffffffffa01e00a4>] nf_nat_setup_info+0x564/0x590 [nf_nat]
...
<4>[46267.085549] Call Trace:
<4>[46267.085622]  [<ffffffffa023421b>] alloc_null_binding+0x5b/0xa0 [iptable_nat]
<4>[46267.085697]  [<ffffffffa02342bc>] nf_nat_rule_find+0x5c/0x80 [iptable_nat]
<4>[46267.085770]  [<ffffffffa0234521>] nf_nat_fn+0x111/0x260 [iptable_nat]
<4>[46267.085843]  [<ffffffffa0234798>] nf_nat_out+0x48/0xd0 [iptable_nat]
<4>[46267.085919]  [<ffffffff814841b9>] nf_iterate+0x69/0xb0
<4>[46267.085991]  [<ffffffff81494e70>] ? ip_finish_output+0x0/0x2f0
<4>[46267.086063]  [<ffffffff81484374>] nf_hook_slow+0x74/0x110
<4>[46267.086133]  [<ffffffff81494e70>] ? ip_finish_output+0x0/0x2f0
<4>[46267.086207]  [<ffffffff814b5890>] ? dst_output+0x0/0x20
<4>[46267.086277]  [<ffffffff81495204>] ip_output+0xa4/0xc0
<4>[46267.086346]  [<ffffffff814b65a4>] raw_sendmsg+0x8b4/0x910
<4>[46267.086419]  [<ffffffff814c10fa>] inet_sendmsg+0x4a/0xb0
<4>[46267.086491]  [<ffffffff814459aa>] ? sock_update_classid+0x3a/0x50
<4>[46267.086562]  [<ffffffff81444d67>] sock_sendmsg+0x117/0x140
<4>[46267.086638]  [<ffffffff8151997b>] ? _spin_unlock_bh+0x1b/0x20
<4>[46267.086712]  [<ffffffff8109d370>] ? autoremove_wake_function+0x0/0x40
<4>[46267.086785]  [<ffffffff81495e80>] ? do_ip_setsockopt+0x90/0xd80
<4>[46267.086858]  [<ffffffff8100be0e>] ? call_function_interrupt+0xe/0x20
<4>[46267.086936]  [<ffffffff8118cb10>] ? ub_slab_ptr+0x20/0x90
<4>[46267.087006]  [<ffffffff8118cb10>] ? ub_slab_ptr+0x20/0x90
<4>[46267.087081]  [<ffffffff8118f2e8>] ? kmem_cache_alloc+0xd8/0x1e0
<4>[46267.087151]  [<ffffffff81445599>] sys_sendto+0x139/0x190
<4>[46267.087229]  [<ffffffff81448c0d>] ? sock_setsockopt+0x16d/0x6f0
<4>[46267.087303]  [<ffffffff810efa47>] ? audit_syscall_entry+0x1d7/0x200
<4>[46267.087378]  [<ffffffff810ef795>] ? __audit_syscall_exit+0x265/0x290
<4>[46267.087454]  [<ffffffff81474885>] ? compat_sys_setsockopt+0x75/0x210
<4>[46267.087531]  [<ffffffff81474b5f>] compat_sys_socketcall+0x13f/0x210
<4>[46267.087607]  [<ffffffff8104dea3>] ia32_sysret+0x0/0x5
<4>[46267.087676] Code: 91 20 e2 01 75 29 48 89 de 4c 89 f7 e8 56 fa ff ff 85 c0 0f 84 68 fc ff ff 0f b6 4d c6 41 8b 45 00 e9 4d fb ff ff e8 7c 19 e9 e0 <0f> 0b eb fe f6 05 17 91 20 e2 80 74 ce 80 3d 5f 2e 00 00 00 74
<1>[46267.088023] RIP  [<ffffffffa01e00a4>] nf_nat_setup_info+0x564/0x590

Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Florian Westphal <fw@strlen.de>
Cc: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Patrick McHardy <kaber@trash.net>
Cc: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Andrey Vagin <avagin@openvz.org>
Acked-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Ani Sinha <ani@arista.com>
Tested-by: "Neal P. Murphy" <neal.p.murphy@alum.wpi.edu>
---
 net/netfilter/nf_conntrack_core.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 9a171b2..71935fc 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -309,6 +309,21 @@ static void death_by_timeout(unsigned long ul_conntrack)
 	nf_ct_put(ct);
 }
 
+static inline bool
+nf_ct_key_equal(struct nf_conntrack_tuple_hash *h,
+			const struct nf_conntrack_tuple *tuple,
+			u16 zone)
+{
+	struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(h);
+
+	/* A conntrack can be recreated with the equal tuple,
+	 * so we need to check that the conntrack is confirmed
+	 */
+	return nf_ct_tuple_equal(tuple, &h->tuple) &&
+		nf_ct_zone(ct) == zone &&
+		nf_ct_is_confirmed(ct);
+}
+
 /*
  * Warning :
  * - Caller must take a reference on returned object
@@ -330,8 +345,7 @@ ____nf_conntrack_find(struct net *net, u16 zone,
 	local_bh_disable();
 begin:
 	hlist_nulls_for_each_entry_rcu(h, n, &net->ct.hash[bucket], hnnode) {
-		if (nf_ct_tuple_equal(tuple, &h->tuple) &&
-		    nf_ct_zone(nf_ct_tuplehash_to_ctrack(h)) == zone) {
+		if (nf_ct_key_equal(h, tuple, zone)) {
 			NF_CT_STAT_INC(net, found);
 			local_bh_enable();
 			return h;
@@ -378,8 +392,7 @@ begin:
 			     !atomic_inc_not_zero(&ct->ct_general.use)))
 			h = NULL;
 		else {
-			if (unlikely(!nf_ct_tuple_equal(tuple, &h->tuple) ||
-				     nf_ct_zone(ct) != zone)) {
+			if (unlikely(!nf_ct_key_equal(h, tuple, zone))) {
 				nf_ct_put(ct);
 				goto begin;
 			}
-- 
2.1.4

[PATCH -stable,backport,3.4.y] netfilter: nf_conntrack: don't release a conntrack with non-zero refcnt

[Pablo Neira Ayuso]

[ backport from e53376bef2cd97d3e3f61fdc677fb8da7d03d0da ]

With this patch, the conntrack refcount is initially set to zero and
it is bumped once it is added to any of the list, so we fulfill
Eric's golden rule which is that all released objects always have a
refcount that equals zero.

Andrey Vagin reports that nf_conntrack_free can't be called for a
conntrack with non-zero ref-counter, because it can race with
nf_conntrack_find_get().

A conntrack slab is created with SLAB_DESTROY_BY_RCU. Non-zero
ref-counter says that this conntrack is used. So when we release
a conntrack with non-zero counter, we break this assumption.

CPU1                                    CPU2
____nf_conntrack_find()
                                        nf_ct_put()
                                         destroy_conntrack()
                                        ...
                                        init_conntrack
                                         __nf_conntrack_alloc (set use = 1)
atomic_inc_not_zero(&ct->use) (use = 2)
                                         if (!l4proto->new(ct, skb, dataoff, timeouts))
                                          nf_conntrack_free(ct); (use = 2 !!!)
                                        ...
                                        __nf_conntrack_alloc (set use = 1)
 if (!nf_ct_key_equal(h, tuple, zone))
  nf_ct_put(ct); (use = 0)
   destroy_conntrack()
                                        /* continue to work with CT */

After applying the path "[PATCH] netfilter: nf_conntrack: fix RCU
race in nf_conntrack_find_get" another bug was triggered in
destroy_conntrack():

<4>[67096.759334] ------------[ cut here ]------------
<2>[67096.759353] kernel BUG at net/netfilter/nf_conntrack_core.c:211!
...
<4>[67096.759837] Pid: 498649, comm: atdd veid: 666 Tainted: G         C ---------------    2.6.32-042stab084.18 #1 042stab084_18 /DQ45CB
<4>[67096.759932] RIP: 0010:[<ffffffffa03d99ac>]  [<ffffffffa03d99ac>] destroy_conntrack+0x15c/0x190 [nf_conntrack]
<4>[67096.760255] Call Trace:
<4>[67096.760255]  [<ffffffff814844a7>] nf_conntrack_destroy+0x17/0x30
<4>[67096.760255]  [<ffffffffa03d9bb5>] nf_conntrack_find_get+0x85/0x130 [nf_conntrack]
<4>[67096.760255]  [<ffffffffa03d9fb2>] nf_conntrack_in+0x352/0xb60 [nf_conntrack]
<4>[67096.760255]  [<ffffffffa048c771>] ipv4_conntrack_local+0x51/0x60 [nf_conntrack_ipv4]
<4>[67096.760255]  [<ffffffff81484419>] nf_iterate+0x69/0xb0
<4>[67096.760255]  [<ffffffff814b5b00>] ? dst_output+0x0/0x20
<4>[67096.760255]  [<ffffffff814845d4>] nf_hook_slow+0x74/0x110
<4>[67096.760255]  [<ffffffff814b5b00>] ? dst_output+0x0/0x20
<4>[67096.760255]  [<ffffffff814b66d5>] raw_sendmsg+0x775/0x910
<4>[67096.760255]  [<ffffffff8104c5a8>] ? flush_tlb_others_ipi+0x128/0x130
<4>[67096.760255]  [<ffffffff8100bc4e>] ? apic_timer_interrupt+0xe/0x20
<4>[67096.760255]  [<ffffffff8100bc4e>] ? apic_timer_interrupt+0xe/0x20
<4>[67096.760255]  [<ffffffff814c136a>] inet_sendmsg+0x4a/0xb0
<4>[67096.760255]  [<ffffffff81444e93>] ? sock_sendmsg+0x13/0x140
<4>[67096.760255]  [<ffffffff81444f97>] sock_sendmsg+0x117/0x140
<4>[67096.760255]  [<ffffffff8102e299>] ? native_smp_send_reschedule+0x49/0x60
<4>[67096.760255]  [<ffffffff81519beb>] ? _spin_unlock_bh+0x1b/0x20
<4>[67096.760255]  [<ffffffff8109d930>] ? autoremove_wake_function+0x0/0x40
<4>[67096.760255]  [<ffffffff814960f0>] ? do_ip_setsockopt+0x90/0xd80
<4>[67096.760255]  [<ffffffff8100bc4e>] ? apic_timer_interrupt+0xe/0x20
<4>[67096.760255]  [<ffffffff8100bc4e>] ? apic_timer_interrupt+0xe/0x20
<4>[67096.760255]  [<ffffffff814457c9>] sys_sendto+0x139/0x190
<4>[67096.760255]  [<ffffffff810efa77>] ? audit_syscall_entry+0x1d7/0x200
<4>[67096.760255]  [<ffffffff810ef7c5>] ? __audit_syscall_exit+0x265/0x290
<4>[67096.760255]  [<ffffffff81474daf>] compat_sys_socketcall+0x13f/0x210
<4>[67096.760255]  [<ffffffff8104dea3>] ia32_sysret+0x0/0x5

I have reused the original title for the RFC patch that Andrey posted and
most of the original patch description.

Cc: Eric Dumazet <edumazet@google.com>
Cc: Andrew Vagin <avagin@parallels.com>
Cc: Florian Westphal <fw@strlen.de>
Reported-by: Andrew Vagin <avagin@parallels.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Reviewed-by: Eric Dumazet <edumazet@google.com>
Acked-by: Andrew Vagin <avagin@parallels.com>
Signed-off-by: Ani Sinha <ani@arista.com>
Tested-by: "Neal P. Murphy" <neal.p.murphy@alum.wpi.edu>
---
 net/netfilter/nf_conntrack_core.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 9a171b2..9a46908 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -441,7 +441,9 @@ nf_conntrack_hash_check_insert(struct nf_conn *ct)
 			goto out;
 
 	add_timer(&ct->timeout);
-	nf_conntrack_get(&ct->ct_general);
+	smp_wmb();
+	/* The caller holds a reference to this object */
+	atomic_set(&ct->ct_general.use, 2);
 	__nf_conntrack_hash_insert(ct, hash, repl_hash);
 	NF_CT_STAT_INC(net, insert);
 	spin_unlock_bh(&nf_conntrack_lock);
@@ -732,11 +734,10 @@ __nf_conntrack_alloc(struct net *net, u16 zone,
 		nf_ct_zone->id = zone;
 	}
 #endif
-	/*
-	 * changes to lookup keys must be done before setting refcnt to 1
+	/* Because we use RCU lookups, we set ct_general.use to zero before
+	 * this is inserted in any list.
 	 */
-	smp_wmb();
-	atomic_set(&ct->ct_general.use, 1);
+	atomic_set(&ct->ct_general.use, 0);
 	return ct;
 
 #ifdef CONFIG_NF_CONNTRACK_ZONES
@@ -759,6 +760,10 @@ EXPORT_SYMBOL_GPL(nf_conntrack_alloc);
 void nf_conntrack_free(struct nf_conn *ct)
 {
 	struct net *net = nf_ct_net(ct);
+	/* A freed object has refcnt == 0, that's
+	 * the golden rule for SLAB_DESTROY_BY_RCU
+	 */
+	NF_CT_ASSERT(atomic_read(&ct->ct_general.use) == 0);
 
 	nf_ct_ext_destroy(ct);
 	atomic_dec(&net->ct.count);
@@ -846,6 +851,9 @@ init_conntrack(struct net *net, struct nf_conn *tmpl,
 		NF_CT_STAT_INC(net, new);
 	}
 
+	/* Now it is inserted into the unconfirmed list, bump refcount */
+	nf_conntrack_get(&ct->ct_general);
+
 	/* Overload tuple linked list to put us in unconfirmed list. */
 	hlist_nulls_add_head_rcu(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode,
 		       &net->ct.unconfirmed);

Re: [PATCH -stable,4.x.y] netfilter: nf_nat_redirect: add missing NULL pointer check

[Greg KH]

On Fri, Nov 06, 2015 at 01:44:43PM +0100, Pablo Neira Ayuso wrote:
> From: Munehisa Kamata <kamatam@amazon.com>
> 
> [ upstream commit 94f9cd81436c85d8c3a318ba92e236ede73752fc ]

This isn't a valid git commit id in Linus's tree :(

Re: [PATCH -stable,4.3.y] netfilter: xt_TEE: fix NULL dereference

[Greg KH]

On Fri, Nov 06, 2015 at 01:44:44PM +0100, Pablo Neira Ayuso wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> [ upstream commit 45efccdbec3cd465c4776ed9ca1d7b1bba1b7e34 ]
> 

Not a valid commit id :(

Re: [PATCH -stable,3.4.y] netfilter: nf_conntrack: fix RCU race in nf_conntrack_find_get

[Greg KH]

On Fri, Nov 06, 2015 at 01:44:45PM +0100, Pablo Neira Ayuso wrote:
> From: Andrey Vagin <avagin@openvz.org>
> 
> [ upstream commit 6aec24dfd7548dba0134f60f28f58580f07c540b ]

Also not valid :(

Re: [PATCH -stable,4.x.y] netfilter: nf_nat_redirect: add missing NULL pointer check

[Pablo Neira Ayuso]

On Fri, Nov 06, 2015 at 09:08:58AM -0800, Greg KH wrote:
> On Fri, Nov 06, 2015 at 01:44:43PM +0100, Pablo Neira Ayuso wrote:
> > From: Munehisa Kamata <kamatam@amazon.com>
> > 
> > [ upstream commit 94f9cd81436c85d8c3a318ba92e236ede73752fc ]
> 
> This isn't a valid git commit id in Linus's tree :(

This is David's tree already:

http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=94f9cd81436c85d8c3a318ba92e236ede73752fc

So this should hit Linus tree soon.

Re: [PATCH -stable,4.3.y] netfilter: xt_TEE: fix NULL dereference

[Pablo Neira Ayuso]

On Fri, Nov 06, 2015 at 09:09:34AM -0800, Greg KH wrote:
> On Fri, Nov 06, 2015 at 01:44:44PM +0100, Pablo Neira Ayuso wrote:
> > From: Eric Dumazet <edumazet@google.com>
> > 
> > [ upstream commit 45efccdbec3cd465c4776ed9ca1d7b1bba1b7e34 ]
> > 
> 
> Not a valid commit id :(

Same thing:

http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=45efccdbec3cd465c4776ed9ca1d7b1bba1b7e34

Re: [PATCH -stable,3.4.y] netfilter: nf_conntrack: fix RCU race in nf_conntrack_find_get

[Pablo Neira Ayuso]

On Fri, Nov 06, 2015 at 09:09:46AM -0800, Greg KH wrote:
> On Fri, Nov 06, 2015 at 01:44:45PM +0100, Pablo Neira Ayuso wrote:
> > From: Andrey Vagin <avagin@openvz.org>
> > 
> > [ upstream commit 6aec24dfd7548dba0134f60f28f58580f07c540b ]
> 
> Also not valid :(

This one you're right:

This should be c6825c0976fa7893692e0e43b09740b419b23c09 instead.

Sorry.

Re: [PATCH -stable,3.4.y] netfilter: nf_conntrack: fix RCU race in nf_conntrack_find_get

[Ani Sinha]

I am not sure how you guys work but I think this two patches needs to
be applied here :

https://git.kernel.org/cgit/linux/kernel/git/lizf/linux-3.4.y-queue.git/



On Fri, Nov 6, 2015 at 9:27 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, Nov 06, 2015 at 09:09:46AM -0800, Greg KH wrote:
>> On Fri, Nov 06, 2015 at 01:44:45PM +0100, Pablo Neira Ayuso wrote:
>> > From: Andrey Vagin <avagin@openvz.org>
>> >
>> > [ upstream commit 6aec24dfd7548dba0134f60f28f58580f07c540b ]
>>
>> Also not valid :(
>
> This one you're right:
>
> This should be c6825c0976fa7893692e0e43b09740b419b23c09 instead.
>
> Sorry.

Re: [PATCH -stable,3.4.y] netfilter: nf_conntrack: fix RCU race in nf_conntrack_find_get

[Greg KH]

On Fri, Nov 06, 2015 at 12:30:55PM -0800, Ani Sinha wrote:
> I am not sure how you guys work but I think this two patches needs to
> be applied here :
> 
> https://git.kernel.org/cgit/linux/kernel/git/lizf/linux-3.4.y-queue.git/

I have no idea what you are referring to here :(

Re: [PATCH -stable,3.4.y] netfilter: nf_conntrack: fix RCU race in nf_conntrack_find_get

[Ani Sinha]

On Fri, Nov 6, 2015 at 2:11 PM, Greg KH <greg@kroah.com> wrote:
> On Fri, Nov 06, 2015 at 12:30:55PM -0800, Ani Sinha wrote:
>> I am not sure how you guys work but I think this two patches needs to
>> be applied here :
>>
>> https://git.kernel.org/cgit/linux/kernel/git/lizf/linux-3.4.y-queue.git/
>
> I have no idea what you are referring to here :(

The patches which we are referring to here is only for linux 3.4
series. The testing was only done on 3.4 not on other -stable
releases. I don't even know if the patch would cleanly apply on other
stable trains. Since I see Zefan maintains 3.4 and he has a linux git
queue, I thought may be these patches should be applied there. That is
what I was wondering. I don't have clear understanding how Zefan pulls
his 3.4 backport patches from.

Does that make sense?

Re: [PATCH -stable,3.4.y] netfilter: nf_conntrack: fix RCU race in nf_conntrack_find_get

[Zefan Li]

Hi Ani,

On 2015/11/7 6:27, Ani Sinha wrote:
> On Fri, Nov 6, 2015 at 2:11 PM, Greg KH <greg@kroah.com> wrote:
>> On Fri, Nov 06, 2015 at 12:30:55PM -0800, Ani Sinha wrote:
>>> I am not sure how you guys work but I think this two patches needs to
>>> be applied here :
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/lizf/linux-3.4.y-queue.git/
>>
>> I have no idea what you are referring to here :(
>
> The patches which we are referring to here is only for linux 3.4
> series. The testing was only done on 3.4 not on other -stable
> releases. I don't even know if the patch would cleanly apply on other
> stable trains. Since I see Zefan maintains 3.4 and he has a linux git
> queue, I thought may be these patches should be applied there. That is
> what I was wondering. I don't have clear understanding how Zefan pulls
> his 3.4 backport patches from.
>
> Does that make sense?
> .

I saw Pablo has already sent those fixes to -stable. I'll queue them up
for next 3.4.y release.

Thanks for working on this!

Re: [PATCH -stable,4.3.y] netfilter: xt_TEE: fix NULL dereference

[Ben Hutchings]

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

On Fri, 2015-11-06 at 18:26 +0100, Pablo Neira Ayuso wrote:
> On Fri, Nov 06, 2015 at 09:09:34AM -0800, Greg KH wrote:
> > On Fri, Nov 06, 2015 at 01:44:44PM +0100, Pablo Neira Ayuso wrote:
> > > From: Eric Dumazet <edumazet@google.com>
> > > 
> > > [ upstream commit 45efccdbec3cd465c4776ed9ca1d7b1bba1b7e34 ]
> > > 
> > 
> > Not a valid commit id :(
> 
> Same thing:
> 
> http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=
> 45efccdbec3cd465c4776ed9ca1d7b1bba1b7e34

But it's not in mainline yet.  You should re-submit to stable once it
is.

Ben.

-- 
Ben Hutchings
73.46% of all statistics are made up.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH -stable,4.3.y] netfilter: xt_TEE: fix NULL dereference

[Pablo Neira Ayuso]

On Sat, Nov 07, 2015 at 04:04:02PM +0000, Ben Hutchings wrote:
> On Fri, 2015-11-06 at 18:26 +0100, Pablo Neira Ayuso wrote:
> > On Fri, Nov 06, 2015 at 09:09:34AM -0800, Greg KH wrote:
> > > On Fri, Nov 06, 2015 at 01:44:44PM +0100, Pablo Neira Ayuso wrote:
> > > > From: Eric Dumazet <edumazet@google.com>
> > > > 
> > > > [ upstream commit 45efccdbec3cd465c4776ed9ca1d7b1bba1b7e34 ]
> > > > 
> > > 
> > > Not a valid commit id :(
> > 
> > Same thing:
> > 
> > http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=
> > 45efccdbec3cd465c4776ed9ca1d7b1bba1b7e34
> 
> But it's not in mainline yet. �You should re-submit to stable once it
> is.

Will do and will update my scripts to avoid this race again.

Thanks Ben.

Re: [PATCH -stable,4.3.y] netfilter: xt_TEE: fix NULL dereference

[Pablo Neira Ayuso]

On Sat, Nov 07, 2015 at 04:04:02PM +0000, Ben Hutchings wrote:
> On Fri, 2015-11-06 at 18:26 +0100, Pablo Neira Ayuso wrote:
> > On Fri, Nov 06, 2015 at 09:09:34AM -0800, Greg KH wrote:
> > > On Fri, Nov 06, 2015 at 01:44:44PM +0100, Pablo Neira Ayuso wrote:
> > > > From: Eric Dumazet <edumazet@google.com>
> > > > 
> > > > [ upstream commit 45efccdbec3cd465c4776ed9ca1d7b1bba1b7e34 ]
> > > > 
> > > 
> > > Not a valid commit id :(
> > 
> > Same thing:
> > 
> > http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=
> > 45efccdbec3cd465c4776ed9ca1d7b1bba1b7e34
> 
> But it's not in mainline yet.  You should re-submit to stable once it
> is.

Will do and will update my scripts to avoid this race again.

Thanks Ben.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH -stable,3.4.y] netfilter: nf_conntrack: fix RCU race in nf_conntrack_find_get

[Ben Hutchings]

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

On Fri, 2015-11-06 at 13:44 +0100, Pablo Neira Ayuso wrote:
> From: Andrey Vagin <avagin@openvz.org>
> 
> [ upstream commit 6aec24dfd7548dba0134f60f28f58580f07c540b ]

The correct commit hash is c6825c0976fa7893692e0e43b09740b419b23c09.

This was applied upstream in 3.14 and also in the 3.12-longterm branch,
but no others.  If it's applicable to 3.4 then it should also be
applicable to 3.10, right?

It also seems to be applicable to 3.2, and even to 2.6.32 (but without
the comparison of zones).

Ben.

> Lets look at destroy_conntrack:
> 
> hlist_nulls_del_rcu(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode);
> ...
> nf_conntrack_free(ct)
> 	> kmem_cache_free(net->ct.nf_conntrack_cachep, ct);
> 
> net->ct.nf_conntrack_cachep is created with SLAB_DESTROY_BY_RCU.
> 
> The hash is protected by rcu, so readers look up conntracks without
> locks.
> A conntrack is removed from the hash, but in this moment a few readers
> still can use the conntrack. Then this conntrack is released and another
> thread creates conntrack with the same address and the equal tuple.
> After this a reader starts to validate the conntrack:
> * It's not dying, because a new conntrack was created
> * nf_ct_tuple_equal() returns true.
> 
> But this conntrack is not initialized yet, so it can not be used by two
> threads concurrently. In this case BUG_ON may be triggered from
> nf_nat_setup_info().
> 
> Florian Westphal suggested to check the confirm bit too. I think it's
> right.
> 
> task 1> 	> 	> 	> task 2> 	> 	> 	> task 3
> 	> 	> 	> nf_conntrack_find_get
> 	> 	> 	>  ____nf_conntrack_find
> destroy_conntrack
>  hlist_nulls_del_rcu
>  nf_conntrack_free
>  kmem_cache_free
> 	> 	> 	> 	> 	> 	> __nf_conntrack_alloc
> 	> 	> 	> 	> 	> 	>  kmem_cache_alloc
> 	> 	> 	> 	> 	> 	>  memset(&ct->tuplehash[IP_CT_DIR_MAX],
> 	> 	> 	>  if (nf_ct_is_dying(ct))
> 	> 	> 	>  if (!nf_ct_tuple_equal()
> 
> I'm not sure, that I have ever seen this race condition in a real life.
> Currently we are investigating a bug, which is reproduced on a few nodes.
> In our case one conntrack is initialized from a few tasks concurrently,
> we don't have any other explanation for this.
> 
> <2>[46267.083061] kernel BUG at net/ipv4/netfilter/nf_nat_core.c:322!
> ...
> <4>[46267.083951] RIP: 0010:[]  [] nf_nat_setup_info+0x564/0x590 [nf_nat]
> ...
> <4>[46267.085549] Call Trace:
> <4>[46267.085622]  [] alloc_null_binding+0x5b/0xa0 [iptable_nat]
> <4>[46267.085697]  [] nf_nat_rule_find+0x5c/0x80 [iptable_nat]
> <4>[46267.085770]  [] nf_nat_fn+0x111/0x260 [iptable_nat]
> <4>[46267.085843]  [] nf_nat_out+0x48/0xd0 [iptable_nat]
> <4>[46267.085919]  [] nf_iterate+0x69/0xb0
> <4>[46267.085991]  [] ? ip_finish_output+0x0/0x2f0
> <4>[46267.086063]  [] nf_hook_slow+0x74/0x110
> <4>[46267.086133]  [] ? ip_finish_output+0x0/0x2f0
> <4>[46267.086207]  [] ? dst_output+0x0/0x20
> <4>[46267.086277]  [] ip_output+0xa4/0xc0
> <4>[46267.086346]  [] raw_sendmsg+0x8b4/0x910
> <4>[46267.086419]  [] inet_sendmsg+0x4a/0xb0
> <4>[46267.086491]  [] ? sock_update_classid+0x3a/0x50
> <4>[46267.086562]  [] sock_sendmsg+0x117/0x140
> <4>[46267.086638]  [] ? _spin_unlock_bh+0x1b/0x20
> <4>[46267.086712]  [] ? autoremove_wake_function+0x0/0x40
> <4>[46267.086785]  [] ? do_ip_setsockopt+0x90/0xd80
> <4>[46267.086858]  [] ? call_function_interrupt+0xe/0x20
> <4>[46267.086936]  [] ? ub_slab_ptr+0x20/0x90
> <4>[46267.087006]  [] ? ub_slab_ptr+0x20/0x90
> <4>[46267.087081]  [] ? kmem_cache_alloc+0xd8/0x1e0
> <4>[46267.087151]  [] sys_sendto+0x139/0x190
> <4>[46267.087229]  [] ? sock_setsockopt+0x16d/0x6f0
> <4>[46267.087303]  [] ? audit_syscall_entry+0x1d7/0x200
> <4>[46267.087378]  [] ? __audit_syscall_exit+0x265/0x290
> <4>[46267.087454]  [] ? compat_sys_setsockopt+0x75/0x210
> <4>[46267.087531]  [] compat_sys_socketcall+0x13f/0x210
> <4>[46267.087607]  [] ia32_sysret+0x0/0x5
> <4>[46267.087676] Code: 91 20 e2 01 75 29 48 89 de 4c 89 f7 e8 56 fa ff ff 85 c0 0f 84 68 fc ff ff 0f b6 4d c6 41 8b 45 00 e9 4d fb ff ff e8 7c 19 e9 e0 <0f> 0b eb fe f6 05 17 91 20 e2 80 74 ce 80 3d 5f 2e 00 00 00 74
> <1>[46267.088023] RIP  [] nf_nat_setup_info+0x564/0x590
> 
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Florian Westphal <fw@strlen.de>
> Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> Cc: Patrick McHardy <kaber@trash.net>
> Cc: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> Signed-off-by: Andrey Vagin <avagin@openvz.org>
> Acked-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> Signed-off-by: Ani Sinha <ani@arista.com>
> Tested-by: "Neal P. Murphy" <neal.p.murphy@alum.wpi.edu>
> ---
>  net/netfilter/nf_conntrack_core.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index 9a171b2..71935fc 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -309,6 +309,21 @@ static void death_by_timeout(unsigned long ul_conntrack)
>  > 	> nf_ct_put(ct);
>  }
>  
> +static inline bool
> +nf_ct_key_equal(struct nf_conntrack_tuple_hash *h,
> +> 	> 	> 	> const struct nf_conntrack_tuple *tuple,
> +> 	> 	> 	> u16 zone)
> +{
> +> 	> struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(h);
> +
> +> 	> /* A conntrack can be recreated with the equal tuple,
> +> 	>  * so we need to check that the conntrack is confirmed
> +> 	>  */
> +> 	> return nf_ct_tuple_equal(tuple, &h->tuple) &&
> +> 	> 	> nf_ct_zone(ct) == zone &&
> +> 	> 	> nf_ct_is_confirmed(ct);
> +}
> +
>  /*
>   * Warning :
>   * - Caller must take a reference on returned object
> @@ -330,8 +345,7 @@ ____nf_conntrack_find(struct net *net, u16 zone,
>  > 	> local_bh_disable();
>  begin:
>  > 	> hlist_nulls_for_each_entry_rcu(h, n, &net->ct.hash[bucket], hnnode) {
> -> 	> 	> if (nf_ct_tuple_equal(tuple, &h->tuple) &&
> -> 	> 	>     nf_ct_zone(nf_ct_tuplehash_to_ctrack(h)) == zone) {
> +> 	> 	> if (nf_ct_key_equal(h, tuple, zone)) {
>  > 	> 	> 	> NF_CT_STAT_INC(net, found);
>  > 	> 	> 	> local_bh_enable();
>  > 	> 	> 	> return h;
> @@ -378,8 +392,7 @@ begin:
>  > 	> 	> 	>      !atomic_inc_not_zero(&ct->ct_general.use)))
>  > 	> 	> 	> h = NULL;
>  > 	> 	> else {
> -> 	> 	> 	> if (unlikely(!nf_ct_tuple_equal(tuple, &h->tuple) ||
> -> 	> 	> 	> 	>      nf_ct_zone(ct) != zone)) {
> +> 	> 	> 	> if (unlikely(!nf_ct_key_equal(h, tuple, zone))) {
>  > 	> 	> 	> 	> nf_ct_put(ct);
>  > 	> 	> 	> 	> goto begin;
>  > 	> 	> 	> }
-- 
Ben Hutchings
Everything should be made as simple as possible, but not simpler.
                                                           - Albert Einstein

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH -stable,4.x.y] netfilter: nf_nat_redirect: add missing NULL pointer check

[Munehisa Kamata]

Hi Greg,

This commit is already in Linus's tree. Could you please pick this up for 4.1.y?

Regards,
Munehisa

On 11/6/2015 9:26 AM, Pablo Neira Ayuso wrote:
> On Fri, Nov 06, 2015 at 09:08:58AM -0800, Greg KH wrote:
>> On Fri, Nov 06, 2015 at 01:44:43PM +0100, Pablo Neira Ayuso wrote:
>>> From: Munehisa Kamata <kamatam@amazon.com>
>>>
>>> [ upstream commit 94f9cd81436c85d8c3a318ba92e236ede73752fc ]
>>
>> This isn't a valid git commit id in Linus's tree :(
> 
> This is David's tree already:
> 
> http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=94f9cd81436c85d8c3a318ba92e236ede73752fc
> 
> So this should hit Linus tree soon.
> 

Re: [PATCH -stable,4.x.y] netfilter: nf_nat_redirect: add missing NULL pointer check

[Pablo Neira Ayuso]

On Mon, Jan 18, 2016 at 11:27:13AM -0800, Munehisa Kamata wrote:
> Hi Greg,
> 
> This commit is already in Linus's tree. Could you please pick this up for 4.1.y?

I submitted this twice... Last time I made it was this one:

http://patchwork.ozlabs.org/patch/544964/

Previous was rejected because it was not in Linus tree:

http://patchwork.ozlabs.org/patch/540946/

But the last attempt got no feedback...

> On 11/6/2015 9:26 AM, Pablo Neira Ayuso wrote:
> > On Fri, Nov 06, 2015 at 09:08:58AM -0800, Greg KH wrote:
> >> On Fri, Nov 06, 2015 at 01:44:43PM +0100, Pablo Neira Ayuso wrote:
> >>> From: Munehisa Kamata <kamatam@amazon.com>
> >>>
> >>> [ upstream commit 94f9cd81436c85d8c3a318ba92e236ede73752fc ]
> >>
> >> This isn't a valid git commit id in Linus's tree :(
> > 
> > This is David's tree already:
> > 
> > http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=94f9cd81436c85d8c3a318ba92e236ede73752fc
> > 
> > So this should hit Linus tree soon.
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH -stable,4.x.y] netfilter: nf_nat_redirect: add missing NULL pointer check

[Pablo Neira Ayuso]

On Mon, Jan 18, 2016 at 08:47:18PM +0100, Pablo Neira Ayuso wrote:
> On Mon, Jan 18, 2016 at 11:27:13AM -0800, Munehisa Kamata wrote:
> > Hi Greg,
> > 
> > This commit is already in Linus's tree. Could you please pick this up for 4.1.y?
> 
> I submitted this twice... Last time I made it was this one:
> 
> http://patchwork.ozlabs.org/patch/544964/
> 
> Previous was rejected because it was not in Linus tree:
> 
> http://patchwork.ozlabs.org/patch/540946/
> 
> But the last attempt got no feedback...

BTW, you can find all my -stable submissions here:

http://patchwork.ozlabs.org/project/netfilter-devel/list/?state=*&q=stable-

Re: [PATCH -stable,4.x.y] netfilter: nf_nat_redirect: add missing NULL pointer check

[Greg KH]

On Mon, Jan 18, 2016 at 08:48:02PM +0100, Pablo Neira Ayuso wrote:
> On Mon, Jan 18, 2016 at 08:47:18PM +0100, Pablo Neira Ayuso wrote:
> > On Mon, Jan 18, 2016 at 11:27:13AM -0800, Munehisa Kamata wrote:
> > > Hi Greg,
> > > 
> > > This commit is already in Linus's tree. Could you please pick this up for 4.1.y?
> > 
> > I submitted this twice... Last time I made it was this one:
> > 
> > http://patchwork.ozlabs.org/patch/544964/
> > 
> > Previous was rejected because it was not in Linus tree:
> > 
> > http://patchwork.ozlabs.org/patch/540946/
> > 
> > But the last attempt got no feedback...
> 
> BTW, you can find all my -stable submissions here:
> 
> http://patchwork.ozlabs.org/project/netfilter-devel/list/?state=*&q=stable-

Relax, I am _way_ behind on stable patch work at the moment, should
hopefully dig out from the mess over the next week...

thanks,

greg k-h

[PATCH -stable,4.x.y] netfilter: nf_nat_redirect: add missing NULL pointer check

[Pablo Neira Ayuso]

From: Munehisa Kamata <kamatam@amazon.com>

[ upstream commit 94f9cd81436c85d8c3a318ba92e236ede73752fc ]

Commit 8b13eddfdf04cbfa561725cfc42d6868fe896f56 ("netfilter: refactor NAT
redirect IPv4 to use it from nf_tables") has introduced a trivial logic
change which can result in the following crash.

BUG: unable to handle kernel NULL pointer dereference at 0000000000000030
IP: [<ffffffffa033002d>] nf_nat_redirect_ipv4+0x2d/0xa0 [nf_nat_redirect]
PGD 3ba662067 PUD 3ba661067 PMD 0
Oops: 0000 [#1] SMP
Modules linked in: ipv6(E) xt_REDIRECT(E) nf_nat_redirect(E) xt_tcpudp(E) iptable_nat(E) nf_conntrack_ipv4(E) nf_defrag_ipv4(E) nf_nat_ipv4(E) nf_nat(E) nf_conntrack(E) ip_tables(E) x_tables(E) binfmt_misc(E) xfs(E) libcrc32c(E) evbug(E) evdev(E) psmouse(E) i2c_piix4(E) i2c_core(E) acpi_cpufreq(E) button(E) ext4(E) crc16(E) jbd2(E) mbcache(E) dm_mirror(E) dm_region_hash(E) dm_log(E) dm_mod(E)
CPU: 0 PID: 2536 Comm: ip Tainted: G            E   4.1.7-15.23.amzn1.x86_64 #1
Hardware name: Xen HVM domU, BIOS 4.2.amazon 05/06/2015
task: ffff8800eb438000 ti: ffff8803ba664000 task.ti: ffff8803ba664000
[...]
Call Trace:
 <IRQ>
 [<ffffffffa0334065>] redirect_tg4+0x15/0x20 [xt_REDIRECT]
 [<ffffffffa02e2e99>] ipt_do_table+0x2b9/0x5e1 [ip_tables]
 [<ffffffffa0328045>] iptable_nat_do_chain+0x25/0x30 [iptable_nat]
 [<ffffffffa031777d>] nf_nat_ipv4_fn+0x13d/0x1f0 [nf_nat_ipv4]
 [<ffffffffa0328020>] ? iptable_nat_ipv4_fn+0x20/0x20 [iptable_nat]
 [<ffffffffa031785e>] nf_nat_ipv4_in+0x2e/0x90 [nf_nat_ipv4]
 [<ffffffffa03280a5>] iptable_nat_ipv4_in+0x15/0x20 [iptable_nat]
 [<ffffffff81449137>] nf_iterate+0x57/0x80
 [<ffffffff814491f7>] nf_hook_slow+0x97/0x100
 [<ffffffff814504d4>] ip_rcv+0x314/0x400

unsigned int
nf_nat_redirect_ipv4(struct sk_buff *skb,
...
{
...
		rcu_read_lock();
		indev = __in_dev_get_rcu(skb->dev);
		if (indev != NULL) {
			ifa = indev->ifa_list;
			newdst = ifa->ifa_local; <---
		}
		rcu_read_unlock();
...
}

Before the commit, 'ifa' had been always checked before access. After the
commit, however, it could be accessed even if it's NULL. Interestingly,
this was once fixed in 2003.

http://marc.info/?l=netfilter-devel&m=106668497403047&w=2

In addition to the original one, we have seen the crash when packets that
need to be redirected somehow arrive on an interface which hasn't been
yet fully configured.

This change just reverts the logic to the old behavior to avoid the crash.

Fixes: 8b13eddfdf04 ("netfilter: refactor NAT redirect IPv4 to use it from nf_tables")
Cc: <stable@vger.kernel.org> # 4.1.x
Cc: <stable@vger.kernel.org> # 4.2.x
Cc: <stable@vger.kernel.org> # 4.3.x
Signed-off-by: Munehisa Kamata <kamatam@amazon.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_nat_redirect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nf_nat_redirect.c b/net/netfilter/nf_nat_redirect.c
index 97b75f9..d438698 100644
--- a/net/netfilter/nf_nat_redirect.c
+++ b/net/netfilter/nf_nat_redirect.c
@@ -55,7 +55,7 @@ nf_nat_redirect_ipv4(struct sk_buff *skb,
 
 		rcu_read_lock();
 		indev = __in_dev_get_rcu(skb->dev);
-		if (indev != NULL) {
+		if (indev && indev->ifa_list) {
 			ifa = indev->ifa_list;
 			newdst = ifa->ifa_local;
 		}
-- 
2.1.4