[PATCH nf 1/1] netfilter: cttimeout: Fix one possible use-after-free issue

[PATCH nf 1/1] netfilter: cttimeout: Fix one possible use-after-free issue

[gfree.wind]

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

Signed-off-by: Gao Feng <fgao@ikuai8.com>
---
 net/netfilter/nfnetlink_cttimeout.c | 5 +++++
 1 file changed, 5 insertions(+)

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);
 
-- 
1.9.1

Re: [PATCH nf 1/1] netfilter: cttimeout: Fix one possible use-after-free issue

[Pablo Neira Ayuso]

On Thu, Apr 06, 2017 at 07:09:09PM +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

This object is released via kfree_rcu().

You have to describe better the race scenario.

RE: [PATCH nf 1/1] netfilter: cttimeout: Fix one possible use-after-free issue

[Gao Feng]

> -----Original Message-----
> From: Pablo Neira Ayuso [mailto:pablo@netfilter.org]
> On Thu, Apr 06, 2017 at 07:09:09PM +0800, gfree.wind@foxmail.com wrote:
> >
> > 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
> 
> This object is released via kfree_rcu().
> 
> You have to describe better the race scenario.

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

Regards
Feng

Re: [PATCH nf 1/1] netfilter: cttimeout: Fix one possible use-after-free issue

[Pablo Neira Ayuso]

On Fri, Apr 14, 2017 at 06:35:13AM +0800, Gao Feng wrote:
> > -----Original Message-----
> > From: Pablo Neira Ayuso [mailto:pablo@netfilter.org]
> > On Thu, Apr 06, 2017 at 07:09:09PM +0800, gfree.wind@foxmail.com wrote:
> > >
> > > 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
> > 
> > This object is released via kfree_rcu().
> > 
> > You have to describe better the race scenario.
> 
> 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, 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.

You add this to your patch description and resubmit.

Please, send me one patch or two maximum at a time. Until I don't
apply one, you don't send me a new one.