Fix the size overflow of addrconf_sysctl array

Patch-next Applied Jin Dongming 4 participants 5 messages Oct 9, 2009 – Oct 14, 2009

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Jin Dongming

author

2009-10-09 02:38 UTC #0 lore raw

(This patch fixes bug of commit f7734fdf61ec6bb848e0bafc1fb8bad2c124bb50
title "make TLLAO option for NA packets configurable")

When the IPV6 conf is used, the function sysctl_set_parent is called and the array addrconf_sysctl is used as a parameter of the function.

The above patch added new conf "force_tllao" into the array addrconf_sysctl, but the size of the array was not modified, the static allocated size is DEVCONF_MAX + 1 but the real size is DEVCONF_MAX + 2, so the problem is that the function sysctl_set_parent accessed wrong address.

I got the following information.

Call Trace:
    [<ffffffff8106085d>] sysctl_set_parent+0x29/0x3e
    [<ffffffff8106085d>] sysctl_set_parent+0x29/0x3e
    [<ffffffff8106085d>] sysctl_set_parent+0x29/0x3e
    [<ffffffff8106085d>] sysctl_set_parent+0x29/0x3e
    [<ffffffff8106085d>] sysctl_set_parent+0x29/0x3e
    [<ffffffff810622d5>] __register_sysctl_paths+0xde/0x272
    [<ffffffff8110892d>] ? __kmalloc_track_caller+0x16e/0x180
    [<ffffffffa00cfac3>] ? __addrconf_sysctl_register+0xc5/0x144 [ipv6]
    [<ffffffff8141f2c9>] register_net_sysctl_table+0x48/0x4b
    [<ffffffffa00cfaf5>] __addrconf_sysctl_register+0xf7/0x144 [ipv6]
    [<ffffffffa00cfc16>] addrconf_init_net+0xd4/0x104 [ipv6]
    [<ffffffff8139195f>] setup_net+0x35/0x82
    [<ffffffff81391f6c>] copy_net_ns+0x76/0xe0
    [<ffffffff8107ad60>] create_new_namespaces+0xf0/0x16e
    [<ffffffff8107afee>] copy_namespaces+0x65/0x9f
    [<ffffffff81056dff>] copy_process+0xb2c/0x12c3
    [<ffffffff810576e1>] do_fork+0x14b/0x2d2
    [<ffffffff8107ac4e>] ? up_read+0xe/0x10
    [<ffffffff81438e73>] ? do_page_fault+0x27a/0x2aa
    [<ffffffff8101044b>] sys_clone+0x28/0x2a
    [<ffffffff81011fb3>] stub_clone+0x13/0x20
    [<ffffffff81011c72>] ? system_call_fastpath+0x16/0x1b

And the information of IPV6 in .config is as following. IPV6 in .config:
CONFIG_IPV6=m
CONFIG_IPV6_PRIVACY=y CONFIG_IPV6_ROUTER_PREF=y CONFIG_IPV6_ROUTE_INFO=y CONFIG_IPV6_OPTIMISTIC_DAD=y CONFIG_IPV6_MIP6=m
CONFIG_IPV6_SIT=m
# CONFIG_IPV6_SIT_6RD is not set CONFIG_IPV6_NDISC_NODETYPE=y CONFIG_IPV6_TUNNEL=m CONFIG_IPV6_MULTIPLE_TABLES=y CONFIG_IPV6_SUBTREES=y CONFIG_IPV6_MROUTE=y CONFIG_IPV6_PIMSM_V2=y
# CONFIG_IP_VS_IPV6 is not set CONFIG_NF_CONNTRACK_IPV6=m CONFIG_IP6_NF_MATCH_IPV6HEADER=m

I confirmed this patch fixes this problem.

Signed-off-by Jin Dongming <[email protected]>

---
 include/linux/ipv6.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

include/linux/ipv6.h +1 -0

