[PATCH net] netlink: access nlk groups safely in netlink bind and getname

[PATCH net] netlink: access nlk groups safely in netlink bind and getname

[Xin Long]

Now there is no lock protecting nlk ngroups/groups' accessing in
netlink bind and getname. It's safe from nlk groups' setting in
netlink_release, but not from netlink_realloc_groups called by
netlink_setsockopt.

netlink_lock_table is needed in both netlink bind and getname when
accessing nlk groups.

Acked-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/netlink/af_netlink.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 94a61e6..3278077 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -955,7 +955,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	struct net *net = sock_net(sk);
 	struct netlink_sock *nlk = nlk_sk(sk);
 	struct sockaddr_nl *nladdr = (struct sockaddr_nl *)addr;
-	int err;
+	int err = 0;
 	long unsigned int groups = nladdr->nl_groups;
 	bool bound;
 
@@ -983,6 +983,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 			return -EINVAL;
 	}
 
+	netlink_lock_table();
 	if (nlk->netlink_bind && groups) {
 		int group;
 
@@ -993,7 +994,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 			if (!err)
 				continue;
 			netlink_undo_bind(group, groups, sk);
-			return err;
+			goto unlock;
 		}
 	}
 
@@ -1006,12 +1007,13 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 			netlink_autobind(sock);
 		if (err) {
 			netlink_undo_bind(nlk->ngroups, groups, sk);
-			return err;
+			goto unlock;
 		}
 	}
 
 	if (!groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
-		return 0;
+		goto unlock;
+	netlink_unlock_table();
 
 	netlink_table_grab();
 	netlink_update_subscriptions(sk, nlk->subscriptions +
@@ -1022,6 +1024,10 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	netlink_table_ungrab();
 
 	return 0;
+
+unlock:
+	netlink_unlock_table();
+	return err;
 }
 
 static int netlink_connect(struct socket *sock, struct sockaddr *addr,
@@ -1079,7 +1085,9 @@ static int netlink_getname(struct socket *sock, struct sockaddr *addr,
 		nladdr->nl_groups = netlink_group_mask(nlk->dst_group);
 	} else {
 		nladdr->nl_pid = nlk->portid;
+		netlink_lock_table();
 		nladdr->nl_groups = nlk->groups ? nlk->groups[0] : 0;
+		netlink_unlock_table();
 	}
 	return 0;
 }
-- 
2.1.0

Re: [PATCH net] netlink: access nlk groups safely in netlink bind and getname

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Wed,  6 Sep 2017 11:53:29 +0800

> Now there is no lock protecting nlk ngroups/groups' accessing in
> netlink bind and getname. It's safe from nlk groups' setting in
> netlink_release, but not from netlink_realloc_groups called by
> netlink_setsockopt.
> 
> netlink_lock_table is needed in both netlink bind and getname when
> accessing nlk groups.
> 
> Acked-by: Florian Westphal <fw@strlen.de>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Applied.

Re: [PATCH net] netlink: access nlk groups safely in netlink bind and getname

[Cong Wang]

On Tue, Sep 5, 2017 at 8:53 PM, Xin Long <lucien.xin@gmail.com> wrote:
> Now there is no lock protecting nlk ngroups/groups' accessing in
> netlink bind and getname. It's safe from nlk groups' setting in
> netlink_release, but not from netlink_realloc_groups called by
> netlink_setsockopt.
>
> netlink_lock_table is needed in both netlink bind and getname when
> accessing nlk groups.

This looks very odd.

netlink_lock_table() should be protecting nl_table, why
it also protects nlk->groups?? For me it looks like you
need lock_sock() instead.

Re: [PATCH net] netlink: access nlk groups safely in netlink bind and getname

[Xin Long]

On Sat, Sep 9, 2017 at 7:35 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Tue, Sep 5, 2017 at 8:53 PM, Xin Long <lucien.xin@gmail.com> wrote:
>> Now there is no lock protecting nlk ngroups/groups' accessing in
>> netlink bind and getname. It's safe from nlk groups' setting in
>> netlink_release, but not from netlink_realloc_groups called by
>> netlink_setsockopt.
>>
>> netlink_lock_table is needed in both netlink bind and getname when
>> accessing nlk groups.
>
> This looks very odd.
>
> netlink_lock_table() should be protecting nl_table, why
> it also protects nlk->groups?? For me it looks like you
> need lock_sock() instead.
I believe netlink_lock_table might be only used to protect nl_table
at the beginning and surely lock_sock is better here. Thanks.

But can you explain why  netlink_lock_table() was also used in
netlink_getsockopt NETLINK_LIST_MEMBERSHIPS ? or it
was just a mistake ?

Re: [PATCH net] netlink: access nlk groups safely in netlink bind and getname

[Cong Wang]

On Sun, Sep 10, 2017 at 4:45 AM, Xin Long <lucien.xin@gmail.com> wrote:
> On Sat, Sep 9, 2017 at 7:35 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Tue, Sep 5, 2017 at 8:53 PM, Xin Long <lucien.xin@gmail.com> wrote:
>>> Now there is no lock protecting nlk ngroups/groups' accessing in
>>> netlink bind and getname. It's safe from nlk groups' setting in
>>> netlink_release, but not from netlink_realloc_groups called by
>>> netlink_setsockopt.
>>>
>>> netlink_lock_table is needed in both netlink bind and getname when
>>> accessing nlk groups.
>>
>> This looks very odd.
>>
>> netlink_lock_table() should be protecting nl_table, why
>> it also protects nlk->groups?? For me it looks like you
>> need lock_sock() instead.
> I believe netlink_lock_table might be only used to protect nl_table
> at the beginning and surely lock_sock is better here. Thanks.
>
> But can you explain why  netlink_lock_table() was also used in
> netlink_getsockopt NETLINK_LIST_MEMBERSHIPS ? or it
> was just a mistake ?

No, it is fine but not necessary, because netlink_realloc_groups()
doesn't change nl_table, it only changes nlk->groups. So we
don't have take the global write lock, the lock sock makes more
sense here, same for your bind() and getname() case.