[Devel,PATCHv3,RH7] ve/net/ip6tables: fix NULL ptr dereference at ip6t_unregister_table

Submitted by Dmitry Safonov on Oct. 31, 2016, 1:12 p.m.

Details

Message ID 20161031131238.31454-1-dsafonov@virtuozzo.com
State New
Series "ve/net/ip6tables: fix NULL ptr dereference at ip6t_unregister_table"
Headers show

Commit Message

Dmitry Safonov Oct. 31, 2016, 1:12 p.m.
We do not init raw table when IPTABLES6 is forbidden in VE.
So we shouldn't unregister/free it.

The following actions:
[root@s186 ~]# vzctl create 100
[root@s186 ~]# prlctl set 100 --netfilter disabled
[root@s186 ~]# prlctl start 100
Starting the CT...
The CT has been successfully started.
[root@s186 ~]# prlctl stop 100
Stopping the CT...

May lead to memory corruption/NULL pointer dereference:
[13990.985979] BUG: unable to handle kernel NULL pointer dereference at 0000000000000020
[13990.995425] Workqueue: netns cleanup_net
[13990.996030] task: ffff880423302610 ti: ffff880422dd0000 task.ti: ffff880422dd0000
[13990.996637] RIP: 0010:[<ffffffffa051f178>]  [<ffffffffa051f178>] ip6t_unregister_table+0x18/0x80 [ip6_tables]
[13991.006983] Call Trace:
[13991.007581]  [<ffffffffa05d6055>] ip6table_raw_net_exit+0x15/0x20 [ip6table_raw]
[13991.008187]  [<ffffffff81527dc9>] ops_exit_list.isra.5+0x39/0x60
[13991.008788]  [<ffffffff81528f50>] cleanup_net+0x260/0x480
[13991.009380]  [<ffffffff8109ef4b>] process_one_work+0x17b/0x470
[13991.009980]  [<ffffffff8109fd1b>] worker_thread+0x11b/0x400
[13991.010576]  [<ffffffff8109fc00>] ? rescuer_thread+0x400/0x400
[13991.011150]  [<ffffffff810a779f>] kthread+0xcf/0xe0
[13991.011717]  [<ffffffff810a76d0>] ? create_kthread+0x60/0x60
[13991.012619]  [<ffffffff81643cd8>] ret_from_fork+0x58/0x90
[13991.013153]  [<ffffffff810a76d0>] ? create_kthread+0x60/0x60
[13991.013668] Code: c4 28 5b 41 5c 41 5d 5d c3 e8 d5 c2 b5 e0 0f 1f 44 00 00 0f 1f 44 00 00 55 48 89 e5 41 57 41 56 49 89 fe 48 89 f7 41 55 41 54 53 <4c> 8b 7e 20 e8 7f 5f 04 e1 4c 8d 68 58 49 89 c4 8b 00 4c 89 eb
[13991.014765] RIP  [<ffffffffa051f178>] ip6t_unregister_table+0x18/0x80 [ip6_tables]

