Still oopsing in nf_nat_move_storage()

Still oopsing in nf_nat_move_storage()

[Chuck Ebbert]

nf_nat_move_storage():
/usr/src/debug/kernel-2.6.23/linux-2.6.23.i686/net/ipv4/netfilter/nf_nat_core.c:612
      87:       f7 47 64 80 01 00 00    testl  $0x180,0x64(%edi)
      8e:       74 39                   je     c9 <nf_nat_move_storage+0x65>

line 612:
        if (!(ct->status & IPS_NAT_DONE_MASK))
                return;

ct is NULL



To reproduce, this is the Fedora iptables config file:

*nat
:PREROUTING ACCEPT [0:0]
:OUTPUT ACCEPT [0:0]
:POSTROUTING ACCEPT [0:0]
-A PREROUTING -i eth0 -p tcp --dport 80 -j DNAT --to :21
COMMIT

*filter
:INPUT ACCEPT [0:0]
:FORWARD ACCEPT [0:0]
:OUTPUT ACCEPT [0:0]
-A INPUT -p icmp --icmp-type any -j ACCEPT
-A INPUT -i lo -j ACCEPT
-A INPUT -i eth0 -m state --state NEW -m tcp -p tcp --dport 21 -j ACCEPT
-A INPUT -m state --state ESTABLISHED,RELATED -j ACCEPT
-A INPUT -m state --state NEW -m tcp -p tcp --dport 22 -j ACCEPT
-A INPUT -j REJECT --reject-with icmp-host-prohibited
-A FORWARD -j REJECT --reject-with icmp-host-prohibited
COMMIT

Re: Still oopsing in nf_nat_move_storage()

[Patrick McHardy]

Chuck Ebbert wrote:
> nf_nat_move_storage():
> /usr/src/debug/kernel-2.6.23/linux-2.6.23.i686/net/ipv4/netfilter/nf_nat_core.c:612
>       87:       f7 47 64 80 01 00 00    testl  $0x180,0x64(%edi)
>       8e:       74 39                   je     c9 <nf_nat_move_storage+0x65>
> 
> line 612:
>         if (!(ct->status & IPS_NAT_DONE_MASK))
>                 return;
> 
> ct is NULL

The current kernel (and 2.6.23-stable) have:

         if (!ct || !(ct->status & IPS_NAT_DONE_MASK))
                 return;

so it seems you're using an old version.

Re: Still oopsing in nf_nat_move_storage()

[Chuck Ebbert]

On 01/29/2008 12:18 PM, Patrick McHardy wrote:
> Chuck Ebbert wrote:
>> nf_nat_move_storage():
>> /usr/src/debug/kernel-2.6.23/linux-2.6.23.i686/net/ipv4/netfilter/nf_nat_core.c:612
>>
>>       87:       f7 47 64 80 01 00 00    testl  $0x180,0x64(%edi)
>>       8e:       74 39                   je     c9
>> <nf_nat_move_storage+0x65>
>>
>> line 612:
>>         if (!(ct->status & IPS_NAT_DONE_MASK))
>>                 return;
>>
>> ct is NULL
> 
> The current kernel (and 2.6.23-stable) have:
> 
>         if (!ct || !(ct->status & IPS_NAT_DONE_MASK))
>                 return;
> 
> so it seems you're using an old version.

Sorry, I re-used the analysis from before that change went in. I now
have an oops report from 2.6.23.14 on x86_64.

It is oopsing there, and only on x86_64 now, because x86_64 refuses to
use a non-canonical address. ct contains what appears to be ASCII data.
i386 might be dereferencing some random address instead of oopsing...


   0:   48 f7 45 78 80 01 00    testq  $0x180,0x78(%rbp)
   7:   00
   8:   74 4c                   je     0x56
   a:   48 c7 c7 e0 18 28 88    mov    $0xffffffff882818e0,%rdi

%rbp has a bogus (non-canonical) address. On i386 there is no such test possible
so it will just dereference the address if it is mapped.

%rbp contains 8 valid ASCII chars: "salcf x\"

Re: Still oopsing in nf_nat_move_storage()

[Chuck Ebbert]

