[PATCH] netfilter: nf_ct_expect: fix possible access to uninitialized timer

[PATCH] netfilter: nf_ct_expect: fix possible access to uninitialized timer

[pablo]

From: Pablo Neira Ayuso <pablo@netfilter.org>

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

 ...
 if (nf_ct_expect_related(rtp_exp) == 0) {
         if (nf_ct_expect_related(rtcp_exp) != 0)
                 nf_ct_unexpect_related(rtp_exp);
 ...

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:

 spin_lock_bh(&nf_conntrack_lock);
 if (del_timer(&exp->timeout)) {
         nf_ct_unlink_expect(exp);
         nf_ct_expect_put(exp);
 }
 spin_unlock_bh(&nf_conntrack_lock);

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.

I think this can be the source of the problem described by:
http://marc.info/?l=netfilter-devel&m=134073514719421&w=2

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_conntrack_expect.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
index 45cf602..b16e70d 100644
--- a/net/netfilter/nf_conntrack_expect.c
+++ b/net/netfilter/nf_conntrack_expect.c
@@ -436,6 +436,13 @@ int nf_ct_expect_related_report(struct nf_conntrack_expect *expect,
 {
 	int ret;
 
+	/* Make sure that nf_ct_unexpect_related always gets an initialized
+	 * timer for the case in which one matching expectation is refreshed
+	 * (and thus, this expectation is not inserted).
+	 */
+	setup_timer(&exp->timeout, nf_ct_expectation_timed_out,
+		    (unsigned long)exp);
+
 	spin_lock_bh(&nf_conntrack_lock);
 	ret = __nf_ct_expect_check(expect);
 	if (ret <= 0)
-- 
1.7.10.4

Re: [PATCH] netfilter: nf_ct_expect: fix possible access to uninitialized timer

[Patrick McHardy]

On Wed, 15 Aug 2012, pablo@netfilter.org wrote:

> From: Pablo Neira Ayuso <pablo@netfilter.org>
>
> 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
> addition:
>
> ...
> if (nf_ct_expect_related(rtp_exp) == 0) {
>         if (nf_ct_expect_related(rtcp_exp) != 0)
>                 nf_ct_unexpect_related(rtp_exp);
> ...
>
> 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:
>
> spin_lock_bh(&nf_conntrack_lock);
> if (del_timer(&exp->timeout)) {
>         nf_ct_unlink_expect(exp);
>         nf_ct_expect_put(exp);
> }
> spin_unlock_bh(&nf_conntrack_lock);
>
> 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.
>
> I think this can be the source of the problem described by:
> http://marc.info/?l=netfilter-devel&m=134073514719421&w=2

OK, so we assume del_timer returned success since otherwise this
would have no effect. This means that detach_timer() was called
and does a

__list_del(entry->prev, entry->next);
entry->prev = LIST_POISON2;

If the expectation from the slab was just uninitialized memory, it
would very likely crash in __list_del(). But even if the memory was 
reused, it would still crash since entry->prev would be set to
LIST_POISON2.

The same applies to the hlist_del/hlist_del_rcu calls in
nf_ct_unlink_expect_report().

So you fix a real bug, but I don't see how it can explain that report.

> diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
> index 45cf602..b16e70d 100644
> --- a/net/netfilter/nf_conntrack_expect.c
> +++ b/net/netfilter/nf_conntrack_expect.c
> @@ -436,6 +436,13 @@ int nf_ct_expect_related_report(struct nf_conntrack_expect *expect,
> {
> 	int ret;
>
> +	/* Make sure that nf_ct_unexpect_related always gets an initialized
> +	 * timer for the case in which one matching expectation is refreshed
> +	 * (and thus, this expectation is not inserted).
> +	 */
> +	setup_timer(&exp->timeout, nf_ct_expectation_timed_out,
> +		    (unsigned long)exp);
> +

We're setting the timer up twice now. I'd suggest to just do it once,
either in nf_ct_expect_alloc() or nf_ct_expect_init().

Once question remains though - if the scenario you describe happens and
we're just refreshing an existing expectation, should that one actually
get unexpected by the nf_ct_unexpect_related() call?

The intention is to remove the expectation with that tuple, the refreshing
is just an optimization, so I think that would make sense.

> 	spin_lock_bh(&nf_conntrack_lock);
> 	ret = __nf_ct_expect_check(expect);
> 	if (ret <= 0)
> -- 
> 1.7.10.4
>

Re: [PATCH] netfilter: nf_ct_expect: fix possible access to uninitialized timer

[Pablo Neira Ayuso]

On Wed, Aug 15, 2012 at 03:25:39AM +0200, Patrick McHardy wrote:
> On Wed, 15 Aug 2012, pablo@netfilter.org wrote:
> 
> >From: Pablo Neira Ayuso <pablo@netfilter.org>
> >
> >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
> >addition:
> >
> >...
> >if (nf_ct_expect_related(rtp_exp) == 0) {
> >        if (nf_ct_expect_related(rtcp_exp) != 0)
> >                nf_ct_unexpect_related(rtp_exp);
> >...
> >
> >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:
> >
> >spin_lock_bh(&nf_conntrack_lock);
> >if (del_timer(&exp->timeout)) {
> >        nf_ct_unlink_expect(exp);
> >        nf_ct_expect_put(exp);
> >}
> >spin_unlock_bh(&nf_conntrack_lock);
> >
> >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.
> >
> >I think this can be the source of the problem described by:
> >http://marc.info/?l=netfilter-devel&m=134073514719421&w=2
> 
> OK, so we assume del_timer returned success since otherwise this
> would have no effect. This means that detach_timer() was called
> and does a
> 
> __list_del(entry->prev, entry->next);
> entry->prev = LIST_POISON2;
>
> If the expectation from the slab was just uninitialized memory, it
> would very likely crash in __list_del(). But even if the memory was
> reused, it would still crash since entry->prev would be set to
> LIST_POISON2.
>
> The same applies to the hlist_del/hlist_del_rcu calls in
> nf_ct_unlink_expect_report().
> 
> So you fix a real bug, but I don't see how it can explain that report.

The user reports crashes in flush_expectation and soft lockups while
in nf_conntrack_expect_related, the latter involves some access to
LIST_POISON1 memory address.

I've spent quite some time in front of this code, this is what I've
found so far. I've passed him a new version of this patch, let's see
what he reports back.

> >diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
> >index 45cf602..b16e70d 100644
> >--- a/net/netfilter/nf_conntrack_expect.c
> >+++ b/net/netfilter/nf_conntrack_expect.c
> >@@ -436,6 +436,13 @@ int nf_ct_expect_related_report(struct nf_conntrack_expect *expect,
> >{
> >	int ret;
> >
> >+	/* Make sure that nf_ct_unexpect_related always gets an initialized
> >+	 * timer for the case in which one matching expectation is refreshed
> >+	 * (and thus, this expectation is not inserted).
> >+	 */
> >+	setup_timer(&exp->timeout, nf_ct_expectation_timed_out,
> >+		    (unsigned long)exp);
> >+
> 
> We're setting the timer up twice now. I'd suggest to just do it once,
> either in nf_ct_expect_alloc() or nf_ct_expect_init().

Yes, I'll move it to nf_ct_expect_init.

> Once question remains though - if the scenario you describe happens and
> we're just refreshing an existing expectation, should that one actually
> get unexpected by the nf_ct_unexpect_related() call?
> 
> The intention is to remove the expectation with that tuple, the refreshing
> is just an optimization, so I think that would make sense.

Yes, that's another possibility that look better to me as the expect
object would be always inserted.

Re: [PATCH] netfilter: nf_ct_expect: fix possible access to uninitialized timer

[Patrick McHardy]

On Wed, 15 Aug 2012, Pablo Neira Ayuso wrote:

> On Wed, Aug 15, 2012 at 03:25:39AM +0200, Patrick McHardy wrote:
>> On Wed, 15 Aug 2012, pablo@netfilter.org wrote:
>>
>>> From: Pablo Neira Ayuso <pablo@netfilter.org>
>>>
>>> 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
>>> addition:
>>>
>>> ...
>>> if (nf_ct_expect_related(rtp_exp) == 0) {
>>>        if (nf_ct_expect_related(rtcp_exp) != 0)
>>>                nf_ct_unexpect_related(rtp_exp);
>>> ...
>>>
>>> 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:
>>>
>>> spin_lock_bh(&nf_conntrack_lock);
>>> if (del_timer(&exp->timeout)) {
>>>        nf_ct_unlink_expect(exp);
>>>        nf_ct_expect_put(exp);
>>> }
>>> spin_unlock_bh(&nf_conntrack_lock);
>>>
>>> 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.
>>>
>>> I think this can be the source of the problem described by:
>>> http://marc.info/?l=netfilter-devel&m=134073514719421&w=2
>>
>> OK, so we assume del_timer returned success since otherwise this
>> would have no effect. This means that detach_timer() was called
>> and does a
>>
>> __list_del(entry->prev, entry->next);
>> entry->prev = LIST_POISON2;
>>
>> If the expectation from the slab was just uninitialized memory, it
>> would very likely crash in __list_del(). But even if the memory was
>> reused, it would still crash since entry->prev would be set to
>> LIST_POISON2.
>>
>> The same applies to the hlist_del/hlist_del_rcu calls in
>> nf_ct_unlink_expect_report().
>>
>> So you fix a real bug, but I don't see how it can explain that report.
>
> The user reports crashes in flush_expectation and soft lockups while
> in nf_conntrack_expect_related, the latter involves some access to
> LIST_POISON1 memory address.
>
> I've spent quite some time in front of this code, this is what I've
> found so far. I've passed him a new version of this patch, let's see
> what he reports back.

Sure. I've had a long look myself, but couldn't find a reason for the
problems he reported so far.

>>> diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
>>> index 45cf602..b16e70d 100644
>>> --- a/net/netfilter/nf_conntrack_expect.c
>>> +++ b/net/netfilter/nf_conntrack_expect.c
>>> @@ -436,6 +436,13 @@ int nf_ct_expect_related_report(struct nf_conntrack_expect *expect,
>>> {
>>> 	int ret;
>>>
>>> +	/* Make sure that nf_ct_unexpect_related always gets an initialized
>>> +	 * timer for the case in which one matching expectation is refreshed
>>> +	 * (and thus, this expectation is not inserted).
>>> +	 */
>>> +	setup_timer(&exp->timeout, nf_ct_expectation_timed_out,
>>> +		    (unsigned long)exp);
>>> +
>>
>> We're setting the timer up twice now. I'd suggest to just do it once,
>> either in nf_ct_expect_alloc() or nf_ct_expect_init().
>
> Yes, I'll move it to nf_ct_expect_init.
>
>> Once question remains though - if the scenario you describe happens and
>> we're just refreshing an existing expectation, should that one actually
>> get unexpected by the nf_ct_unexpect_related() call?
>>
>> The intention is to remove the expectation with that tuple, the refreshing
>> is just an optimization, so I think that would make sense.
>
> Yes, that's another possibility that look better to me as the expect
> object would be always inserted.

So we'd just remove the refreshing, kill the old expectation and insert
the new one instead?

Re: [PATCH] netfilter: nf_ct_expect: fix possible access to uninitialized timer

[Pablo Neira Ayuso]

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

On Thu, Aug 16, 2012 at 01:15:24AM +0200, Patrick McHardy wrote:
[...]
> >>Once question remains though - if the scenario you describe happens and
> >>we're just refreshing an existing expectation, should that one actually
> >>get unexpected by the nf_ct_unexpect_related() call?
> >>
> >>The intention is to remove the expectation with that tuple, the refreshing
> >>is just an optimization, so I think that would make sense.
> >
> >Yes, that's another possibility that look better to me as the expect
> >object would be always inserted.
> 
> So we'd just remove the refreshing, kill the old expectation and insert
> the new one instead?

Agreed. Patch attached.

[-- Attachment #2: 0001-netfilter-nf_ct_expect-fix-possible-access-to-uninit.patch --]
[-- Type: text/x-diff, Size: 4143 bytes --]

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

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

 ...
 if (nf_ct_expect_related(rtp_exp) == 0) {
         if (nf_ct_expect_related(rtcp_exp) != 0)
                 nf_ct_unexpect_related(rtp_exp);
 ...

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:

 spin_lock_bh(&nf_conntrack_lock);
 if (del_timer(&exp->timeout)) {
         nf_ct_unlink_expect(exp);
         nf_ct_expect_put(exp);
 }
 spin_unlock_bh(&nf_conntrack_lock);

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

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

I think this may be the source of the problem described by:
http://marc.info/?l=netfilter-devel&m=134073514719421&w=2

Reported-by: Rafal Fitt <rafalf@aplusc.com.pl>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 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;
-- 
1.7.10.4

Re: [PATCH] netfilter: nf_ct_expect: fix possible access to uninitialized timer

[Patrick McHardy]

On Thu, 16 Aug 2012, Pablo Neira Ayuso wrote:

> On Thu, Aug 16, 2012 at 01:15:24AM +0200, Patrick McHardy wrote:
> [...]
>>>> Once question remains though - if the scenario you describe happens and
>>>> we're just refreshing an existing expectation, should that one actually
>>>> get unexpected by the nf_ct_unexpect_related() call?
>>>>
>>>> The intention is to remove the expectation with that tuple, the refreshing
>>>> is just an optimization, so I think that would make sense.
>>>
>>> Yes, that's another possibility that look better to me as the expect
>>> object would be always inserted.
>>
>> So we'd just remove the refreshing, kill the old expectation and insert
>> the new one instead?
>
> Agreed. Patch attached.
>

Looks fine. We probably still have another bug though, will have another
look tommorrow.

Acked-by: Patrick McHardy <kaber@trash.net>