Patchwork netfilter: nf_ct_expect: fix possible access to uninitialized timer

login
register
mail settings
Submitter Pablo Neira
Date Aug. 15, 2012, 12:43 a.m.
Message ID <1344991422-2514-1-git-send-email-pablo@netfilter.org>
Download mbox | patch
Permalink /patch/177524/
State Superseded
Headers show

Comments

Pablo Neira - Aug. 15, 2012, 12:43 a.m.
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(+)
Patrick McHardy - Aug. 15, 2012, 1:25 a.m.
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
>
--
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
Pablo Neira - Aug. 15, 2012, 5:14 p.m.
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.
--
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
Patrick McHardy - Aug. 15, 2012, 11:15 p.m.
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?
--
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

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)