[PATCH net v2] ipv4: Fix use-after-free when flushing FIB tables

[PATCH net v2] ipv4: Fix use-after-free when flushing FIB tables

[Ido Schimmel]

Since commit 0ddcf43d5d4a ("ipv4: FIB Local/MAIN table collapse") the
local table uses the same trie allocated for the main table when custom
rules are not in use.

When a net namespace is dismantled, the main table is flushed and freed
(via an RCU callback) before the local table. In case the callback is
invoked before the local table is iterated, a use-after-free can occur.

Fix this by iterating over the FIB tables in reverse order, so that the
main table is always freed after the local table.

v2: Add a comment to make the fix more explicit per Dave's and Alex's
feedback.

Fixes: 0ddcf43d5d4a ("ipv4: FIB Local/MAIN table collapse")
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Reported-by: Fengguang Wu <fengguang.wu@intel.com>
---
 net/ipv4/fib_frontend.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index f52d27a422c3..08ce7f14ede1 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -1298,14 +1298,17 @@ static int __net_init ip_fib_net_init(struct net *net)
 
 static void ip_fib_net_exit(struct net *net)
 {
-	unsigned int i;
+	int i;
 
 	rtnl_lock();
 #ifdef CONFIG_IP_MULTIPLE_TABLES
 	RCU_INIT_POINTER(net->ipv4.fib_main, NULL);
 	RCU_INIT_POINTER(net->ipv4.fib_default, NULL);
 #endif
-	for (i = 0; i < FIB_TABLE_HASHSZ; i++) {
+	/* The local table must be destroyed before the main table,
+	 * as it might be using main's trie.
+	 */
+	for (i = FIB_TABLE_HASHSZ - 1; i >= 0; i--) {
 		struct hlist_head *head = &net->ipv4.fib_table_hash[i];
 		struct hlist_node *tmp;
 		struct fib_table *tb;
-- 
2.14.3

Re: [PATCH net v2] ipv4: Fix use-after-free when flushing FIB tables

[Alexander Duyck]

On Wed, Dec 20, 2017 at 12:51 AM, Ido Schimmel <idosch@mellanox.com> wrote:
> Since commit 0ddcf43d5d4a ("ipv4: FIB Local/MAIN table collapse") the
> local table uses the same trie allocated for the main table when custom
> rules are not in use.
>
> When a net namespace is dismantled, the main table is flushed and freed
> (via an RCU callback) before the local table. In case the callback is
> invoked before the local table is iterated, a use-after-free can occur.
>
> Fix this by iterating over the FIB tables in reverse order, so that the
> main table is always freed after the local table.
>
> v2: Add a comment to make the fix more explicit per Dave's and Alex's
> feedback.
>
> Fixes: 0ddcf43d5d4a ("ipv4: FIB Local/MAIN table collapse")
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> ---
>  net/ipv4/fib_frontend.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
> index f52d27a422c3..08ce7f14ede1 100644
> --- a/net/ipv4/fib_frontend.c
> +++ b/net/ipv4/fib_frontend.c
> @@ -1298,14 +1298,17 @@ static int __net_init ip_fib_net_init(struct net *net)
>
>  static void ip_fib_net_exit(struct net *net)
>  {
> -       unsigned int i;
> +       int i;
>
>         rtnl_lock();
>  #ifdef CONFIG_IP_MULTIPLE_TABLES
>         RCU_INIT_POINTER(net->ipv4.fib_main, NULL);
>         RCU_INIT_POINTER(net->ipv4.fib_default, NULL);
>  #endif
> -       for (i = 0; i < FIB_TABLE_HASHSZ; i++) {
> +       /* The local table must be destroyed before the main table,
> +        * as it might be using main's trie.
> +        */

I think we might want even more description here. Specifically why
reversing the order allows local to be destroyed before main. I was
thinking something along the lines of:

Destroy the tables in reverse order to guarantee that the local table,
ID 255, is destroyed before main table, ID 254. This is necessary as
local may contain references to data contained in main.

> +       for (i = FIB_TABLE_HASHSZ - 1; i >= 0; i--) {
>                 struct hlist_head *head = &net->ipv4.fib_table_hash[i];
>                 struct hlist_node *tmp;
>                 struct fib_table *tb;
> --
> 2.14.3
>

Re: [PATCH net v2] ipv4: Fix use-after-free when flushing FIB tables

[Ido Schimmel]

On Wed, Dec 20, 2017 at 07:32:21AM -0800, Alexander Duyck wrote:
> I think we might want even more description here. Specifically why
> reversing the order allows local to be destroyed before main. I was
> thinking something along the lines of:
> 
> Destroy the tables in reverse order to guarantee that the local table,
> ID 255, is destroyed before main table, ID 254. This is necessary as
> local may contain references to data contained in main.

Sent v3:
http://patchwork.ozlabs.org/patch/851564/

Thanks