index ae74ede..5640425 100644
@@ -208,6 +208,7 @@ enum {
 	DEVCONF_MC_FORWARDING,
 	DEVCONF_DISABLE_IPV6,
 	DEVCONF_ACCEPT_DAD,
+	DEVCONF_FORCE_TLLAO,
 	DEVCONF_MAX
 };

Signature · 1 line

1.6.2.2

David Miller

maintainer Applied

2009-10-09 05:44 UTC #1 lore raw

From: Jin Dongming <[email protected]>
Date: Fri, 09 Oct 2009 11:37:59 +0900

Please post networking patches always CC:'d to [email protected] so that it gets added to our networking patch tracking system at:

http://patchwork.ozlabs.org/project/netdev/list/

Thank you.

I've applied your fix, thanks!

Quoted 79 lines

(This patch fixes bug of commit f7734fdf61ec6bb848e0bafc1fb8bad2c124bb50
title "make TLLAO option for NA packets configurable")
When the IPV6 conf is used, the function sysctl_set_parent is called and the array addrconf_sysctl is used as a parameter of the function.
The above patch added new conf "force_tllao" into the array addrconf_sysctl, but the size of the array was not modified, the static allocated size is DEVCONF_MAX + 1 but the real size is DEVCONF_MAX + 2, so the problem is that the function sysctl_set_parent accessed wrong address.
I got the following information.
Call Trace:
    [<ffffffff8106085d>] sysctl_set_parent+0x29/0x3e
    [<ffffffff8106085d>] sysctl_set_parent+0x29/0x3e
    [<ffffffff8106085d>] sysctl_set_parent+0x29/0x3e
    [<ffffffff8106085d>] sysctl_set_parent+0x29/0x3e
    [<ffffffff8106085d>] sysctl_set_parent+0x29/0x3e
    [<ffffffff810622d5>] __register_sysctl_paths+0xde/0x272
    [<ffffffff8110892d>] ? __kmalloc_track_caller+0x16e/0x180
    [<ffffffffa00cfac3>] ? __addrconf_sysctl_register+0xc5/0x144 [ipv6]
    [<ffffffff8141f2c9>] register_net_sysctl_table+0x48/0x4b
    [<ffffffffa00cfaf5>] __addrconf_sysctl_register+0xf7/0x144 [ipv6]
    [<ffffffffa00cfc16>] addrconf_init_net+0xd4/0x104 [ipv6]
    [<ffffffff8139195f>] setup_net+0x35/0x82
    [<ffffffff81391f6c>] copy_net_ns+0x76/0xe0
    [<ffffffff8107ad60>] create_new_namespaces+0xf0/0x16e
    [<ffffffff8107afee>] copy_namespaces+0x65/0x9f
    [<ffffffff81056dff>] copy_process+0xb2c/0x12c3
    [<ffffffff810576e1>] do_fork+0x14b/0x2d2
    [<ffffffff8107ac4e>] ? up_read+0xe/0x10
    [<ffffffff81438e73>] ? do_page_fault+0x27a/0x2aa
    [<ffffffff8101044b>] sys_clone+0x28/0x2a
    [<ffffffff81011fb3>] stub_clone+0x13/0x20
    [<ffffffff81011c72>] ? system_call_fastpath+0x16/0x1b
And the information of IPV6 in .config is as following. IPV6 in .config:
CONFIG_IPV6=m
CONFIG_IPV6_PRIVACY=y CONFIG_IPV6_ROUTER_PREF=y CONFIG_IPV6_ROUTE_INFO=y CONFIG_IPV6_OPTIMISTIC_DAD=y CONFIG_IPV6_MIP6=m
CONFIG_IPV6_SIT=m
# CONFIG_IPV6_SIT_6RD is not set CONFIG_IPV6_NDISC_NODETYPE=y CONFIG_IPV6_TUNNEL=m CONFIG_IPV6_MULTIPLE_TABLES=y CONFIG_IPV6_SUBTREES=y CONFIG_IPV6_MROUTE=y CONFIG_IPV6_PIMSM_V2=y
# CONFIG_IP_VS_IPV6 is not set CONFIG_NF_CONNTRACK_IPV6=m CONFIG_IP6_NF_MATCH_IPV6HEADER=m
I confirmed this patch fixes this problem.
Signed-off-by Jin Dongming <[email protected]>
---
 include/linux/ipv6.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
