[PATCH] ipv6: Prevent access to uninitialized fib_table_hash via /proc/net/ipv6_route

/proc/net/ipv6_route reflects the contents of fib_table_hash. The proc
handler is installed in ip6_route_net_init() whereas fib_table_hash is
allocated in fib6_net_init() _after_ the proc handler has been installed.

This opens up a short time frame to access fib_table_hash with its pants
down.

fib6_init() as a whole can't be moved to an earlier position as it also
registers the rtnetlink message handlers which should be registered at
the end. Therefore split it into fib6_init() which is run early and
fib6_init_late() to register the rtnetlink message handlers.

Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 include/net/ip6_fib.h |    2 ++
 net/ipv6/ip6_fib.c    |   18 +++++++++++-------
 net/ipv6/route.c      |   16 +++++++++++-----
 3 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 0ae759a..209af13 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h

@@ -271,6 +271,8 @@ extern void			fib6_run_gc(unsigned long expires,
 extern void			fib6_gc_cleanup(void);
 
 extern int			fib6_init(void);
+extern int			fib6_init_late(void);
+extern void			fib6_cleanup_late(void);
 
 #ifdef CONFIG_IPV6_MULTIPLE_TABLES
 extern int			fib6_rules_init(void);

diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 74c21b9..fbd4aff 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c

@@ -1692,21 +1692,25 @@ int __init fib6_init(void)
 	ret = register_pernet_subsys(&fib6_net_ops);
 	if (ret)
 		goto out_kmem_cache_create;
-
-	ret = __rtnl_register(PF_INET6, RTM_GETROUTE, NULL, inet6_dump_fib,
-			      NULL);
-	if (ret)
-		goto out_unregister_subsys;
 out:
 	return ret;
 
-out_unregister_subsys:
-	unregister_pernet_subsys(&fib6_net_ops);
 out_kmem_cache_create:
 	kmem_cache_destroy(fib6_node_kmem);
 	goto out;
 }
 