On 01/31/2008 01:03 PM, Chuck Ebbert wrote:
> On 01/29/2008 12:18 PM, Patrick McHardy wrote:
>> Chuck Ebbert wrote:
>>> nf_nat_move_storage():
>>> /usr/src/debug/kernel-2.6.23/linux-2.6.23.i686/net/ipv4/netfilter/nf_nat_core.c:612
>>>
>>>       87:       f7 47 64 80 01 00 00    testl  $0x180,0x64(%edi)
>>>       8e:       74 39                   je     c9
>>> <nf_nat_move_storage+0x65>
>>>
>>> line 612:
>>>         if (!(ct->status & IPS_NAT_DONE_MASK))
>>>                 return;
>>>
>>> ct is NULL
>> The current kernel (and 2.6.23-stable) have:
>>
>>         if (!ct || !(ct->status & IPS_NAT_DONE_MASK))
>>                 return;
>>
>> so it seems you're using an old version.

So, it is now oopsing after the test for NULL and only x86_64 is
catching the invalid address because it is non-canonical. Checking
for NULL is obviously not enough...

Re: Still oopsing in nf_nat_move_storage()

[Patrick McHardy]

Chuck Ebbert wrote:
> On 01/31/2008 01:03 PM, Chuck Ebbert wrote:
>> On 01/29/2008 12:18 PM, Patrick McHardy wrote:
>>> Chuck Ebbert wrote:
>>>> nf_nat_move_storage():
>>>> /usr/src/debug/kernel-2.6.23/linux-2.6.23.i686/net/ipv4/netfilter/nf_nat_core.c:612
>>>>
>>>>       87:       f7 47 64 80 01 00 00    testl  $0x180,0x64(%edi)
>>>>       8e:       74 39                   je     c9
>>>> <nf_nat_move_storage+0x65>
>>>>
>>>> line 612:
>>>>         if (!(ct->status & IPS_NAT_DONE_MASK))
>>>>                 return;
>>>>
>>>> ct is NULL
>>> The current kernel (and 2.6.23-stable) have:
>>>
>>>         if (!ct || !(ct->status & IPS_NAT_DONE_MASK))
>>>                 return;
>>>
>>> so it seems you're using an old version.
> 
> So, it is now oopsing after the test for NULL and only x86_64 is
> catching the invalid address because it is non-canonical. Checking
> for NULL is obviously not enough...


The addresses passed to ->move seems to be bogus, we're doing:

           t->move(ct, ct->ext + ct->ext->offset[i]);

without assigning the new ct->ext first, which is wrong for
two reasons:

- the new ext hasn't been assigned to the conntrack yet,
   so its moving within the same extension

- ct->ext + ct->ext->offset[i] should be (void *)ct->ext + ...

I'll fix it and send a patch after some testing. Still wondering
why this wasn't noticed before.

Re: Still oopsing in nf_nat_move_storage()

[Patrick McHardy]

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

Chuck Ebbert wrote:
> On 01/31/2008 01:03 PM, Chuck Ebbert wrote:
>> On 01/29/2008 12:18 PM, Patrick McHardy wrote:
>>> Chuck Ebbert wrote:
>>>> nf_nat_move_storage():
>>>> /usr/src/debug/kernel-2.6.23/linux-2.6.23.i686/net/ipv4/netfilter/nf_nat_core.c:612
>>>>
>>>>       87:       f7 47 64 80 01 00 00    testl  $0x180,0x64(%edi)
>>>>       8e:       74 39                   je     c9
>>>> <nf_nat_move_storage+0x65>
>>>>
>>>> line 612:
>>>>         if (!(ct->status & IPS_NAT_DONE_MASK))
>>>>                 return;
>>>>
>>>> ct is NULL
>>> The current kernel (and 2.6.23-stable) have:
>>>
>>>         if (!ct || !(ct->status & IPS_NAT_DONE_MASK))
>>>                 return;
>>>
>>> so it seems you're using an old version.
> 
> So, it is now oopsing after the test for NULL and only x86_64 is
> catching the invalid address because it is non-canonical. Checking
> for NULL is obviously not enough...


Could you try whether this patch fixes it please?


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 2518 bytes --]

commit 6953954cc566c19a84b7ca9647c16dabe4646c03
Author: Patrick McHardy <kaber@trash.net>
Date:   Sat Feb 2 12:01:03 2008 +0100

    [NETFILTER]: nf_conntrack: fix ct_extend ->move operation
    
    The ->move operation has two bugs:
    
    - It is called with the same extension as source and destination,
      so it doesn't update the new extension.
    
    - The address of the old extension is calculated incorrectly,
      instead of (void *)ct->ext + ct->ext->offset[i] it uses
      ct->ext + ct->ext->offset[i].
    
    Should fix a crash on x86_64 reported by Chuck Ebbert <cebbert@redhat.com>
    and Thomas Woerner <twoerner@redhat.com>.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/include/net/netfilter/nf_conntrack_extend.h b/include/net/netfilter/nf_conntrack_extend.h