include/linux/ipv6.h +1 -0
index ae74ede..5640425 100644
@@ -208,6 +208,7 @@ enum {
 	DEVCONF_MC_FORWARDING,
 	DEVCONF_DISABLE_IPV6,
 	DEVCONF_ACCEPT_DAD,
+	DEVCONF_FORCE_TLLAO,
 	DEVCONF_MAX
 };
 
Signature · 1 line
1.6.2.2

Cosmin Ratiu

2009-10-09 13:12 UTC #2 lore raw

Shouldn't this be changed too then?

Or better yet, wouldn't a change that eliminates the need of adding a new option in two separate places be useful?

I see the only use for that DEVCONF enum is to dump the settings via netlink. Wouldn't a memcpy suffice?

Cosmin.

Quoted 76 lines · On Friday 09 October 2009 08:44:15 David Miller wrote:

From: Jin Dongming <[email protected]>
Date: Fri, 09 Oct 2009 11:37:59 +0900
Please post networking patches always CC:'d to [email protected] so that it gets added to our networking patch tracking system at:
http://patchwork.ozlabs.org/project/netdev/list/
Thank you.
I've applied your fix, thanks!
Quoted 64 lines
(This patch fixes bug of commit f7734fdf61ec6bb848e0bafc1fb8bad2c124bb50
title "make TLLAO option for NA packets configurable")
When the IPV6 conf is used, the function sysctl_set_parent is called and the array addrconf_sysctl is used as a parameter of the function.
The above patch added new conf "force_tllao" into the array addrconf_sysctl, but the size of the array was not modified, the static allocated size is DEVCONF_MAX + 1 but the real size is DEVCONF_MAX + 2, so the problem is that the function sysctl_set_parent accessed wrong address.
I got the following information.
Call Trace:
    [<ffffffff8106085d>] sysctl_set_parent+0x29/0x3e
    [<ffffffff8106085d>] sysctl_set_parent+0x29/0x3e
    [<ffffffff8106085d>] sysctl_set_parent+0x29/0x3e
    [<ffffffff8106085d>] sysctl_set_parent+0x29/0x3e
    [<ffffffff8106085d>] sysctl_set_parent+0x29/0x3e
    [<ffffffff810622d5>] __register_sysctl_paths+0xde/0x272
    [<ffffffff8110892d>] ? __kmalloc_track_caller+0x16e/0x180
    [<ffffffffa00cfac3>] ? __addrconf_sysctl_register+0xc5/0x144 [ipv6]
    [<ffffffff8141f2c9>] register_net_sysctl_table+0x48/0x4b
    [<ffffffffa00cfaf5>] __addrconf_sysctl_register+0xf7/0x144 [ipv6]
    [<ffffffffa00cfc16>] addrconf_init_net+0xd4/0x104 [ipv6]
    [<ffffffff8139195f>] setup_net+0x35/0x82
    [<ffffffff81391f6c>] copy_net_ns+0x76/0xe0
    [<ffffffff8107ad60>] create_new_namespaces+0xf0/0x16e
    [<ffffffff8107afee>] copy_namespaces+0x65/0x9f
    [<ffffffff81056dff>] copy_process+0xb2c/0x12c3
    [<ffffffff810576e1>] do_fork+0x14b/0x2d2
    [<ffffffff8107ac4e>] ? up_read+0xe/0x10
    [<ffffffff81438e73>] ? do_page_fault+0x27a/0x2aa
    [<ffffffff8101044b>] sys_clone+0x28/0x2a
    [<ffffffff81011fb3>] stub_clone+0x13/0x20
    [<ffffffff81011c72>] ? system_call_fastpath+0x16/0x1b