Fixes: commit bff5233618d8 ("ve/net/ip6tables: fix autoloading of the
ip6table_raw module from CT")

https://jira.sw.ru/browse/PSBM-54244

Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
---
v2: make it safe for possible online ipt_mask reconfiguration,
    that may come with some day in future
v3: null-set table on ip6table_raw_net_exit()

 net/ipv6/netfilter/ip6table_raw.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/net/ipv6/netfilter/ip6table_raw.c b/net/ipv6/netfilter/ip6table_raw.c
index 271835df64ce..b23d6c96899e 100644
--- a/net/ipv6/netfilter/ip6table_raw.c
+++ b/net/ipv6/netfilter/ip6table_raw.c
@@ -33,6 +33,10 @@  static struct nf_hook_ops *rawtable_ops __read_mostly;
 static int __net_init ip6table_raw_net_init(struct net *net)
 {
 	struct ip6t_replace *repl;
+	struct xt_table *ip6table_raw;
+
+	if (WARN_ON(net->ipv6.ip6table_raw))
+		net->ipv6.ip6table_raw = NULL;
 
 	if (!net_ipt_permitted(net, VE_IP_IPTABLES6))
 		return 0;
@@ -40,15 +44,23 @@  static int __net_init ip6table_raw_net_init(struct net *net)
 	repl = ip6t_alloc_initial_table(&packet_raw);
 	if (repl == NULL)
 		return -ENOMEM;
-	net->ipv6.ip6table_raw =
-		ip6t_register_table(net, &packet_raw, repl);
+	ip6table_raw = ip6t_register_table(net, &packet_raw, repl);
 	kfree(repl);
-	return PTR_RET(net->ipv6.ip6table_raw);
+
+	if (!IS_ERR(ip6table_raw))
+		net->ipv6.ip6table_raw = ip6table_raw;
+
+	return PTR_RET(ip6table_raw);
 }
 
 static void __net_exit ip6table_raw_net_exit(struct net *net)
 {
+	if (!net->ipv6.ip6table_raw)
+		return;
+
 	ip6t_unregister_table(net, net->ipv6.ip6table_raw);
+
+	net->ipv6.ip6table_raw = NULL;
 }
 
 static struct pernet_operations ip6table_raw_net_ops = {

Comments

Kirill Tkhai Oct. 31, 2016, 3:03 p.m.
On 31.10.2016 16:12, Dmitry Safonov wrote:
> We do not init raw table when IPTABLES6 is forbidden in VE.
> So we shouldn't unregister/free it.
> 
> The following actions:
> [root@s186 ~]# vzctl create 100
> [root@s186 ~]# prlctl set 100 --netfilter disabled
> [root@s186 ~]# prlctl start 100
> Starting the CT...
> The CT has been successfully started.
> [root@s186 ~]# prlctl stop 100
> Stopping the CT...
> 
> May lead to memory corruption/NULL pointer dereference:
> [13990.985979] BUG: unable to handle kernel NULL pointer dereference at 0000000000000020
> [13990.995425] Workqueue: netns cleanup_net
> [13990.996030] task: ffff880423302610 ti: ffff880422dd0000 task.ti: ffff880422dd0000
> [13990.996637] RIP: 0010:[<ffffffffa051f178>]  [<ffffffffa051f178>] ip6t_unregister_table+0x18/0x80 [ip6_tables]
> [13991.006983] Call Trace:
> [13991.007581]  [<ffffffffa05d6055>] ip6table_raw_net_exit+0x15/0x20 [ip6table_raw]
> [13991.008187]  [<ffffffff81527dc9>] ops_exit_list.isra.5+0x39/0x60
> [13991.008788]  [<ffffffff81528f50>] cleanup_net+0x260/0x480
> [13991.009380]  [<ffffffff8109ef4b>] process_one_work+0x17b/0x470
> [13991.009980]  [<ffffffff8109fd1b>] worker_thread+0x11b/0x400
> [13991.010576]  [<ffffffff8109fc00>] ? rescuer_thread+0x400/0x400
> [13991.011150]  [<ffffffff810a779f>] kthread+0xcf/0xe0
> [13991.011717]  [<ffffffff810a76d0>] ? create_kthread+0x60/0x60
> [13991.012619]  [<ffffffff81643cd8>] ret_from_fork+0x58/0x90
> [13991.013153]  [<ffffffff810a76d0>] ? create_kthread+0x60/0x60
> [13991.013668] Code: c4 28 5b 41 5c 41 5d 5d c3 e8 d5 c2 b5 e0 0f 1f 44 00 00 0f 1f 44 00 00 55 48 89 e5 41 57 41 56 49 89 fe 48 89 f7 41 55 41 54 53 <4c> 8b 7e 20 e8 7f 5f 04 e1 4c 8d 68 58 49 89 c4 8b 00 4c 89 eb
> [13991.014765] RIP  [<ffffffffa051f178>] ip6t_unregister_table+0x18/0x80 [ip6_tables]
> 
> Fixes: commit bff5233618d8 ("ve/net/ip6tables: fix autoloading of the
> ip6table_raw module from CT")
> 
> https://jira.sw.ru/browse/PSBM-54244
> 
> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
> Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>

Acked-by: Kirill Tkhai <ktkhai@virtuozzo.com>

> ---
> v2: make it safe for possible online ipt_mask reconfiguration,
>     that may come with some day in future
> v3: null-set table on ip6table_raw_net_exit()
> 
>  net/ipv6/netfilter/ip6table_raw.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/net/ipv6/netfilter/ip6table_raw.c b/net/ipv6/netfilter/ip6table_raw.c
> index 271835df64ce..b23d6c96899e 100644
> --- a/net/ipv6/netfilter/ip6table_raw.c
> +++ b/net/ipv6/netfilter/ip6table_raw.c
> @@ -33,6 +33,10 @@ static struct nf_hook_ops *rawtable_ops __read_mostly;
>  static int __net_init ip6table_raw_net_init(struct net *net)
>  {
>  	struct ip6t_replace *repl;
> +	struct xt_table *ip6table_raw;
> +
> +	if (WARN_ON(net->ipv6.ip6table_raw))
> +		net->ipv6.ip6table_raw = NULL;
>  
>  	if (!net_ipt_permitted(net, VE_IP_IPTABLES6))
>  		return 0;
> @@ -40,15 +44,23 @@ static int __net_init ip6table_raw_net_init(struct net *net)
>  	repl = ip6t_alloc_initial_table(&packet_raw);
>  	if (repl == NULL)
>  		return -ENOMEM;
> -	net->ipv6.ip6table_raw =
> -		ip6t_register_table(net, &packet_raw, repl);
> +	ip6table_raw = ip6t_register_table(net, &packet_raw, repl);
>  	kfree(repl);
> -	return PTR_RET(net->ipv6.ip6table_raw);
> +
> +	if (!IS_ERR(ip6table_raw))
> +		net->ipv6.ip6table_raw = ip6table_raw;
> +
> +	return PTR_RET(ip6table_raw);
>  }
>  
>  static void __net_exit ip6table_raw_net_exit(struct net *net)
>  {
> +	if (!net->ipv6.ip6table_raw)
> +		return;
> +
>  	ip6t_unregister_table(net, net->ipv6.ip6table_raw);
> +
> +	net->ipv6.ip6table_raw = NULL;
>  }
>  
>  static struct pernet_operations ip6table_raw_net_ops = {
>
Andrey Ryabinin Oct. 31, 2016, 3:04 p.m.
On 10/31/2016 04:12 PM, Dmitry Safonov wrote:
> We do not init raw table when IPTABLES6 is forbidden in VE.
> So we shouldn't unregister/free it.
> 
> The following actions:
> [root@s186 ~]# vzctl create 100
> [root@s186 ~]# prlctl set 100 --netfilter disabled
> [root@s186 ~]# prlctl start 100
> Starting the CT...
> The CT has been successfully started.
> [root@s186 ~]# prlctl stop 100
> Stopping the CT...
> 
> May lead to memory corruption/NULL pointer dereference:
> [13990.985979] BUG: unable to handle kernel NULL pointer dereference at 0000000000000020
> [13990.995425] Workqueue: netns cleanup_net
> [13990.996030] task: ffff880423302610 ti: ffff880422dd0000 task.ti: ffff880422dd0000
> [13990.996637] RIP: 0010:[<ffffffffa051f178>]  [<ffffffffa051f178>] ip6t_unregister_table+0x18/0x80 [ip6_tables]
> [13991.006983] Call Trace:
> [13991.007581]  [<ffffffffa05d6055>] ip6table_raw_net_exit+0x15/0x20 [ip6table_raw]
> [13991.008187]  [<ffffffff81527dc9>] ops_exit_list.isra.5+0x39/0x60
> [13991.008788]  [<ffffffff81528f50>] cleanup_net+0x260/0x480
> [13991.009380]  [<ffffffff8109ef4b>] process_one_work+0x17b/0x470
> [13991.009980]  [<ffffffff8109fd1b>] worker_thread+0x11b/0x400
> [13991.010576]  [<ffffffff8109fc00>] ? rescuer_thread+0x400/0x400
> [13991.011150]  [<ffffffff810a779f>] kthread+0xcf/0xe0
> [13991.011717]  [<ffffffff810a76d0>] ? create_kthread+0x60/0x60
> [13991.012619]  [<ffffffff81643cd8>] ret_from_fork+0x58/0x90
> [13991.013153]  [<ffffffff810a76d0>] ? create_kthread+0x60/0x60
> [13991.013668] Code: c4 28 5b 41 5c 41 5d 5d c3 e8 d5 c2 b5 e0 0f 1f 44 00 00 0f 1f 44 00 00 55 48 89 e5 41 57 41 56 49 89 fe 48 89 f7 41 55 41 54 53 <4c> 8b 7e 20 e8 7f 5f 04 e1 4c 8d 68 58 49 89 c4 8b 00 4c 89 eb
> [13991.014765] RIP  [<ffffffffa051f178>] ip6t_unregister_table+0x18/0x80 [ip6_tables]
> 
> Fixes: commit bff5233618d8 ("ve/net/ip6tables: fix autoloading of the
> ip6table_raw module from CT")
> 
> https://jira.sw.ru/browse/PSBM-54244
> 
> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
> Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
> ---

Acked-by: Andrey Ryabinin <aryabinin@virtuozzo.com>


> v2: make it safe for possible online ipt_mask reconfiguration,
>     that may come with some day in future
> v3: null-set table on ip6table_raw_net_exit()
> 
>  net/ipv6/netfilter/ip6table_raw.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/net/ipv6/netfilter/ip6table_raw.c b/net/ipv6/netfilter/ip6table_raw.c
> index 271835df64ce..b23d6c96899e 100644
> --- a/net/ipv6/netfilter/ip6table_raw.c
> +++ b/net/ipv6/netfilter/ip6table_raw.c
> @@ -33,6 +33,10 @@ static struct nf_hook_ops *rawtable_ops __read_mostly;
>  static int __net_init ip6table_raw_net_init(struct net *net)
>  {
>  	struct ip6t_replace *repl;
> +	struct xt_table *ip6table_raw;
> +
> +	if (WARN_ON(net->ipv6.ip6table_raw))
> +		net->ipv6.ip6table_raw = NULL;
>  
>  	if (!net_ipt_permitted(net, VE_IP_IPTABLES6))
>  		return 0;
> @@ -40,15 +44,23 @@ static int __net_init ip6table_raw_net_init(struct net *net)
>  	repl = ip6t_alloc_initial_table(&packet_raw);
>  	if (repl == NULL)
>  		return -ENOMEM;
> -	net->ipv6.ip6table_raw =
> -		ip6t_register_table(net, &packet_raw, repl);
> +	ip6table_raw = ip6t_register_table(net, &packet_raw, repl);
>  	kfree(repl);
> -	return PTR_RET(net->ipv6.ip6table_raw);
> +
> +	if (!IS_ERR(ip6table_raw))
> +		net->ipv6.ip6table_raw = ip6table_raw;
> +
> +	return PTR_RET(ip6table_raw);
>  }
>  
>  static void __net_exit ip6table_raw_net_exit(struct net *net)
>  {
> +	if (!net->ipv6.ip6table_raw)
> +		return;
> +
>  	ip6t_unregister_table(net, net->ipv6.ip6table_raw);
> +
> +	net->ipv6.ip6table_raw = NULL;
>  }
>  
>  static struct pernet_operations ip6table_raw_net_ops = {
>