[nf,v2] netfilter: cttimeout: Fix one possible use-after-free issue

Submitted by Gao Feng on April 14, 2017, 1:13 a.m.

Details

Message ID 1492132420-70400-1-git-send-email-gfree.wind@foxmail.com
State Not Applicable
Delegated to: Pablo Neira
Headers show

Commit Message

Gao Feng April 14, 2017, 1:13 a.m.
From: Gao Feng <fgao@ikuai8.com>

The function ctnl_untimeout is used to untimeout every conntrack
which is using the timeout. But it is necessary to add one barrier
synchronize_rcu because of racing. Maybe one conntrack has already
owned this timeout, but it is not inserted into unconfirmed list or
the hash list, when ctnl_untimeout untimeout the conntracks

Let me describe it with a call path
CPU1				CPU2
alloc new conn
add timeout ext
				ctnl_timeout_try_del
				untimeout all conns in list
				kfree_rcu.
conn is confirmed.

As the show above, when cpu2 untimeout all conns in list and the new
conn of cpu1 is not confirmed, the new conn still owns the timeout
pointer. After the timeout mem is freed really, it points to one
invalid mem.

Signed-off-by: Gao Feng <fgao@ikuai8.com>
---
 v2: Add the call path in the comment
 v1: initial version

 net/netfilter/nfnetlink_cttimeout.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Pablo Neira April 14, 2017, 1:28 a.m.
On Fri, Apr 14, 2017 at 09:13:40AM +0800, gfree.wind@foxmail.com wrote:
> From: Gao Feng <fgao@ikuai8.com>
> 
> The function ctnl_untimeout is used to untimeout every conntrack
> which is using the timeout. But it is necessary to add one barrier
> synchronize_rcu because of racing. Maybe one conntrack has already
> owned this timeout, but it is not inserted into unconfirmed list or
> the hash list, when ctnl_untimeout untimeout the conntracks
> 
> Let me describe it with a call path
> CPU1				CPU2
> alloc new conn
> add timeout ext
> 				ctnl_timeout_try_del
> 				untimeout all conns in list
> 				kfree_rcu.
> conn is confirmed.

Then this confirmed conn gets a timeout_ext->timeout == NULL since
ctnl_untimeout() is called first.
--
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
Gao Feng April 14, 2017, 2:13 a.m.
> From: Pablo Neira Ayuso [mailto:pablo@netfilter.org]
> On Fri, Apr 14, 2017 at 09:13:40AM +0800, gfree.wind@foxmail.com wrote:
> > From: Gao Feng <fgao@ikuai8.com>
> >
> > The function ctnl_untimeout is used to untimeout every conntrack which
> > is using the timeout. But it is necessary to add one barrier
> > synchronize_rcu because of racing. Maybe one conntrack has already
> > owned this timeout, but it is not inserted into unconfirmed list or
> > the hash list, when ctnl_untimeout untimeout the conntracks
> >
> > Let me describe it with a call path
> > CPU1				CPU2
> > alloc new conn
> > add timeout ext
> > 				ctnl_timeout_try_del
> > 				untimeout all conns in list
> > 				kfree_rcu.
> > conn is confirmed.
> 
> Then this confirmed conn gets a timeout_ext->timeout == NULL since
> ctnl_untimeout() is called first.

The new conn gets the timeout_ext->timeout before ctnl_untimeout, when add
the timeout ext.
Then ctnl_untimeout happens before the new conn is inserted into unconfirmed
list.
It could not reset the timeout pointer of new conn to NULL.
Maybe I should add the "insert unconfirm list" in the call path.

I could not understand why the conn gets one NULL pointer, not invalid
pointer in this case.

Regards
Feng




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

Patch hide | download patch | download mbox

diff --git a/net/netfilter/nfnetlink_cttimeout.c b/net/netfilter/nfnetlink_cttimeout.c
index 47d6656..af0cc87 100644
--- a/net/netfilter/nfnetlink_cttimeout.c
+++ b/net/netfilter/nfnetlink_cttimeout.c
@@ -304,6 +304,11 @@  static void ctnl_untimeout(struct net *net, struct ctnl_timeout *timeout)
 	spinlock_t *lock;
 	int i, cpu;
 
+	/* Make sure the conntrack using the timeout already in the unconfirmed
+	 * list or in the hash table.
+	 */
+	synchronize_rcu();
+
 	for_each_possible_cpu(cpu) {
 		struct ct_pcpu *pcpu = per_cpu_ptr(net->ct.pcpu_lists, cpu);