[ 3.5.yuz extended stable ] Patch "netfilter: nf_ct_expect: fix possible access to" has been added to staging queue

Herton Ronaldo Krzesinski herton.krzesinski at canonical.com
Tue Nov 13 20:35:01 UTC 2012

This is a note to let you know that I have just added a patch titled

    netfilter: nf_ct_expect: fix possible access to

to the linux-3.5.y-queue branch of the 3.5.yuz extended stable tree 
which can be found at:


If you, or anyone else, feels it should not be added to this tree, please 
reply to this email.

For more information about the 3.5.yuz tree, see



>From c732a539bc4a58f3ed5609e1db13734a50b93875 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo at netfilter.org>
Date: Thu, 16 Aug 2012 02:25:24 +0200
Subject: [PATCH] netfilter: nf_ct_expect: fix possible access to
 uninitialized timer

commit 2614f86490122bf51eb7c12ec73927f1900f4e7d upstream.

In __nf_ct_expect_check, the function refresh_timer returns 1
if a matching expectation is found and its timer is successfully
refreshed. This results in nf_ct_expect_related returning 0.
Note that at this point:

- the passed expectation is not inserted in the expectation table
  and its timer was not initialized, since we have refreshed one
  matching/existing expectation.

- nf_ct_expect_alloc uses kmem_cache_alloc, so the expectation
  timer is in some undefined state just after the allocation,
  until it is appropriately initialized.

This can be a problem for the SIP helper during the expectation

 if (nf_ct_expect_related(rtp_exp) == 0) {
         if (nf_ct_expect_related(rtcp_exp) != 0)

Note that nf_ct_expect_related(rtp_exp) may return 0 for the timer refresh
case that is detailed above. Then, if nf_ct_unexpect_related(rtcp_exp)
returns != 0, nf_ct_unexpect_related(rtp_exp) is called, which does:

 if (del_timer(&exp->timeout)) {

Note that del_timer always returns false if the timer has been
initialized.  However, the timer was not initialized since setup_timer
was not called, therefore, the expectation timer remains in some
undefined state. If I'm not missing anything, this may lead to the
removal an unexistent expectation.

To fix this, the optimization that allows refreshing an expectation
is removed. Now nf_conntrack_expect_related looks more consistent
to me since it always add the expectation in case that it returns

Thanks to Patrick McHardy for participating in the discussion of
this patch.

I think this may be the source of the problem described by:

Reported-by: Rafal Fitt <rafalf at aplusc.com.pl>
Acked-by: Patrick McHardy <kaber at trash.net>
Signed-off-by: Pablo Neira Ayuso <pablo at netfilter.org>
Acked-by: David S. Miller <davem at davemloft.net>
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski at canonical.com>
 net/netfilter/nf_conntrack_expect.c |   29 ++++++-----------------------
 1 file changed, 6 insertions(+), 23 deletions(-)

diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
index 45cf602..527651a 100644
--- a/net/netfilter/nf_conntrack_expect.c
+++ b/net/netfilter/nf_conntrack_expect.c
@@ -361,23 +361,6 @@ static void evict_oldest_expect(struct nf_conn *master,

-static inline int refresh_timer(struct nf_conntrack_expect *i)
-	struct nf_conn_help *master_help = nfct_help(i->master);
-	const struct nf_conntrack_expect_policy *p;
-	if (!del_timer(&i->timeout))
-		return 0;
-	p = &rcu_dereference_protected(
-		master_help->helper,
-		lockdep_is_held(&nf_conntrack_lock)
-		)->expect_policy[i->class];
-	i->timeout.expires = jiffies + p->timeout * HZ;
-	add_timer(&i->timeout);
-	return 1;
 static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
 	const struct nf_conntrack_expect_policy *p;
@@ -386,7 +369,7 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
 	struct nf_conn_help *master_help = nfct_help(master);
 	struct nf_conntrack_helper *helper;
 	struct net *net = nf_ct_exp_net(expect);
-	struct hlist_node *n;
+	struct hlist_node *n, *next;
 	unsigned int h;
 	int ret = 1;

@@ -395,12 +378,12 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
 		goto out;
 	h = nf_ct_expect_dst_hash(&expect->tuple);
-	hlist_for_each_entry(i, n, &net->ct.expect_hash[h], hnode) {
+	hlist_for_each_entry_safe(i, n, next, &net->ct.expect_hash[h], hnode) {
 		if (expect_matches(i, expect)) {
-			/* Refresh timer: if it's dying, ignore.. */
-			if (refresh_timer(i)) {
-				ret = 0;
-				goto out;
+			if (del_timer(&i->timeout)) {
+				nf_ct_unlink_expect(i);
+				nf_ct_expect_put(i);
+				break;
 		} else if (expect_clash(i, expect)) {
 			ret = -EBUSY;

More information about the kernel-team mailing list