[PATCH] netlink: Safer deletion of sk_bind_node

[PATCH] netlink: Safer deletion of sk_bind_node

[Harish, Jenny, K, N]

From: Harish Jenny K N <harish_kandiga@mentor.com>

    Unable to handle kernel NULL pointer dereference at virtual address 00000000
        (netlink_release+0x0/0x2a0) from [<8034e78c>] (sock_release+0x28/0xa4)
        (sock_release+0x0/0xa4) from [<8034e830>] (sock_close+0x28/0x34)
        (sock_close+0x0/0x34) from [<800f3490>] (__fput+0xf0/0x1ec)
        (__fput+0x0/0x1ec) from [<800f3634>] (____fput+0x10/0x14)
        (____fput+0x0/0x14) from [<80040a64>] (task_work_run+0xb8/0xd8)
        (task_work_run+0x0/0xd8) from [<800113a0>] (do_work_pending+0xb0/0xc4)
        (do_work_pending+0x0/0xc4) from [<8000d960>] (work_pending+0xc/0x20)
    Call flow of the inline and static functions
        netlink_release
        -----netlink_remove
        ---------__sk_del_bind_node
        --------------__hlist_del

Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
---
 net/netlink/af_netlink.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index c416725..21a6b32 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1103,7 +1103,7 @@ static void netlink_remove(struct sock *sk)
 
 	netlink_table_grab();
 	if (nlk_sk(sk)->subscriptions)
-		__sk_del_bind_node(sk);
+		hlist_del_init(&sk->sk_bind_node);
 	netlink_table_ungrab();
 }
 
@@ -1382,7 +1382,7 @@ netlink_update_subscriptions(struct sock *sk, unsigned int subscriptions)
 	struct netlink_sock *nlk = nlk_sk(sk);
 
 	if (nlk->subscriptions && !subscriptions)
-		__sk_del_bind_node(sk);
+		hlist_del_init(&sk->sk_bind_node);
 	else if (!nlk->subscriptions && subscriptions)
 		sk_add_bind_node(sk, &nl_table[sk->sk_protocol].mc_list);
 	nlk->subscriptions = subscriptions;
-- 
1.7.9.5

Re: [PATCH] netlink: Safer deletion of sk_bind_node

[Eric W. Biederman]

Harish Jenny K N writes:

> From: Harish Jenny K N <harish_kandiga@mentor.com>
>
>     Unable to handle kernel NULL pointer dereference at virtual address 00000000
>         (netlink_release+0x0/0x2a0) from [<8034e78c>] (sock_release+0x28/0xa4)
>         (sock_release+0x0/0xa4) from [<8034e830>] (sock_close+0x28/0x34)
>         (sock_close+0x0/0x34) from [<800f3490>] (__fput+0xf0/0x1ec)
>         (__fput+0x0/0x1ec) from [<800f3634>] (____fput+0x10/0x14)
>         (____fput+0x0/0x14) from [<80040a64>] (task_work_run+0xb8/0xd8)
>         (task_work_run+0x0/0xd8) from [<800113a0>] (do_work_pending+0xb0/0xc4)
>         (do_work_pending+0x0/0xc4) from [<8000d960>] (work_pending+0xc/0x20)
>     Call flow of the inline and static functions
>         netlink_release
>         -----netlink_remove
>         ---------__sk_del_bind_node
>         --------------__hlist_del

Is there any reason __sk_del_bind_node should not be changed instead?

If not there should be a description of what makes netlink's use of
__sk_del_bind_node special....

Eric

p.s. Your name was in your from line, but not your email address making
it hard to reply to you.

> Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
> ---
>  net/netlink/af_netlink.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index c416725..21a6b32 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -1103,7 +1103,7 @@ static void netlink_remove(struct sock *sk)
>  
>  	netlink_table_grab();
>  	if (nlk_sk(sk)->subscriptions)
> -		__sk_del_bind_node(sk);
> +		hlist_del_init(&sk->sk_bind_node);
>  	netlink_table_ungrab();
>  }
>  
> @@ -1382,7 +1382,7 @@ netlink_update_subscriptions(struct sock *sk, unsigned int subscriptions)
>  	struct netlink_sock *nlk = nlk_sk(sk);
>  
>  	if (nlk->subscriptions && !subscriptions)
> -		__sk_del_bind_node(sk);
> +		hlist_del_init(&sk->sk_bind_node);
>  	else if (!nlk->subscriptions && subscriptions)
>  		sk_add_bind_node(sk, &nl_table[sk->sk_protocol].mc_list);
>  	nlk->subscriptions = subscriptions;

Re: [PATCH] netlink: Safer deletion of sk_bind_node

[David Miller]

From: Harish Jenny K N
Date: Mon, 1 Sep 2014 12:38:29 +0530

Firstly, you really need to fix your outgoing email so that your email
address appears in your From: header properly.

> From: Harish Jenny K N <harish_kandiga@mentor.com>
> 
>     Unable to handle kernel NULL pointer dereference at virtual address 00000000
>         (netlink_release+0x0/0x2a0) from [<8034e78c>] (sock_release+0x28/0xa4)
>         (sock_release+0x0/0xa4) from [<8034e830>] (sock_close+0x28/0x34)
>         (sock_close+0x0/0x34) from [<800f3490>] (__fput+0xf0/0x1ec)
>         (__fput+0x0/0x1ec) from [<800f3634>] (____fput+0x10/0x14)
>         (____fput+0x0/0x14) from [<80040a64>] (task_work_run+0xb8/0xd8)
>         (task_work_run+0x0/0xd8) from [<800113a0>] (do_work_pending+0xb0/0xc4)
>         (do_work_pending+0x0/0xc4) from [<8000d960>] (work_pending+0xc/0x20)
>     Call flow of the inline and static functions
>         netlink_release
>         -----netlink_remove
>         ---------__sk_del_bind_node
>         --------------__hlist_del
> 
> Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>