+int __init fib6_init_late(void)
+{
+	return __rtnl_register(PF_INET6, RTM_GETROUTE, NULL, inet6_dump_fib,
+			       NULL);
+}
+
+void fib6_cleanup_late(void)
+{
+	rtnl_unregister(PF_INET6, RTM_GETROUTE);
+}
+
 void fib6_gc_cleanup(void)
 {
 	unregister_pernet_subsys(&fib6_net_ops);

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 999a982..dc60bf5 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c

@@ -3018,10 +3018,14 @@ int __init ip6_route_init(void)
 	if (ret)
 		goto out_kmem_cache;
 
-	ret = register_pernet_subsys(&ip6_route_net_ops);
+	ret = fib6_init();
 	if (ret)
 		goto out_dst_entries;
 
+	ret = register_pernet_subsys(&ip6_route_net_ops);
+	if (ret)
+		goto out_fib6_init;
+
 	ip6_dst_blackhole_ops.kmem_cachep = ip6_dst_ops_template.kmem_cachep;
 
 	/* Registering of the loopback is done before this portion of code,

@@ -3035,13 +3039,13 @@ int __init ip6_route_init(void)
 	init_net.ipv6.ip6_blk_hole_entry->dst.dev = init_net.loopback_dev;
 	init_net.ipv6.ip6_blk_hole_entry->rt6i_idev = in6_dev_get(init_net.loopback_dev);
   #endif
-	ret = fib6_init();
+	ret = fib6_init_late();
 	if (ret)
 		goto out_register_subsys;
 
 	ret = xfrm6_init();
 	if (ret)
-		goto out_fib6_init;
+		goto out_fib6_init_late;
 
 	ret = fib6_rules_init();
 	if (ret)

@@ -3064,10 +3068,12 @@ fib6_rules_init:
 	fib6_rules_cleanup();
 xfrm6_init:
 	xfrm6_fini();
-out_fib6_init:
-	fib6_gc_cleanup();
+out_fib6_init_late:
+	fib6_cleanup_late();
 out_register_subsys:
 	unregister_pernet_subsys(&ip6_route_net_ops);
+out_fib6_init:
+	fib6_gc_cleanup();
 out_dst_entries:
 	dst_entries_destroy(&ip6_dst_blackhole_ops);
 out_kmem_cache:

-- 
1.7.7.6

On Fri, Jun 15, 2012 at 11:00:17AM +0200, Thomas Graf wrote:

/proc/net/ipv6_route reflects the contents of fib_table_hash. The proc
handler is installed in ip6_route_net_init() whereas fib_table_hash is
allocated in fib6_net_init() _after_ the proc handler has been installed.

This opens up a short time frame to access fib_table_hash with its pants
down.

fib6_init() as a whole can't be moved to an earlier position as it also
registers the rtnetlink message handlers which should be registered at
the end. Therefore split it into fib6_init() which is run early and
fib6_init_late() to register the rtnetlink message handlers.

Signed-off-by: Thomas Graf <tgraf@suug.ch>

Reviewed-by: Neil Horman <nhorman@tuxdriver.com>

From: Neil Horman <nhorman@tuxdriver.com>
Date: Fri, 15 Jun 2012 06:56:55 -0400

On Fri, Jun 15, 2012 at 11:00:17AM +0200, Thomas Graf wrote:

quoted

/proc/net/ipv6_route reflects the contents of fib_table_hash. The proc
handler is installed in ip6_route_net_init() whereas fib_table_hash is
allocated in fib6_net_init() _after_ the proc handler has been installed.

This opens up a short time frame to access fib_table_hash with its pants
down.

fib6_init() as a whole can't be moved to an earlier position as it also
registers the rtnetlink message handlers which should be registered at
the end. Therefore split it into fib6_init() which is run early and
fib6_init_late() to register the rtnetlink message handlers.

Signed-off-by: Thomas Graf <tgraf@suug.ch>

Reviewed-by: Neil Horman <nhorman@tuxdriver.com>

Applied.

Since you're snooping around in here, you might notice that on network
namespace shutdown, we leak all user configured ipv6 FIB rules.

From: David Miller <davem@davemloft.net>
Date: Fri, 15 Jun 2012 15:32:40 -0700 (PDT)

From: Neil Horman <nhorman@tuxdriver.com>
Date: Fri, 15 Jun 2012 06:56:55 -0400

quoted

On Fri, Jun 15, 2012 at 11:00:17AM +0200, Thomas Graf wrote:

quoted

/proc/net/ipv6_route reflects the contents of fib_table_hash. The proc
handler is installed in ip6_route_net_init() whereas fib_table_hash is
allocated in fib6_net_init() _after_ the proc handler has been installed.

This opens up a short time frame to access fib_table_hash with its pants
down.

fib6_init() as a whole can't be moved to an earlier position as it also
registers the rtnetlink message handlers which should be registered at
the end. Therefore split it into fib6_init() which is run early and
fib6_init_late() to register the rtnetlink message handlers.

Signed-off-by: Thomas Graf <tgraf@suug.ch>

Reviewed-by: Neil Horman <nhorman@tuxdriver.com>

Applied.

Since you're snooping around in here, you might notice that on network
namespace shutdown, we leak all user configured ipv6 FIB rules.

Thomas, this patch is buggy.

We will now initialize fib6_init() before ip6_net_route_net_ops is registerd.

This causes fib6_net_init() to run before net->ipv6.ip6_null_entry it
initialized.

Any route lookup will crash when we dereference a root's ->leaf
because it will be NULL.

Please test your changes more thoroughly.

From: David Miller <davem@davemloft.net>
Date: Fri, 15 Jun 2012 22:15:02 -0700 (PDT)

We will now initialize fib6_init() before ip6_net_route_net_ops is registerd.

This causes fib6_net_init() to run before net->ipv6.ip6_null_entry it
initialized.

Any route lookup will crash when we dereference a root's ->leaf
because it will be NULL.

I've decided to revert this change for now.

On Fri, Jun 15, 2012 at 10:15:02PM -0700, David Miller wrote:

From: David Miller <davem@davemloft.net>
Date: Fri, 15 Jun 2012 15:32:40 -0700 (PDT)

quoted

From: Neil Horman <nhorman@tuxdriver.com>
Date: Fri, 15 Jun 2012 06:56:55 -0400

quoted

On Fri, Jun 15, 2012 at 11:00:17AM +0200, Thomas Graf wrote:

quoted

/proc/net/ipv6_route reflects the contents of fib_table_hash. The proc
handler is installed in ip6_route_net_init() whereas fib_table_hash is
allocated in fib6_net_init() _after_ the proc handler has been installed.

This opens up a short time frame to access fib_table_hash with its pants
down.

fib6_init() as a whole can't be moved to an earlier position as it also
registers the rtnetlink message handlers which should be registered at
the end. Therefore split it into fib6_init() which is run early and
fib6_init_late() to register the rtnetlink message handlers.

Signed-off-by: Thomas Graf <tgraf@suug.ch>

Reviewed-by: Neil Horman <nhorman@tuxdriver.com>

Applied.

Since you're snooping around in here, you might notice that on network
namespace shutdown, we leak all user configured ipv6 FIB rules.

Thomas, this patch is buggy.

We will now initialize fib6_init() before ip6_net_route_net_ops is registerd.

This causes fib6_net_init() to run before net->ipv6.ip6_null_entry it
initialized.

Any route lookup will crash when we dereference a root's ->leaf
because it will be NULL.

Perhaps the flag on the proc file like we discussed might be the better way to
go after all here Thomas.

From: Neil Horman <nhorman@tuxdriver.com>
Date: Sat, 16 Jun 2012 09:07:23 -0400

Perhaps the flag on the proc file like we discussed might be the
better way to go after all here Thomas.

I suspect Thomas's general approach was fine, he just was trying
to hard. :-)

Just seperate exactly the procfs, sysctl, etc. stuff into a completely
seperate set of network namespace ops, and do it dead last in the
initialization list.

At least that is how I would approach the fix.

On Fri, Jun 15, 2012 at 10:15:02PM -0700, David Miller wrote:

Thomas, this patch is buggy.

We will now initialize fib6_init() before ip6_net_route_net_ops is registerd.

This causes fib6_net_init() to run before net->ipv6.ip6_null_entry it
initialized.

Any route lookup will crash when we dereference a root's ->leaf
because it will be NULL.

Please test your changes more thoroughly.

Sorry, that this has slipped through. I must have booted the wrong
kernel. I even had this run by an external tester to confirm that
the original panic disapppeared...

I'll come up with a new fix.

On Fri, Jun 15, 2012 at 03:32:40PM -0700, David Miller wrote:

Since you're snooping around in here, you might notice that on network
namespace shutdown, we leak all user configured ipv6 FIB rules.

I looked into this. fib_rules_unregister() will free all rules
belonging to the address family in that namespace.

Or were you referring to other rules?

From: Thomas Graf <tgraf@suug.ch>
Date: Tue, 19 Jun 2012 07:36:02 -0400

On Fri, Jun 15, 2012 at 03:32:40PM -0700, David Miller wrote:

quoted

Since you're snooping around in here, you might notice that on network
namespace shutdown, we leak all user configured ipv6 FIB rules.

I looked into this. fib_rules_unregister() will free all rules
belonging to the address family in that namespace.

Or were you referring to other rules?

Sorry, the leak I saw was for the fib6 tables, not the rules
themselves.

IPV4 has ip_fib_net_exit() which walks the FIB4 table hash
and releases everything.

I couldn't find the IPV6 counterpart.  All I could find was code which
explicitly liberates the ipv6 main and local tables.

There is no ipv6 code I can find which traverses fib_table_hash and
liberates the dynamically generated tables.