netdev - Re: Oops in nf_nat_core.c:find_appropriate

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 10 Jun 2008 15:56:04 +0200 (CEST)
From:	Krzysztof Oledzki <ole@....pl>
To:	Patrick McHardy <kaber@...sh.net>
cc:	Chuck Ebbert <cebbert@...hat.com>, Netdev <netdev@...r.kernel.org>,
	Netfilter Development Mailinglist 
	<netfilter-devel@...r.kernel.org>
Subject: Re: Oops in nf_nat_core.c:find_appropriate_src(), kernel 2.6.25.4



On Tue, 10 Jun 2008, Patrick McHardy wrote:

> Krzysztof Oledzki wrote:
>> On Sat, 7 Jun 2008, Patrick McHardy wrote:
>> 
>>> Patrick McHardy wrote:
>>>> Chuck Ebbert wrote:
>>>>> Reported at https://bugzilla.redhat.com/show_bug.cgi?id=449315
>>>>> 
>>>>> In find_appropriate_src():
>>>>>
>>>>>         hlist_for_each_entry_rcu(nat, n, &bysource[h], bysource) {
>>>>>                 ct = nat->ct;
>>>>>                 if (same_src(ct, tuple)) {
>>>>> 
>>>>> Dereference of ct in same_src() causes the oops. This only seems to
>>>>> happen on heavily loaded firewall machines. Kernel 2.6.24.7 works.
>>>>> 
>>>>> The reporter identifies commit 4d354c5782dc352cec187845d17eedc2c2bfcf67
>>>>> ("[NETFILTER]: nf_nat: use RCU for bysource hash") as a possible cause
>>>>> of the problem.
>>>> 
>>>> We have a similar looking report, but that one also affects 2.6.24:
>>>> 
>>>> http://bugzilla.kernel.org/show_bug.cgi?id=10875
>
>
> We found the reason for that crash and I've queued these two
> patches. Please let me know whether they also fix the problem
> from the redhat bugzilla.

There is a one thing that still bugs me. This patch removes setting the 
nat->ct pointer to NULL:

--- a/net/ipv4/netfilter/nf_nat_core.c
+++ b/net/ipv4/netfilter/nf_nat_core.c
@@ -556,7 +556,6 @@ static void nf_nat_cleanup_conntrack(struct nf_conn 
*ct)

         spin_lock_bh(&nf_nat_lock);
         hlist_del_rcu(&nat->bysource);
-       nat->ct = NULL;
         spin_unlock_bh(&nf_nat_lock);
  }

After this patch the whole function looks like this:

/* Noone using conntrack by the time this called. */
static void nf_nat_cleanup_conntrack(struct nf_conn *ct)
{
         struct nf_conn_nat *nat = nf_ct_ext_find(ct, NF_CT_EXT_NAT);

         if (nat == NULL || nat->ct == NULL)
                 return;

         NF_CT_ASSERT(nat->ct->status & IPS_NAT_DONE_MASK);

         spin_lock_bh(&nf_nat_lock);
         hlist_del_rcu(&nat->bysource);
         spin_unlock_bh(&nf_nat_lock);
}

As you can see we still check if nat->ct is NULL here. So, or the check is 
now unnecessary, or it is still possible that nat->ct may become NULL. If 
the second statement is true than we may need to check ct before calling 
same_src in the find_appropriate_src function.

Best regards,

 			Krzysztof Olędzki