This doesn't tell us anything about how this situation can be
arrived at.

When subscriptions changes, we delete the node with the table lock
held if subscriptions goes to zero.  We only try to delete the node
when subscriptions was zero.

Re: [RFC PATCH] netlink: Safer deletion of sk_bind_node

[Harish Jenny Kandiga Nagaraj]

In one of our random test runs we observed the crash mentioned in the previous mail.

After debugging we found out that the call flow of the inline and static functions were
netlink_release
-----netlink_remove
---------__sk_del_bind_node
--------------__hlist_del

*pprev was NULL in __hlist_del function while deleting &sk->sk_bind_node hlist_node. Hence the patch was given.

In netlink_remove function , first the sk_del_node_init function will be called. This internally calls __sk_del_node_init function. While deleting &sk->sk_node hlist_node using __sk_del_node function there is a NULL check with sk_hashed function.

Why there is no NULL check for *pprev while deleting &sk->sk_bind_node ?

On Tuesday 02 September 2014 10:33 AM, David Miller wrote:
> From: Harish Jenny K N
> Date: Mon, 1 Sep 2014 12:38:29 +0530
>
> Firstly, you really need to fix your outgoing email so that your email
> address appears in your From: header properly.
>
>> From: Harish Jenny K N <harish_kandiga@mentor.com>
>>
>>     Unable to handle kernel NULL pointer dereference at virtual address 00000000
>>         (netlink_release+0x0/0x2a0) from [<8034e78c>] (sock_release+0x28/0xa4)
>>         (sock_release+0x0/0xa4) from [<8034e830>] (sock_close+0x28/0x34)
>>         (sock_close+0x0/0x34) from [<800f3490>] (__fput+0xf0/0x1ec)
>>         (__fput+0x0/0x1ec) from [<800f3634>] (____fput+0x10/0x14)
>>         (____fput+0x0/0x14) from [<80040a64>] (task_work_run+0xb8/0xd8)
>>         (task_work_run+0x0/0xd8) from [<800113a0>] (do_work_pending+0xb0/0xc4)
>>         (do_work_pending+0x0/0xc4) from [<8000d960>] (work_pending+0xc/0x20)
>>     Call flow of the inline and static functions
>>         netlink_release
>>         -----netlink_remove
>>         ---------__sk_del_bind_node
>>         --------------__hlist_del
>>
>> Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
> This doesn't tell us anything about how this situation can be
> arrived at.
>
> When subscriptions changes, we delete the node with the table lock
> held if subscriptions goes to zero.  We only try to delete the node
> when subscriptions was zero.

Re: [RFC PATCH] netlink: Safer deletion of sk_bind_node

[David Miller]

From: Harish Jenny Kandiga Nagaraj <harish_kandiga@mentor.com>
Date: Tue, 2 Sep 2014 14:14:38 +0530

> In one of our random test runs we observed the crash mentioned in the previous mail.
> 
> After debugging we found out that the call flow of the inline and static functions were
> netlink_release
> -----netlink_remove
> ---------__sk_del_bind_node
> --------------__hlist_del
> 
> *pprev was NULL in __hlist_del function while deleting &sk->sk_bind_node hlist_node. Hence the patch was given.
> 
> In netlink_remove function , first the sk_del_node_init function will be called. This internally calls __sk_del_node_init function. While deleting &sk->sk_node hlist_node using __sk_del_node function there is a NULL check with sk_hashed function.
> 
> Why there is no NULL check for *pprev while deleting &sk->sk_bind_node ?

Because if ->subscriptions is non-zero, it must be on a list, and therefore
pprev must be non-NULL.

Re: [RFC PATCH] netlink: Safer deletion of sk_bind_node

[Harish Jenny Kandiga Nagaraj]

If that is the case , then subscriptions of netlink_sock should have been updated after netlink_remove or netlink_release.
I don't see that happening.

On Wednesday 03 September 2014 12:22 AM, David Miller wrote:
> From: Harish Jenny Kandiga Nagaraj <harish_kandiga@mentor.com>
> Date: Tue, 2 Sep 2014 14:14:38 +0530
>
>> In one of our random test runs we observed the crash mentioned in the previous mail.
>>
>> After debugging we found out that the call flow of the inline and static functions were
>> netlink_release
>> -----netlink_remove
>> ---------__sk_del_bind_node
>> --------------__hlist_del
>>
>> *pprev was NULL in __hlist_del function while deleting &sk->sk_bind_node hlist_node. Hence the patch was given.
>>
>> In netlink_remove function , first the sk_del_node_init function will be called. This internally calls __sk_del_node_init function. While deleting &sk->sk_node hlist_node using __sk_del_node function there is a NULL check with sk_hashed function.
>>
>> Why there is no NULL check for *pprev while deleting &sk->sk_bind_node ?
> Because if ->subscriptions is non-zero, it must be on a list, and therefore
> pprev must be non-NULL.

Re: [RFC PATCH] netlink: Safer deletion of sk_bind_node

[David Miller]

From: Harish Jenny Kandiga Nagaraj <harish_kandiga@mentor.com>
Date: Wed, 3 Sep 2014 10:49:59 +0530

> If that is the case , then subscriptions of netlink_sock should have
> been updated after netlink_remove or netlink_release.  I don't see
> that happening.

Please do not top-post.  First provide the quoted context, then provide
your response afterwards, rather than the other way around.

The point I was trying to make is that all code paths that operate
on this list have to follow this rule, if there is a code path that
is not then that is your bug and that is where the fix belongs.

We're unexpectedly having double deletes occur on this list, I would
prefer if we don't paper over it by allowing it to silently succeed.