Patchwork [2/2] netfilter: nf_ct_expect: fix possible invalid dereference while event reporting

login
register
mail settings
Submitter Pablo Neira
Date Aug. 9, 2012, 11:02 p.m.
Message ID <1344553332-8536-3-git-send-email-pablo@netfilter.org>
Download mbox | patch
Permalink /patch/176297/
State Rejected
Headers show

Comments

Pablo Neira - Aug. 9, 2012, 11:02 p.m.
From: Pablo Neira Ayuso <pablo@netfilter.org>

Bump expectation refcount to make sure it does not vanish while
reporting the event via ctnetlink. One user reported a crash
while on nf_ct_expect_related_report triggered by the SIP helper.

Reported-by: Rafal Fitt <rafalf@aplusc.com.pl>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_conntrack_expect.c |    4 ++++
 1 file changed, 4 insertions(+)
Patrick McHardy - Aug. 9, 2012, 11:34 p.m.
On Fri, 10 Aug 2012, pablo@netfilter.org wrote:

> From: Pablo Neira Ayuso <pablo@netfilter.org>
>
> Bump expectation refcount to make sure it does not vanish while
> reporting the event via ctnetlink. One user reported a crash
> while on nf_ct_expect_related_report triggered by the SIP helper.
>
> Reported-by: Rafal Fitt <rafalf@aplusc.com.pl>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

I don't think this can be correct. The nf_ct_expect_related_report()
call is usually done through the nf_ct_expect_related() call chain,
in which case the helper still has one refcount for its one reference,
so the helper can't be destroyed at this point. Even if another CPU
tries to remove it again, it will just release the reference of the
expectation hash and not the one the helper is holding.

Am I missing something here?

> ---
> net/netfilter/nf_conntrack_expect.c |    4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
> index ec8bb0d..d5fccd3 100644
> --- a/net/netfilter/nf_conntrack_expect.c
> +++ b/net/netfilter/nf_conntrack_expect.c
> @@ -444,8 +444,12 @@ int nf_ct_expect_related_report(struct nf_conntrack_expect *expect,
> 	ret = nf_ct_expect_insert(expect);
> 	if (ret < 0)
> 		goto out;
> +
> +	atomic_inc(&expect->use);
> 	spin_unlock_bh(&nf_conntrack_lock);
> 	nf_ct_expect_event_report(IPEXP_NEW, expect, pid, report);
> +	nf_ct_expect_put(expect);
> +
> 	return ret;
> out:
> 	spin_unlock_bh(&nf_conntrack_lock);
> -- 
> 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

Patch

diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
index ec8bb0d..d5fccd3 100644
--- a/net/netfilter/nf_conntrack_expect.c
+++ b/net/netfilter/nf_conntrack_expect.c
@@ -444,8 +444,12 @@  int nf_ct_expect_related_report(struct nf_conntrack_expect *expect,
 	ret = nf_ct_expect_insert(expect);
 	if (ret < 0)
 		goto out;
+
+	atomic_inc(&expect->use);
 	spin_unlock_bh(&nf_conntrack_lock);
 	nf_ct_expect_event_report(IPEXP_NEW, expect, pid, report);
+	nf_ct_expect_put(expect);
+
 	return ret;
 out:
 	spin_unlock_bh(&nf_conntrack_lock);