NULL pointer dereference in xt_register_target()

NULL pointer dereference in xt_register_target()

[Cong Wang]

Hi, folks,

The latest net-next tree can't boot due to a NULL ptr def
bug in the kernel, the full backtrack is:

http://img1.douban.com/view/photo/photo/public/p1697139550.jpg

the kernel .config file is:

http://pastebin.com/9YTnkqKN

I don't have time to look into the issue. If you need other info,
please let me know.

Thanks.

Re: NULL pointer dereference in xt_register_target()

[Eric Dumazet]

On Wed, 2012-09-05 at 23:43 +0800, Cong Wang wrote:
> Hi, folks,
> 
> The latest net-next tree can't boot due to a NULL ptr def
> bug in the kernel, the full backtrack is:
> 
> http://img1.douban.com/view/photo/photo/public/p1697139550.jpg
> 
> the kernel .config file is:
> 
> http://pastebin.com/9YTnkqKN
> 
> I don't have time to look into the issue. If you need other info,
> please let me know.

It seems xt_nat_init() is called before xt_init(), so xt array is not
yet setup.

Re: NULL pointer dereference in xt_register_target()

[Eric Dumazet]

On Wed, 2012-09-05 at 17:55 +0200, Eric Dumazet wrote:
> On Wed, 2012-09-05 at 23:43 +0800, Cong Wang wrote:
> > Hi, folks,
> > 
> > The latest net-next tree can't boot due to a NULL ptr def
> > bug in the kernel, the full backtrack is:
> > 
> > http://img1.douban.com/view/photo/photo/public/p1697139550.jpg
> > 
> > the kernel .config file is:
> > 
> > http://pastebin.com/9YTnkqKN
> > 
> > I don't have time to look into the issue. If you need other info,
> > please let me know.
> 
> It seems xt_nat_init() is called before xt_init(), so xt array is not
> yet setup.
> 
> 

Seems the following patch should help, I have to try it ;)

diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index 8d987c3..afcea11 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -1390,6 +1390,6 @@ static void __exit xt_fini(void)
 	kfree(xt);
 }
 
-module_init(xt_init);
+core_initcall(xt_init);
 module_exit(xt_fini);
 

[PATCH net-next] netfilter: x_tables: xt_init() should run earlier

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Cong Wang reported a NULL dereference in xt_register_target()

It turns out xt_nat_init() was called before xt_init(), so xt array
was not yet setup.

xt_init() should be marked core_initcall() to solve this problem.

Reported-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/netfilter/x_tables.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index 8d987c3..afcea11 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -1390,6 +1390,6 @@ static void __exit xt_fini(void)
 	kfree(xt);
 }
 
-module_init(xt_init);
+core_initcall(xt_init);
 module_exit(xt_fini);
 

Re: NULL pointer dereference in xt_register_target()

[Pablo Neira Ayuso]

On Wed, Sep 05, 2012 at 05:55:06PM +0200, Eric Dumazet wrote:
> On Wed, 2012-09-05 at 23:43 +0800, Cong Wang wrote:
> > Hi, folks,
> > 
> > The latest net-next tree can't boot due to a NULL ptr def
> > bug in the kernel, the full backtrack is:
> > 
> > http://img1.douban.com/view/photo/photo/public/p1697139550.jpg
> > 
> > the kernel .config file is:
> > 
> > http://pastebin.com/9YTnkqKN
> > 
> > I don't have time to look into the issue. If you need other info,
> > please let me know.
> 
> It seems xt_nat_init() is called before xt_init(), so xt array is not
> yet setup.

I have enqueued the following patch to fix this:

http://1984.lsi.us.es/git/nf-next/commit/?id=00545bec9412d130c77f72a08d6c8b6ad21d4a1
e
commit 00545bec9412d130c77f72a08d6c8b6ad21d4a1e
Author: Pablo Neira Ayuso <pablo@netfilter.org>
Date:   Wed Sep 5 18:24:55 2012 +0200

    netfilter: fix crash during boot if NAT has been compiled built-in

Thanks.