And the information of IPV6 in .config is as following. IPV6 in .config:
CONFIG_IPV6=m
CONFIG_IPV6_PRIVACY=y CONFIG_IPV6_ROUTER_PREF=y CONFIG_IPV6_ROUTE_INFO=y CONFIG_IPV6_OPTIMISTIC_DAD=y CONFIG_IPV6_MIP6=m
CONFIG_IPV6_SIT=m
# CONFIG_IPV6_SIT_6RD is not set CONFIG_IPV6_NDISC_NODETYPE=y CONFIG_IPV6_TUNNEL=m CONFIG_IPV6_MULTIPLE_TABLES=y CONFIG_IPV6_SUBTREES=y CONFIG_IPV6_MROUTE=y CONFIG_IPV6_PIMSM_V2=y
# CONFIG_IP_VS_IPV6 is not set CONFIG_NF_CONNTRACK_IPV6=m CONFIG_IP6_NF_MATCH_IPV6HEADER=m
I confirmed this patch fixes this problem.
Signed-off-by Jin Dongming <[email protected]>
---
 include/linux/ipv6.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

Quoted 12 lines

include/linux/ipv6.h enum { · line 213

Quoted 11 lines

include/linux/ipv6.h +1 -0

index ae74ede..5640425 100644
@@ -208,6 +208,7 @@ enum {
 	DEVCONF_MC_FORWARDING,
 	DEVCONF_DISABLE_IPV6,
 	DEVCONF_ACCEPT_DAD,
+	DEVCONF_FORCE_TLLAO,
 	DEVCONF_MAX
 };

From f72949922a102ee9e728a2805287a9bd9a4d2e67 Mon Sep 17 00:00:00 2001
From: Cosmin Ratiu <[email protected]>
Date: Fri, 9 Oct 2009 15:57:09 +0300
Subject: [PATCH] [ipv6]: fix devconf after adding force_tllao option

Signed-off-by Cosmin Ratiu <[email protected]>

---
 net/ipv6/addrconf.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

net/ipv6/addrconf.c +1 -0

index 1fd0a3d..a065f40 100644
@@ -3708,6 +3708,7 @@ static inline void ipv6_store_devconf(struct ipv6_devconf *cnf,
 #endif
 	array[DEVCONF_DISABLE_IPV6] = cnf->disable_ipv6;
 	array[DEVCONF_ACCEPT_DAD] = cnf->accept_dad;
+	array[DEVCONF_FORCE_TLLAO] = cnf->force_tllao;
 }
 
 static inline size_t inet6_if_nlmsg_size(void)

Signature · 1 line

1.6.3.3

David Miller

maintainer Applied

2009-10-13 10:45 UTC #3 lore raw

From: Cosmin Ratiu <[email protected]>
Date: Fri, 9 Oct 2009 16:11:14 +0300

Quoted 4 lines

Shouldn't this be changed too then?
Or better yet, wouldn't a change that eliminates the need of adding a new option in two separate places be useful?

Yes, it's crummy how things work now, indeed.

Quoted 2 lines

I see the only use for that DEVCONF enum is to dump the settings via netlink. Wouldn't a memcpy suffice?

It should be.

I've applied your patch, thanks!

Octavian Purdila

2009-10-14 18:18 UTC #4 lore raw

Quoted 8 lines · On Tuesday 13 October 2009 13:45:09 you wrote:

From: Cosmin Ratiu <[email protected]>
Date: Fri, 9 Oct 2009 16:11:14 +0300
Quoted 4 lines
Shouldn't this be changed too then?
Or better yet, wouldn't a change that eliminates the need of adding a new option in two separate places be useful?

Quoted 7 lines

Yes, it's crummy how things work now, indeed.
Quoted 2 lines
I see the only use for that DEVCONF enum is to dump the settings via netlink. Wouldn't a memcpy suffice?
It should be.

I've taken a look at this and it seems the current way of doing it is really required as we need to preserve userspace ABI across different CONFIG_ settings.

tavi