index 73b5711..49aac63 100644
--- a/include/net/netfilter/nf_conntrack_extend.h
+++ b/include/net/netfilter/nf_conntrack_extend.h
@@ -67,7 +67,7 @@ struct nf_ct_ext_type
 	void (*destroy)(struct nf_conn *ct);
 	/* Called when realloacted (can be NULL).
 	   Contents has already been moved. */
-	void (*move)(struct nf_conn *ct, void *old);
+	void (*move)(void *new, void *old);
 
 	enum nf_ct_ext_id id;
 
diff --git a/net/ipv4/netfilter/nf_nat_core.c b/net/ipv4/netfilter/nf_nat_core.c
index dd07362..0d5fa3a 100644
--- a/net/ipv4/netfilter/nf_nat_core.c
+++ b/net/ipv4/netfilter/nf_nat_core.c
@@ -600,10 +600,10 @@ static void nf_nat_cleanup_conntrack(struct nf_conn *ct)
 	spin_unlock_bh(&nf_nat_lock);
 }
 
-static void nf_nat_move_storage(struct nf_conn *conntrack, void *old)
+static void nf_nat_move_storage(void *new, void *old)
 {
-	struct nf_conn_nat *new_nat = nf_ct_ext_find(conntrack, NF_CT_EXT_NAT);
-	struct nf_conn_nat *old_nat = (struct nf_conn_nat *)old;
+	struct nf_conn_nat *new_nat = new;
+	struct nf_conn_nat *old_nat = old;
 	struct nf_conn *ct = old_nat->ct;
 
 	if (!ct || !(ct->status & IPS_NAT_DONE_MASK))
diff --git a/net/netfilter/nf_conntrack_extend.c b/net/netfilter/nf_conntrack_extend.c
index cf6ba66..8b9be1e 100644
--- a/net/netfilter/nf_conntrack_extend.c
+++ b/net/netfilter/nf_conntrack_extend.c
@@ -109,7 +109,8 @@ void *__nf_ct_ext_add(struct nf_conn *ct, enum nf_ct_ext_id id, gfp_t gfp)
 			rcu_read_lock();
 			t = rcu_dereference(nf_ct_ext_types[i]);
 			if (t && t->move)
-				t->move(ct, ct->ext + ct->ext->offset[i]);
+				t->move((void *)new + new->offset[i],
+					(void *)ct->ext + ct->ext->offset[i]);
 			rcu_read_unlock();
 		}
 		kfree(ct->ext);

Re: Still oopsing in nf_nat_move_storage()

[Thomas Woerner]

Patrick McHardy wrote:
> Chuck Ebbert wrote:
>> On 01/31/2008 01:03 PM, Chuck Ebbert wrote:
>>> On 01/29/2008 12:18 PM, Patrick McHardy wrote:
>>>> Chuck Ebbert wrote:
>>>>> nf_nat_move_storage():
>>>>> /usr/src/debug/kernel-2.6.23/linux-2.6.23.i686/net/ipv4/netfilter/nf_nat_core.c:612 
>>>>>
>>>>>
>>>>>       87:       f7 47 64 80 01 00 00    testl  $0x180,0x64(%edi)
>>>>>       8e:       74 39                   je     c9
>>>>> <nf_nat_move_storage+0x65>
>>>>>
>>>>> line 612:
>>>>>         if (!(ct->status & IPS_NAT_DONE_MASK))
>>>>>                 return;
>>>>>
>>>>> ct is NULL
>>>> The current kernel (and 2.6.23-stable) have:
>>>>
>>>>         if (!ct || !(ct->status & IPS_NAT_DONE_MASK))
>>>>                 return;
>>>>
>>>> so it seems you're using an old version.
>>
>> So, it is now oopsing after the test for NULL and only x86_64 is
>> catching the invalid address because it is non-canonical. Checking
>> for NULL is obviously not enough...
> 
> 
> Could you try whether this patch fixes it please?
> 
Hallo Patrick,

I have tested the patch and the problem seems to be fixed.

Thanks,
Thomas

-- 
Thomas Woerner
Software Engineer            Phone: +49-711-96437-0
Red Hat GmbH                 Fax  : +49-711-96437-111
Hauptstaetterstr. 58         Email: Thomas Woerner <twoerner@redhat.com>
D-70178 Stuttgart            Web  : http://www.redhat.de/