Re: [PATCH net-next] netfilter: x_tables: xt_init() should run earlier

[Pablo Neira Ayuso]

Hi Eric,

On Wed, Sep 05, 2012 at 06:37:53PM +0200, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> Cong Wang reported a NULL dereference in xt_register_target()
> 
> It turns out xt_nat_init() was called before xt_init(), so xt array
> was not yet setup.
> 
> xt_init() should be marked core_initcall() to solve this problem.
> 
> Reported-by: Cong Wang <xiyou.wangcong@gmail.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/netfilter/x_tables.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
> index 8d987c3..afcea11 100644
> --- a/net/netfilter/x_tables.c
> +++ b/net/netfilter/x_tables.c
> @@ -1390,6 +1390,6 @@ static void __exit xt_fini(void)
>  	kfree(xt);
>  }
>  
> -module_init(xt_init);
> +core_initcall(xt_init);
>  module_exit(xt_fini);

It seems we've clashed fixing this, sorry. Can you still see any
problem with my patch?

Thanks for looking into this.

Re: [PATCH net-next] netfilter: x_tables: xt_init() should run earlier

[Eric Dumazet]

On Wed, 2012-09-05 at 18:53 +0200, Pablo Neira Ayuso wrote:
> Hi Eric,

> 
> It seems we've clashed fixing this, sorry. Can you still see any
> problem with my patch?
> 
> Thanks for looking into this.

No problem !

It seems link order is the way to go, so your patch is good too !

Thanks

Re: NULL pointer dereference in xt_register_target()

[Cong Wang]

On Thu, Sep 6, 2012 at 12:48 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Sep 05, 2012 at 05:55:06PM +0200, Eric Dumazet wrote:
>> On Wed, 2012-09-05 at 23:43 +0800, Cong Wang wrote:
>> > Hi, folks,
>> >
>> > The latest net-next tree can't boot due to a NULL ptr def
>> > bug in the kernel, the full backtrack is:
>> >
>> > http://img1.douban.com/view/photo/photo/public/p1697139550.jpg
>> >
>> > the kernel .config file is:
>> >
>> > http://pastebin.com/9YTnkqKN
>> >
>> > I don't have time to look into the issue. If you need other info,
>> > please let me know.
>>
>> It seems xt_nat_init() is called before xt_init(), so xt array is not
>> yet setup.
>
> I have enqueued the following patch to fix this:
>
> http://1984.lsi.us.es/git/nf-next/commit/?id=00545bec9412d130c77f72a08d6c8b6ad21d4a1
> e
> commit 00545bec9412d130c77f72a08d6c8b6ad21d4a1e
> Author: Pablo Neira Ayuso <pablo@netfilter.org>
> Date:   Wed Sep 5 18:24:55 2012 +0200
>
>     netfilter: fix crash during boot if NAT has been compiled built-in
>

Yeah, this indeed fixes the bug.

Please push it to net-next as soon as possible?

Thanks!

Re: NULL pointer dereference in xt_register_target()

[Pablo Neira Ayuso]

On Thu, Sep 06, 2012 at 10:27:22PM +0800, Cong Wang wrote:
> On Thu, Sep 6, 2012 at 12:48 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Wed, Sep 05, 2012 at 05:55:06PM +0200, Eric Dumazet wrote:
> >> On Wed, 2012-09-05 at 23:43 +0800, Cong Wang wrote:
> >> > Hi, folks,
> >> >
> >> > The latest net-next tree can't boot due to a NULL ptr def
> >> > bug in the kernel, the full backtrack is:
> >> >
> >> > http://img1.douban.com/view/photo/photo/public/p1697139550.jpg
> >> >
> >> > the kernel .config file is:
> >> >
> >> > http://pastebin.com/9YTnkqKN
> >> >
> >> > I don't have time to look into the issue. If you need other info,
> >> > please let me know.
> >>
> >> It seems xt_nat_init() is called before xt_init(), so xt array is not
> >> yet setup.
> >
> > I have enqueued the following patch to fix this:
> >
> > http://1984.lsi.us.es/git/nf-next/commit/?id=00545bec9412d130c77f72a08d6c8b6ad21d4a1
> > e
> > commit 00545bec9412d130c77f72a08d6c8b6ad21d4a1e
> > Author: Pablo Neira Ayuso <pablo@netfilter.org>
> > Date:   Wed Sep 5 18:24:55 2012 +0200
> >
> >     netfilter: fix crash during boot if NAT has been compiled built-in
> >
> 
> Yeah, this indeed fixes the bug.
> 
> Please push it to net-next as soon as possible?

Will do, thanks for testing.

Re: [PATCH net-next] netfilter: x_tables: xt_init() should run earlier

[Patrick McHardy]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 877 bytes --]

On Wed, 5 Sep 2012, Eric Dumazet wrote:

> From: Eric Dumazet <edumazet@google.com>
>
> Cong Wang reported a NULL dereference in xt_register_target()
>
> It turns out xt_nat_init() was called before xt_init(), so xt array
> was not yet setup.
>
> xt_init() should be marked core_initcall() to solve this problem.
>
> Reported-by: Cong Wang <xiyou.wangcong@gmail.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
> net/netfilter/x_tables.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
> index 8d987c3..afcea11 100644
> --- a/net/netfilter/x_tables.c
> +++ b/net/netfilter/x_tables.c
> @@ -1390,6 +1390,6 @@ static void __exit xt_fini(void)
> 	kfree(xt);
> }
>
> -module_init(xt_init);
> +core_initcall(xt_init);
> module_exit(xt_fini);

Shouldn't we simply change the Makefile order?

[-- Attachment #2: Type: TEXT/PLAIN, Size: 1448 bytes --]

commit ecc4508e476e4325e747dad5d86c03248ed16271
Author: Patrick McHardy <kaber@trash.net>
Date:   Sat Sep 8 19:45:12 2012 +0200

    netfilter: fix xt_nat link order
    
    Cong Wang reported a NULL dereference in xt_register_target()
    
    It turns out xt_nat_init() was called before xt_init(), so xt array
    was not yet setup.
    
    Move xt_nat down in the Makefile to avoid initialization before
    x_tables is initialized.
    
    Based on patch from Eric Dumazet.
    
    Reported-by: Cong Wang <xiyou.wangcong@gmail.com>
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/Makefile b/net/netfilter/Makefile
index 98244d4..6ad6616 100644
--- a/net/netfilter/Makefile
+++ b/net/netfilter/Makefile
@@ -47,7 +47,6 @@ nf_nat-y	:= nf_nat_core.o nf_nat_proto_unknown.o nf_nat_proto_common.o \
 		   nf_nat_proto_udp.o nf_nat_proto_tcp.o nf_nat_helper.o
 
 obj-$(CONFIG_NF_NAT) += nf_nat.o
-obj-$(CONFIG_NF_NAT) += xt_nat.o
 
 # NAT protocols (nf_nat)
 obj-$(CONFIG_NF_NAT_PROTO_DCCP) += nf_nat_proto_dccp.o
@@ -93,6 +92,7 @@ obj-$(CONFIG_NETFILTER_XT_TARGET_TCPOPTSTRIP) += xt_TCPOPTSTRIP.o
 obj-$(CONFIG_NETFILTER_XT_TARGET_TEE) += xt_TEE.o
 obj-$(CONFIG_NETFILTER_XT_TARGET_TRACE) += xt_TRACE.o
 obj-$(CONFIG_NETFILTER_XT_TARGET_IDLETIMER) += xt_IDLETIMER.o
+obj-$(CONFIG_NF_NAT) += xt_nat.o
 
 # matches
 obj-$(CONFIG_NETFILTER_XT_MATCH_ADDRTYPE) += xt_addrtype.o

Re: [PATCH net-next] netfilter: x_tables: xt_init() should run earlier

[Eric Dumazet]

On Sat, 2012-09-08 at 19:50 +0200, Patrick McHardy wrote:

> Shouldn't we simply change the Makefile order?

Yes, this is what Pablo did.