diff mbox series

[net,v3] tipc: fix missing RTNL lock protection during setting link properties

Message ID 1518512686-4266-1-git-send-email-ying.xue@windriver.com
State Superseded, archived
Delegated to: David Miller
Headers show
Series [net,v3] tipc: fix missing RTNL lock protection during setting link properties | expand

Commit Message

Ying Xue Feb. 13, 2018, 9:04 a.m. UTC 
Currently when user changes link properties, TIPC first checks if
user's command message contains media name or bearer name through
tipc_media_find() or tipc_bearer_find() which is protected by RTNL
lock. But when tipc_nl_compat_link_set() conducts the checking with
the two functions, it doesn't hold RTNL lock at all, as a result,
the following complaints were reported:

audit: type=1400 audit(1514679888.244:9): avc:  denied  { write } for
pid=3194 comm="syzkaller021477" path="socket:[11143]" dev="sockfs"
ino=11143 scontext=unconfined_u:system_r:insmod_t:s0-s0:c0.c1023
tcontext=unconfined_u:system_r:insmod_t:s0-s0:c0.c1023
tclass=netlink_generic_socket permissive=1

Comments

Kirill Tkhai Feb. 13, 2018, 11:03 a.m. UTC | #1 
Hi, Ying,

On 13.02.2018 12:04, Ying Xue wrote:
> Currently when user changes link properties, TIPC first checks if
> user's command message contains media name or bearer name through
> tipc_media_find() or tipc_bearer_find() which is protected by RTNL
> lock. But when tipc_nl_compat_link_set() conducts the checking with
> the two functions, it doesn't hold RTNL lock at all, as a result,
> the following complaints were reported:
> 
> audit: type=1400 audit(1514679888.244:9): avc:  denied  { write } for
> pid=3194 comm="syzkaller021477" path="socket:[11143]" dev="sockfs"
> ino=11143 scontext=unconfined_u:system_r:insmod_t:s0-s0:c0.c1023
> tcontext=unconfined_u:system_r:insmod_t:s0-s0:c0.c1023
> tclass=netlink_generic_socket permissive=1
> =============================
> WARNING: suspicious RCU usage
> 4.15.0-rc5+ #152 Not tainted
> -----------------------------
> net/tipc/bearer.c:177 suspicious rcu_dereference_protected() usage!
> 
> other info that might help us debug this:
> 
> rcu_scheduler_active = 2, debug_locks = 1
> 2 locks held by syzkaller021477/3194:
>   #0:  (cb_lock){++++}, at: [<00000000d20133ea>] genl_rcv+0x19/0x40
> net/netlink/genetlink.c:634
>   #1:  (genl_mutex){+.+.}, at: [<00000000fcc5d1bc>] genl_lock
> net/netlink/genetlink.c:33 [inline]
>   #1:  (genl_mutex){+.+.}, at: [<00000000fcc5d1bc>] genl_rcv_msg+0x115/0x140
> net/netlink/genetlink.c:622
> 
> stack backtrace:
> CPU: 1 PID: 3194 Comm: syzkaller021477 Not tainted 4.15.0-rc5+ #152
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
>   __dump_stack lib/dump_stack.c:17 [inline]
>   dump_stack+0x194/0x257 lib/dump_stack.c:53
>   lockdep_rcu_suspicious+0x123/0x170 kernel/locking/lockdep.c:4585
>   tipc_bearer_find+0x2b4/0x3b0 net/tipc/bearer.c:177
>   tipc_nl_compat_link_set+0x329/0x9f0 net/tipc/netlink_compat.c:729
>   __tipc_nl_compat_doit net/tipc/netlink_compat.c:288 [inline]
>   tipc_nl_compat_doit+0x15b/0x660 net/tipc/netlink_compat.c:335
>   tipc_nl_compat_handle net/tipc/netlink_compat.c:1119 [inline]
>   tipc_nl_compat_recv+0x112f/0x18f0 net/tipc/netlink_compat.c:1201
>   genl_family_rcv_msg+0x7b7/0xfb0 net/netlink/genetlink.c:599
>   genl_rcv_msg+0xb2/0x140 net/netlink/genetlink.c:624
>   netlink_rcv_skb+0x21e/0x460 net/netlink/af_netlink.c:2408
>   genl_rcv+0x28/0x40 net/netlink/genetlink.c:635
>   netlink_unicast_kernel net/netlink/af_netlink.c:1275 [inline]
>   netlink_unicast+0x4e8/0x6f0 net/netlink/af_netlink.c:1301
>   netlink_sendmsg+0xa4a/0xe60 net/netlink/af_netlink.c:1864
>   sock_sendmsg_nosec net/socket.c:636 [inline]
>   sock_sendmsg+0xca/0x110 net/socket.c:646
>   sock_write_iter+0x31a/0x5d0 net/socket.c:915
>   call_write_iter include/linux/fs.h:1772 [inline]
>   new_sync_write fs/read_write.c:469 [inline]
>   __vfs_write+0x684/0x970 fs/read_write.c:482
>   vfs_write+0x189/0x510 fs/read_write.c:544
>   SYSC_write fs/read_write.c:589 [inline]
>   SyS_write+0xef/0x220 fs/read_write.c:581
>   do_syscall_32_irqs_on arch/x86/entry/common.c:327 [inline]
>   do_fast_syscall_32+0x3ee/0xf9d arch/x86/entry/common.c:389
>   entry_SYSENTER_compat+0x54/0x63 arch/x86/entry/entry_64_compat.S:129
> 
> In order to correct the mistake, __tipc_nl_compat_doit() has been
> protected by RTNL lock, which means the whole operation of setting
> bearer/media properties is under RTNL protection.
> 
> Signed-off-by: Ying Xue <ying.xue@windriver.com>
> Reported-by: syzbot <syzbot+6345fd433db009b29413@syzkaller.appspotmail.com>
> ---
> v3: 
>     - Optimized return method of __tipc_nl_bearer_enable() regarding
>       the comments from David M and Kirill Tkhai
>     - Moved the allocations of memory in __tipc_nl_compat_doit() out
>       of RTNL lock to minimize the time of holding RTNL lock according
>       to the suggestion of Kirill Tkhai.
> v2:
>     - The whole operation of setting bearer/media properties has been
>       protected under RTNL, as per feedback from David M.
> 
>  net/tipc/bearer.c         | 82 +++++++++++++++++++++++++++++------------------
>  net/tipc/bearer.h         |  4 +++
>  net/tipc/net.c            | 15 +++++++--
>  net/tipc/net.h            |  1 +
>  net/tipc/netlink_compat.c | 43 +++++++++++++------------
>  5 files changed, 91 insertions(+), 54 deletions(-)
> 
> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> index c800147..3e3dce3 100644
> --- a/net/tipc/bearer.c
> +++ b/net/tipc/bearer.c
> @@ -813,7 +813,7 @@ int tipc_nl_bearer_get(struct sk_buff *skb, struct genl_info *info)
>  	return err;
>  }
>  
> -int tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info)
> +int __tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info)
>  {
>  	int err;
>  	char *name;
> @@ -835,20 +835,27 @@ int tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info)
>  
>  	name = nla_data(attrs[TIPC_NLA_BEARER_NAME]);
>  
> -	rtnl_lock();
>  	bearer = tipc_bearer_find(net, name);
> -	if (!bearer) {
> -		rtnl_unlock();
> +	if (!bearer)
>  		return -EINVAL;
> -	}
>  
>  	bearer_disable(net, bearer);
> -	rtnl_unlock();
>  
>  	return 0;
>  }
>  
> -int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
> +int tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info)
> +{
> +	int err;
> +
> +	rtnl_lock();
> +	err = __tipc_nl_bearer_disable(skb, info);
> +	rtnl_unlock();
> +
> +	return err;
> +}
> +
> +int __tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
>  {
>  	int err;
>  	char *bearer;
> @@ -890,15 +897,18 @@ int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
>  			prio = nla_get_u32(props[TIPC_NLA_PROP_PRIO]);
>  	}
>  
> +	return tipc_enable_bearer(net, bearer, domain, prio, attrs);
> +}
> +
> +int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
> +{
> +	int err;
> +
>  	rtnl_lock();
> -	err = tipc_enable_bearer(net, bearer, domain, prio, attrs);
> -	if (err) {
> -		rtnl_unlock();
> -		return err;
> -	}
> +	err = __tipc_nl_bearer_enable(skb, info);
>  	rtnl_unlock();
>  
> -	return 0;
> +	return err;
>  }
>  
>  int tipc_nl_bearer_add(struct sk_buff *skb, struct genl_info *info)
> @@ -944,7 +954,7 @@ int tipc_nl_bearer_add(struct sk_buff *skb, struct genl_info *info)
>  	return 0;
>  }
>  
> -int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
> +int __tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
>  {
>  	int err;
>  	char *name;
> @@ -965,22 +975,17 @@ int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
>  		return -EINVAL;
>  	name = nla_data(attrs[TIPC_NLA_BEARER_NAME]);
>  
> -	rtnl_lock();
>  	b = tipc_bearer_find(net, name);
> -	if (!b) {
> -		rtnl_unlock();
> +	if (!b)
>  		return -EINVAL;
> -	}
>  
>  	if (attrs[TIPC_NLA_BEARER_PROP]) {
>  		struct nlattr *props[TIPC_NLA_PROP_MAX + 1];
>  
>  		err = tipc_nl_parse_link_prop(attrs[TIPC_NLA_BEARER_PROP],
>  					      props);
> -		if (err) {
> -			rtnl_unlock();
> +		if (err)
>  			return err;
> -		}
>  
>  		if (props[TIPC_NLA_PROP_TOL])
>  			b->tolerance = nla_get_u32(props[TIPC_NLA_PROP_TOL]);
> @@ -989,11 +994,21 @@ int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
>  		if (props[TIPC_NLA_PROP_WIN])
>  			b->window = nla_get_u32(props[TIPC_NLA_PROP_WIN]);
>  	}
> -	rtnl_unlock();
>  
>  	return 0;
>  }
>  
> +int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
> +{
> +	int err;
> +
> +	rtnl_lock();
> +	err = __tipc_nl_bearer_set(skb, info);
> +	rtnl_unlock();
> +
> +	return err;
> +}
> +
>  static int __tipc_nl_add_media(struct tipc_nl_msg *msg,
>  			       struct tipc_media *media, int nlflags)
>  {
> @@ -1115,7 +1130,7 @@ int tipc_nl_media_get(struct sk_buff *skb, struct genl_info *info)
>  	return err;
>  }
>  
> -int tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info)
> +int __tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info)
>  {
>  	int err;
>  	char *name;
> @@ -1133,22 +1148,17 @@ int tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info)
>  		return -EINVAL;
>  	name = nla_data(attrs[TIPC_NLA_MEDIA_NAME]);
>  
> -	rtnl_lock();
>  	m = tipc_media_find(name);
> -	if (!m) {
> -		rtnl_unlock();
> +	if (!m)
>  		return -EINVAL;
> -	}
>  
>  	if (attrs[TIPC_NLA_MEDIA_PROP]) {
>  		struct nlattr *props[TIPC_NLA_PROP_MAX + 1];
>  
>  		err = tipc_nl_parse_link_prop(attrs[TIPC_NLA_MEDIA_PROP],
>  					      props);
> -		if (err) {
> -			rtnl_unlock();
> +		if (err)
>  			return err;
> -		}
>  
>  		if (props[TIPC_NLA_PROP_TOL])
>  			m->tolerance = nla_get_u32(props[TIPC_NLA_PROP_TOL]);
> @@ -1157,7 +1167,17 @@ int tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info)
>  		if (props[TIPC_NLA_PROP_WIN])
>  			m->window = nla_get_u32(props[TIPC_NLA_PROP_WIN]);
>  	}
> -	rtnl_unlock();
>  
>  	return 0;
>  }
> +
> +int tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info)
> +{
> +	int err;
> +
> +	rtnl_lock();
> +	err = __tipc_nl_media_set(skb, info);
> +	rtnl_unlock();
> +
> +	return err;
> +}
> diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
> index 42d6eee..a53613d 100644
> --- a/net/tipc/bearer.h
> +++ b/net/tipc/bearer.h
> @@ -188,15 +188,19 @@ extern struct tipc_media udp_media_info;
>  #endif
>  
>  int tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info);
> +int __tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info);
>  int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info);
> +int __tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info);
>  int tipc_nl_bearer_dump(struct sk_buff *skb, struct netlink_callback *cb);
>  int tipc_nl_bearer_get(struct sk_buff *skb, struct genl_info *info);
>  int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info);
> +int __tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info);
>  int tipc_nl_bearer_add(struct sk_buff *skb, struct genl_info *info);
>  
>  int tipc_nl_media_dump(struct sk_buff *skb, struct netlink_callback *cb);
>  int tipc_nl_media_get(struct sk_buff *skb, struct genl_info *info);
>  int tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info);
> +int __tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info);
>  
>  int tipc_media_set_priority(const char *name, u32 new_value);
>  int tipc_media_set_window(const char *name, u32 new_value);
> diff --git a/net/tipc/net.c b/net/tipc/net.c
> index 719c592..1a2fde0 100644
> --- a/net/tipc/net.c
> +++ b/net/tipc/net.c
> @@ -200,7 +200,7 @@ int tipc_nl_net_dump(struct sk_buff *skb, struct netlink_callback *cb)
>  	return skb->len;
>  }
>  
> -int tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info)
> +int __tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info)
>  {
>  	struct net *net = sock_net(skb->sk);
>  	struct tipc_net *tn = net_generic(net, tipc_net_id);
> @@ -241,10 +241,19 @@ int tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info)
>  		if (!tipc_addr_node_valid(addr))
>  			return -EINVAL;
>  
> -		rtnl_lock();
>  		tipc_net_start(net, addr);
> -		rtnl_unlock();
>  	}
>  
>  	return 0;
>  }
> +
> +int tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info)
> +{
> +	int err;
> +
> +	rtnl_lock();
> +	err = __tipc_nl_net_set(skb, info);
> +	rtnl_unlock();
> +
> +	return err;
> +}
> diff --git a/net/tipc/net.h b/net/tipc/net.h
> index c7c2549..c0306aa 100644
> --- a/net/tipc/net.h
> +++ b/net/tipc/net.h
> @@ -47,5 +47,6 @@ void tipc_net_stop(struct net *net);
>  
>  int tipc_nl_net_dump(struct sk_buff *skb, struct netlink_callback *cb);
>  int tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info);
> +int __tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info);
>  
>  #endif
> diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
> index e48f0b2..4492cda 100644
> --- a/net/tipc/netlink_compat.c
> +++ b/net/tipc/netlink_compat.c
> @@ -285,10 +285,6 @@ static int __tipc_nl_compat_doit(struct tipc_nl_compat_cmd_doit *cmd,
>  	if (!trans_buf)
>  		return -ENOMEM;
>  
> -	err = (*cmd->transcode)(cmd, trans_buf, msg);
> -	if (err)
> -		goto trans_out;
> -
>  	attrbuf = kmalloc((tipc_genl_family.maxattr + 1) *
>  			sizeof(struct nlattr *), GFP_KERNEL);
>  	if (!attrbuf) {
> @@ -296,27 +292,34 @@ static int __tipc_nl_compat_doit(struct tipc_nl_compat_cmd_doit *cmd,
>  		goto trans_out;
>  	}
>  
> -	err = nla_parse(attrbuf, tipc_genl_family.maxattr,
> -			(const struct nlattr *)trans_buf->data,
> -			trans_buf->len, NULL, NULL);
> -	if (err)
> -		goto parse_out;
> -
>  	doit_buf = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
>  	if (!doit_buf) {
>  		err = -ENOMEM;
> -		goto parse_out;
> +		goto attrbuf_out;
>  	}
>  
> -	doit_buf->sk = msg->dst_sk;
> -
>  	memset(&info, 0, sizeof(info));
>  	info.attrs = attrbuf;
>  
> +	rtnl_lock();
> +	err = (*cmd->transcode)(cmd, trans_buf, msg);
> +	if (err)
> +		goto doit_out;
> +
> +	err = nla_parse(attrbuf, tipc_genl_family.maxattr,
> +			(const struct nlattr *)trans_buf->data,
> +			trans_buf->len, NULL, NULL);
> +	if (err)
> +		goto doit_out;
> +
> +	doit_buf->sk = msg->dst_sk;
> +
>  	err = (*cmd->doit)(doit_buf, &info);
> +doit_out:
> +	rtnl_unlock();
>  
>  	kfree_skb(doit_buf);
> -parse_out:
> +attrbuf_out:
>  	kfree(attrbuf);
>  trans_out:
>  	kfree_skb(trans_buf);
> @@ -722,13 +725,13 @@ static int tipc_nl_compat_link_set(struct tipc_nl_compat_cmd_doit *cmd,
>  
>  	media = tipc_media_find(lc->name);
>  	if (media) {
> -		cmd->doit = &tipc_nl_media_set;
> +		cmd->doit = &__tipc_nl_media_set;
>  		return tipc_nl_compat_media_set(skb, msg);
>  	}
>  
>  	bearer = tipc_bearer_find(msg->net, lc->name);
>  	if (bearer) {
> -		cmd->doit = &tipc_nl_bearer_set;
> +		cmd->doit = &__tipc_nl_bearer_set;
>  		return tipc_nl_compat_bearer_set(skb, msg);
>  	}
>  
> @@ -1089,12 +1092,12 @@ static int tipc_nl_compat_handle(struct tipc_nl_compat_msg *msg)
>  		return tipc_nl_compat_dumpit(&dump, msg);
>  	case TIPC_CMD_ENABLE_BEARER:
>  		msg->req_type = TIPC_TLV_BEARER_CONFIG;
> -		doit.doit = tipc_nl_bearer_enable;
> +		doit.doit = __tipc_nl_bearer_enable;
>  		doit.transcode = tipc_nl_compat_bearer_enable;
>  		return tipc_nl_compat_doit(&doit, msg);
>  	case TIPC_CMD_DISABLE_BEARER:
>  		msg->req_type = TIPC_TLV_BEARER_NAME;
> -		doit.doit = tipc_nl_bearer_disable;
> +		doit.doit = __tipc_nl_bearer_disable;
>  		doit.transcode = tipc_nl_compat_bearer_disable;
>  		return tipc_nl_compat_doit(&doit, msg);
>  	case TIPC_CMD_SHOW_LINK_STATS:
> @@ -1148,12 +1151,12 @@ static int tipc_nl_compat_handle(struct tipc_nl_compat_msg *msg)
>  		return tipc_nl_compat_dumpit(&dump, msg);
>  	case TIPC_CMD_SET_NODE_ADDR:
>  		msg->req_type = TIPC_TLV_NET_ADDR;
> -		doit.doit = tipc_nl_net_set;
> +		doit.doit = __tipc_nl_net_set;
>  		doit.transcode = tipc_nl_compat_net_set;
>  		return tipc_nl_compat_doit(&doit, msg);
>  	case TIPC_CMD_SET_NETID:
>  		msg->req_type = TIPC_TLV_UNSIGNED;
> -		doit.doit = tipc_nl_net_set;
> +		doit.doit = __tipc_nl_net_set;
>  		doit.transcode = tipc_nl_compat_net_set;
>  		return tipc_nl_compat_doit(&doit, msg);
>  	case TIPC_CMD_GET_NETID:

The patch is logically OK for me. The only thing I'm confused,
I had to split it in 7 patches to review, otherwise the patch
looks difficult to do. There is possible to extract:

1)Refactoring in __tipc_nl_compat_doit
2)Introduce __tipc_nl_bearer_disable()
3)Introduce __tipc_nl_bearer_enable()
4)Introduce __tipc_nl_bearer_set()
5)Introduce __tipc_nl_media_set()
6)Introduce __tipc_nl_net_set()
7)tipc: fix missing RTNL lock protection during setting link properties

After that it becomes very easy to review the change you made in this patch.

I attached them to message, and you may take the patches if there is one
more version.

Thanks,
Kirill
Introduce tipc_nl_net_set()


---
 net/tipc/net.c |   15 ++++++++++++---
 net/tipc/net.h |    1 +
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/net/tipc/net.c b/net/tipc/net.c
index 719c5924b638..1a2fde0d6f61 100644
--- a/net/tipc/net.c
+++ b/net/tipc/net.c
@@ -200,7 +200,7 @@ int tipc_nl_net_dump(struct sk_buff *skb, struct netlink_callback *cb)
 	return skb->len;
 }
 
-int tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info)
+int __tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info)
 {
 	struct net *net = sock_net(skb->sk);
 	struct tipc_net *tn = net_generic(net, tipc_net_id);
@@ -241,10 +241,19 @@ int tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info)
 		if (!tipc_addr_node_valid(addr))
 			return -EINVAL;
 
-		rtnl_lock();
 		tipc_net_start(net, addr);
-		rtnl_unlock();
 	}
 
 	return 0;
 }
+
+int tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info)
+{
+	int err;
+
+	rtnl_lock();
+	err = __tipc_nl_net_set(skb, info);
+	rtnl_unlock();
+
+	return err;
+}
diff --git a/net/tipc/net.h b/net/tipc/net.h
index c7c254902873..c0306aa2374b 100644
--- a/net/tipc/net.h
+++ b/net/tipc/net.h
@@ -47,5 +47,6 @@ void tipc_net_stop(struct net *net);
 
 int tipc_nl_net_dump(struct sk_buff *skb, struct netlink_callback *cb);
 int tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info);
+int __tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info);
 
 #endif
Introduce __tipc_nl_media_set()


---
 net/tipc/bearer.c |   23 ++++++++++++++---------
 net/tipc/bearer.h |    1 +
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index f92c9c58d686..3e3dce3d4c63 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -1130,7 +1130,7 @@ int tipc_nl_media_get(struct sk_buff *skb, struct genl_info *info)
 	return err;
 }
 
-int tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info)
+int __tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info)
 {
 	int err;
 	char *name;
@@ -1148,22 +1148,17 @@ int tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info)
 		return -EINVAL;
 	name = nla_data(attrs[TIPC_NLA_MEDIA_NAME]);
 
-	rtnl_lock();
 	m = tipc_media_find(name);
-	if (!m) {
-		rtnl_unlock();
+	if (!m)
 		return -EINVAL;
-	}
 
 	if (attrs[TIPC_NLA_MEDIA_PROP]) {
 		struct nlattr *props[TIPC_NLA_PROP_MAX + 1];
 
 		err = tipc_nl_parse_link_prop(attrs[TIPC_NLA_MEDIA_PROP],
 					      props);
-		if (err) {
-			rtnl_unlock();
+		if (err)
 			return err;
-		}
 
 		if (props[TIPC_NLA_PROP_TOL])
 			m->tolerance = nla_get_u32(props[TIPC_NLA_PROP_TOL]);
@@ -1172,7 +1167,17 @@ int tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info)
 		if (props[TIPC_NLA_PROP_WIN])
 			m->window = nla_get_u32(props[TIPC_NLA_PROP_WIN]);
 	}
-	rtnl_unlock();
 
 	return 0;
 }
+
+int tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info)
+{
+	int err;
+
+	rtnl_lock();
+	err = __tipc_nl_media_set(skb, info);
+	rtnl_unlock();
+
+	return err;
+}
diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
index cc0f529a56b5..a53613d95bc9 100644
--- a/net/tipc/bearer.h
+++ b/net/tipc/bearer.h
@@ -200,6 +200,7 @@ int tipc_nl_bearer_add(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_media_dump(struct sk_buff *skb, struct netlink_callback *cb);
 int tipc_nl_media_get(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info);
+int __tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info);
 
 int tipc_media_set_priority(const char *name, u32 new_value);
 int tipc_media_set_window(const char *name, u32 new_value);
Introduce __tipc_nl_bearer_set()


---
 net/tipc/bearer.c |   23 ++++++++++++++---------
 net/tipc/bearer.h |    1 +
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index faf8fa033740..f92c9c58d686 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -954,7 +954,7 @@ int tipc_nl_bearer_add(struct sk_buff *skb, struct genl_info *info)
 	return 0;
 }
 
-int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
+int __tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
 {
 	int err;
 	char *name;
@@ -975,22 +975,17 @@ int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
 		return -EINVAL;
 	name = nla_data(attrs[TIPC_NLA_BEARER_NAME]);
 
-	rtnl_lock();
 	b = tipc_bearer_find(net, name);
-	if (!b) {
-		rtnl_unlock();
+	if (!b)
 		return -EINVAL;
-	}
 
 	if (attrs[TIPC_NLA_BEARER_PROP]) {
 		struct nlattr *props[TIPC_NLA_PROP_MAX + 1];
 
 		err = tipc_nl_parse_link_prop(attrs[TIPC_NLA_BEARER_PROP],
 					      props);
-		if (err) {
-			rtnl_unlock();
+		if (err)
 			return err;
-		}
 
 		if (props[TIPC_NLA_PROP_TOL])
 			b->tolerance = nla_get_u32(props[TIPC_NLA_PROP_TOL]);
@@ -999,11 +994,21 @@ int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
 		if (props[TIPC_NLA_PROP_WIN])
 			b->window = nla_get_u32(props[TIPC_NLA_PROP_WIN]);
 	}
-	rtnl_unlock();
 
 	return 0;
 }
 
+int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
+{
+	int err;
+
+	rtnl_lock();
+	err = __tipc_nl_bearer_set(skb, info);
+	rtnl_unlock();
+
+	return err;
+}
+
 static int __tipc_nl_add_media(struct tipc_nl_msg *msg,
 			       struct tipc_media *media, int nlflags)
 {
diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
index fc81150ca9c9..cc0f529a56b5 100644
--- a/net/tipc/bearer.h
+++ b/net/tipc/bearer.h
@@ -194,6 +194,7 @@ int __tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_bearer_dump(struct sk_buff *skb, struct netlink_callback *cb);
 int tipc_nl_bearer_get(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info);
+int __tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_bearer_add(struct sk_buff *skb, struct genl_info *info);
 
 int tipc_nl_media_dump(struct sk_buff *skb, struct netlink_callback *cb);
Introduce __tipc_nl_bearer_enable()


---
 net/tipc/bearer.c |   17 ++++++++++-------
 net/tipc/bearer.h |    1 +
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 61b6625f93a4..faf8fa033740 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -855,7 +855,7 @@ int tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info)
 	return err;
 }
 
-int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
+int __tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
 {
 	int err;
 	char *bearer;
@@ -897,15 +897,18 @@ int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
 			prio = nla_get_u32(props[TIPC_NLA_PROP_PRIO]);
 	}
 
+	return tipc_enable_bearer(net, bearer, domain, prio, attrs);
+}
+
+int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
+{
+	int err;
+
 	rtnl_lock();
-	err = tipc_enable_bearer(net, bearer, domain, prio, attrs);
-	if (err) {
-		rtnl_unlock();
-		return err;
-	}
+	err = __tipc_nl_bearer_enable(skb, info);
 	rtnl_unlock();
 
-	return 0;
+	return err;
 }
 
 int tipc_nl_bearer_add(struct sk_buff *skb, struct genl_info *info)
diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
index bcc6d5f7014b..fc81150ca9c9 100644
--- a/net/tipc/bearer.h
+++ b/net/tipc/bearer.h
@@ -190,6 +190,7 @@ extern struct tipc_media udp_media_info;
 int tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info);
 int __tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info);
+int __tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_bearer_dump(struct sk_buff *skb, struct netlink_callback *cb);
 int tipc_nl_bearer_get(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info);
Introduce __tipc_nl_bearer_disable()


---
 net/tipc/bearer.c |   19 +++++++++++++------
 net/tipc/bearer.h |    1 +
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index c8001471da6c..61b6625f93a4 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -813,7 +813,7 @@ int tipc_nl_bearer_get(struct sk_buff *skb, struct genl_info *info)
 	return err;
 }
 
-int tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info)
+int __tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info)
 {
 	int err;
 	char *name;
@@ -835,19 +835,26 @@ int tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info)
 
 	name = nla_data(attrs[TIPC_NLA_BEARER_NAME]);
 
-	rtnl_lock();
 	bearer = tipc_bearer_find(net, name);
-	if (!bearer) {
-		rtnl_unlock();
+	if (!bearer)
 		return -EINVAL;
-	}
 
 	bearer_disable(net, bearer);
-	rtnl_unlock();
 
 	return 0;
 }
 
+int tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info)
+{
+	int err;
+
+	rtnl_lock();
+	err = __tipc_nl_bearer_disable(skb, info);
+	rtnl_unlock();
+
+	return err;
+}
+
 int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
 {
 	int err;
diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
index 42d6eeeb646d..bcc6d5f7014b 100644
--- a/net/tipc/bearer.h
+++ b/net/tipc/bearer.h
@@ -188,6 +188,7 @@ extern struct tipc_media udp_media_info;
 #endif
 
 int tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info);
+int __tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_bearer_dump(struct sk_buff *skb, struct netlink_callback *cb);
 int tipc_nl_bearer_get(struct sk_buff *skb, struct genl_info *info);
Refactoring in __tipc_nl_compat_doit


---
 net/tipc/netlink_compat.c |   29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index e48f0b2c01b9..974169059b9c 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -285,10 +285,6 @@ static int __tipc_nl_compat_doit(struct tipc_nl_compat_cmd_doit *cmd,
 	if (!trans_buf)
 		return -ENOMEM;
 
-	err = (*cmd->transcode)(cmd, trans_buf, msg);
-	if (err)
-		goto trans_out;
-
 	attrbuf = kmalloc((tipc_genl_family.maxattr + 1) *
 			sizeof(struct nlattr *), GFP_KERNEL);
 	if (!attrbuf) {
@@ -296,27 +292,32 @@ static int __tipc_nl_compat_doit(struct tipc_nl_compat_cmd_doit *cmd,
 		goto trans_out;
 	}
 
-	err = nla_parse(attrbuf, tipc_genl_family.maxattr,
-			(const struct nlattr *)trans_buf->data,
-			trans_buf->len, NULL, NULL);
-	if (err)
-		goto parse_out;
-
 	doit_buf = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
 	if (!doit_buf) {
 		err = -ENOMEM;
-		goto parse_out;
+		goto attrbuf_out;
 	}
 
-	doit_buf->sk = msg->dst_sk;
-
 	memset(&info, 0, sizeof(info));
 	info.attrs = attrbuf;
 
+	err = (*cmd->transcode)(cmd, trans_buf, msg);
+	if (err)
+		goto doit_out;
+
+	err = nla_parse(attrbuf, tipc_genl_family.maxattr,
+			(const struct nlattr *)trans_buf->data,
+			trans_buf->len, NULL, NULL);
+	if (err)
+		goto doit_out;
+
+	doit_buf->sk = msg->dst_sk;
+
 	err = (*cmd->doit)(doit_buf, &info);
+doit_out:
 
 	kfree_skb(doit_buf);
-parse_out:
+attrbuf_out:
 	kfree(attrbuf);
 trans_out:
 	kfree_skb(trans_buf);
tipc: fix missing RTNL lock protection during setting link properties

From: Ying Xue <ying.xue@windriver.com>

Currently when user changes link properties, TIPC first checks if
user's command message contains media name or bearer name through
tipc_media_find() or tipc_bearer_find() which is protected by RTNL
lock. But when tipc_nl_compat_link_set() conducts the checking with
the two functions, it doesn't hold RTNL lock at all, as a result,
the following complaints were reported:

audit: type=1400 audit(1514679888.244:9): avc:  denied  { write } for
pid=3194 comm="syzkaller021477" path="socket:[11143]" dev="sockfs"
ino=11143 scontext=unconfined_u:system_r:insmod_t:s0-s0:c0.c1023
tcontext=unconfined_u:system_r:insmod_t:s0-s0:c0.c1023
tclass=netlink_generic_socket permissive=1
=============================
WARNING: suspicious RCU usage
4.15.0-rc5+ #152 Not tainted
-----------------------------
net/tipc/bearer.c:177 suspicious rcu_dereference_protected() usage!

other info that might help us debug this:

rcu_scheduler_active = 2, debug_locks = 1
2 locks held by syzkaller021477/3194:
  #0:  (cb_lock){++++}, at: [<00000000d20133ea>] genl_rcv+0x19/0x40
net/netlink/genetlink.c:634
  #1:  (genl_mutex){+.+.}, at: [<00000000fcc5d1bc>] genl_lock
net/netlink/genetlink.c:33 [inline]
  #1:  (genl_mutex){+.+.}, at: [<00000000fcc5d1bc>] genl_rcv_msg+0x115/0x140
net/netlink/genetlink.c:622

stack backtrace:
CPU: 1 PID: 3194 Comm: syzkaller021477 Not tainted 4.15.0-rc5+ #152
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:17 [inline]
  dump_stack+0x194/0x257 lib/dump_stack.c:53
  lockdep_rcu_suspicious+0x123/0x170 kernel/locking/lockdep.c:4585
  tipc_bearer_find+0x2b4/0x3b0 net/tipc/bearer.c:177
  tipc_nl_compat_link_set+0x329/0x9f0 net/tipc/netlink_compat.c:729
  __tipc_nl_compat_doit net/tipc/netlink_compat.c:288 [inline]
  tipc_nl_compat_doit+0x15b/0x660 net/tipc/netlink_compat.c:335
  tipc_nl_compat_handle net/tipc/netlink_compat.c:1119 [inline]
  tipc_nl_compat_recv+0x112f/0x18f0 net/tipc/netlink_compat.c:1201
  genl_family_rcv_msg+0x7b7/0xfb0 net/netlink/genetlink.c:599
  genl_rcv_msg+0xb2/0x140 net/netlink/genetlink.c:624
  netlink_rcv_skb+0x21e/0x460 net/netlink/af_netlink.c:2408
  genl_rcv+0x28/0x40 net/netlink/genetlink.c:635
  netlink_unicast_kernel net/netlink/af_netlink.c:1275 [inline]
  netlink_unicast+0x4e8/0x6f0 net/netlink/af_netlink.c:1301
  netlink_sendmsg+0xa4a/0xe60 net/netlink/af_netlink.c:1864
  sock_sendmsg_nosec net/socket.c:636 [inline]
  sock_sendmsg+0xca/0x110 net/socket.c:646
  sock_write_iter+0x31a/0x5d0 net/socket.c:915
  call_write_iter include/linux/fs.h:1772 [inline]
  new_sync_write fs/read_write.c:469 [inline]
  __vfs_write+0x684/0x970 fs/read_write.c:482
  vfs_write+0x189/0x510 fs/read_write.c:544
  SYSC_write fs/read_write.c:589 [inline]
  SyS_write+0xef/0x220 fs/read_write.c:581
  do_syscall_32_irqs_on arch/x86/entry/common.c:327 [inline]
  do_fast_syscall_32+0x3ee/0xf9d arch/x86/entry/common.c:389
  entry_SYSENTER_compat+0x54/0x63 arch/x86/entry/entry_64_compat.S:129

In order to correct the mistake, __tipc_nl_compat_doit() has been
protected by RTNL lock, which means the whole operation of setting
bearer/media properties is under RTNL protection.

Signed-off-by: Ying Xue <ying.xue@windriver.com>
Reported-by: syzbot <syzbot+6345fd433db009b29413@syzkaller.appspotmail.com>
---
 net/tipc/netlink_compat.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index 974169059b9c..4492cda45566 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -301,6 +301,7 @@ static int __tipc_nl_compat_doit(struct tipc_nl_compat_cmd_doit *cmd,
 	memset(&info, 0, sizeof(info));
 	info.attrs = attrbuf;
 
+	rtnl_lock();
 	err = (*cmd->transcode)(cmd, trans_buf, msg);
 	if (err)
 		goto doit_out;
@@ -315,6 +316,7 @@ static int __tipc_nl_compat_doit(struct tipc_nl_compat_cmd_doit *cmd,
 
 	err = (*cmd->doit)(doit_buf, &info);
 doit_out:
+	rtnl_unlock();
 
 	kfree_skb(doit_buf);
 attrbuf_out:
@@ -723,13 +725,13 @@ static int tipc_nl_compat_link_set(struct tipc_nl_compat_cmd_doit *cmd,
 
 	media = tipc_media_find(lc->name);
 	if (media) {
-		cmd->doit = &tipc_nl_media_set;
+		cmd->doit = &__tipc_nl_media_set;
 		return tipc_nl_compat_media_set(skb, msg);
 	}
 
 	bearer = tipc_bearer_find(msg->net, lc->name);
 	if (bearer) {
-		cmd->doit = &tipc_nl_bearer_set;
+		cmd->doit = &__tipc_nl_bearer_set;
 		return tipc_nl_compat_bearer_set(skb, msg);
 	}
 
@@ -1090,12 +1092,12 @@ static int tipc_nl_compat_handle(struct tipc_nl_compat_msg *msg)
 		return tipc_nl_compat_dumpit(&dump, msg);
 	case TIPC_CMD_ENABLE_BEARER:
 		msg->req_type = TIPC_TLV_BEARER_CONFIG;
-		doit.doit = tipc_nl_bearer_enable;
+		doit.doit = __tipc_nl_bearer_enable;
 		doit.transcode = tipc_nl_compat_bearer_enable;
 		return tipc_nl_compat_doit(&doit, msg);
 	case TIPC_CMD_DISABLE_BEARER:
 		msg->req_type = TIPC_TLV_BEARER_NAME;
-		doit.doit = tipc_nl_bearer_disable;
+		doit.doit = __tipc_nl_bearer_disable;
 		doit.transcode = tipc_nl_compat_bearer_disable;
 		return tipc_nl_compat_doit(&doit, msg);
 	case TIPC_CMD_SHOW_LINK_STATS:
@@ -1149,12 +1151,12 @@ static int tipc_nl_compat_handle(struct tipc_nl_compat_msg *msg)
 		return tipc_nl_compat_dumpit(&dump, msg);
 	case TIPC_CMD_SET_NODE_ADDR:
 		msg->req_type = TIPC_TLV_NET_ADDR;
-		doit.doit = tipc_nl_net_set;
+		doit.doit = __tipc_nl_net_set;
 		doit.transcode = tipc_nl_compat_net_set;
 		return tipc_nl_compat_doit(&doit, msg);
 	case TIPC_CMD_SET_NETID:
 		msg->req_type = TIPC_TLV_UNSIGNED;
-		doit.doit = tipc_nl_net_set;
+		doit.doit = __tipc_nl_net_set;
 		doit.transcode = tipc_nl_compat_net_set;
 		return tipc_nl_compat_doit(&doit, msg);
 	case TIPC_CMD_GET_NETID:
# This series applies on GIT commit cf19e5e2054f5172c07a152f9e04eb3bae3d86dd
p1
p2
p3
p4
p5
p6
tipc-fix-missing-rtnl-lock
Ying Xue Feb. 14, 2018, 5:40 a.m. UTC | #2 
On 02/13/2018 07:03 PM, Kirill Tkhai wrote:
> The patch is logically OK for me. The only thing I'm confused,
> I had to split it in 7 patches to review, otherwise the patch
> looks difficult to do. There is possible to extract:
> 
> 1)Refactoring in __tipc_nl_compat_doit
> 2)Introduce __tipc_nl_bearer_disable()
> 3)Introduce __tipc_nl_bearer_enable()
> 4)Introduce __tipc_nl_bearer_set()
> 5)Introduce __tipc_nl_media_set()
> 6)Introduce __tipc_nl_net_set()
> 7)tipc: fix missing RTNL lock protection during setting link properties
> 
> After that it becomes very easy to review the change you made in this patch.
> 
> I attached them to message, and you may take the patches if there is one
> more version.

Thank you very much for your suggested patches! I have revised them
based on your idea and sent out v4 version. Please review them.

Thanks,
Ying
diff mbox series

Patch

=============================
WARNING: suspicious RCU usage
4.15.0-rc5+ #152 Not tainted
-----------------------------
net/tipc/bearer.c:177 suspicious rcu_dereference_protected() usage!

other info that might help us debug this:

rcu_scheduler_active = 2, debug_locks = 1
2 locks held by syzkaller021477/3194:
  #0:  (cb_lock){++++}, at: [<00000000d20133ea>] genl_rcv+0x19/0x40
net/netlink/genetlink.c:634
  #1:  (genl_mutex){+.+.}, at: [<00000000fcc5d1bc>] genl_lock
net/netlink/genetlink.c:33 [inline]
  #1:  (genl_mutex){+.+.}, at: [<00000000fcc5d1bc>] genl_rcv_msg+0x115/0x140
net/netlink/genetlink.c:622

stack backtrace:
CPU: 1 PID: 3194 Comm: syzkaller021477 Not tainted 4.15.0-rc5+ #152
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:17 [inline]
  dump_stack+0x194/0x257 lib/dump_stack.c:53
  lockdep_rcu_suspicious+0x123/0x170 kernel/locking/lockdep.c:4585
  tipc_bearer_find+0x2b4/0x3b0 net/tipc/bearer.c:177
  tipc_nl_compat_link_set+0x329/0x9f0 net/tipc/netlink_compat.c:729
  __tipc_nl_compat_doit net/tipc/netlink_compat.c:288 [inline]
  tipc_nl_compat_doit+0x15b/0x660 net/tipc/netlink_compat.c:335
  tipc_nl_compat_handle net/tipc/netlink_compat.c:1119 [inline]
  tipc_nl_compat_recv+0x112f/0x18f0 net/tipc/netlink_compat.c:1201
  genl_family_rcv_msg+0x7b7/0xfb0 net/netlink/genetlink.c:599
  genl_rcv_msg+0xb2/0x140 net/netlink/genetlink.c:624
  netlink_rcv_skb+0x21e/0x460 net/netlink/af_netlink.c:2408
  genl_rcv+0x28/0x40 net/netlink/genetlink.c:635
  netlink_unicast_kernel net/netlink/af_netlink.c:1275 [inline]
  netlink_unicast+0x4e8/0x6f0 net/netlink/af_netlink.c:1301
  netlink_sendmsg+0xa4a/0xe60 net/netlink/af_netlink.c:1864
  sock_sendmsg_nosec net/socket.c:636 [inline]
  sock_sendmsg+0xca/0x110 net/socket.c:646
  sock_write_iter+0x31a/0x5d0 net/socket.c:915
  call_write_iter include/linux/fs.h:1772 [inline]
  new_sync_write fs/read_write.c:469 [inline]
  __vfs_write+0x684/0x970 fs/read_write.c:482
  vfs_write+0x189/0x510 fs/read_write.c:544
  SYSC_write fs/read_write.c:589 [inline]
  SyS_write+0xef/0x220 fs/read_write.c:581
  do_syscall_32_irqs_on arch/x86/entry/common.c:327 [inline]
  do_fast_syscall_32+0x3ee/0xf9d arch/x86/entry/common.c:389
  entry_SYSENTER_compat+0x54/0x63 arch/x86/entry/entry_64_compat.S:129

In order to correct the mistake, __tipc_nl_compat_doit() has been
protected by RTNL lock, which means the whole operation of setting
bearer/media properties is under RTNL protection.

Signed-off-by: Ying Xue <ying.xue@windriver.com>
Reported-by: syzbot <syzbot+6345fd433db009b29413@syzkaller.appspotmail.com>
---
v3: 
    - Optimized return method of __tipc_nl_bearer_enable() regarding
      the comments from David M and Kirill Tkhai
    - Moved the allocations of memory in __tipc_nl_compat_doit() out
      of RTNL lock to minimize the time of holding RTNL lock according
      to the suggestion of Kirill Tkhai.
v2:
    - The whole operation of setting bearer/media properties has been
      protected under RTNL, as per feedback from David M.

 net/tipc/bearer.c         | 82 +++++++++++++++++++++++++++++------------------
 net/tipc/bearer.h         |  4 +++
 net/tipc/net.c            | 15 +++++++--
 net/tipc/net.h            |  1 +
 net/tipc/netlink_compat.c | 43 +++++++++++++------------
 5 files changed, 91 insertions(+), 54 deletions(-)

diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index c800147..3e3dce3 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -813,7 +813,7 @@  int tipc_nl_bearer_get(struct sk_buff *skb, struct genl_info *info)
 	return err;
 }
 
-int tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info)
+int __tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info)
 {
 	int err;
 	char *name;
@@ -835,20 +835,27 @@  int tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info)
 
 	name = nla_data(attrs[TIPC_NLA_BEARER_NAME]);
 
-	rtnl_lock();
 	bearer = tipc_bearer_find(net, name);
-	if (!bearer) {
-		rtnl_unlock();
+	if (!bearer)
 		return -EINVAL;
-	}
 
 	bearer_disable(net, bearer);
-	rtnl_unlock();
 
 	return 0;
 }
 
-int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
+int tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info)
+{
+	int err;
+
+	rtnl_lock();
+	err = __tipc_nl_bearer_disable(skb, info);
+	rtnl_unlock();
+
+	return err;
+}
+
+int __tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
 {
 	int err;
 	char *bearer;
@@ -890,15 +897,18 @@  int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
 			prio = nla_get_u32(props[TIPC_NLA_PROP_PRIO]);
 	}
 
+	return tipc_enable_bearer(net, bearer, domain, prio, attrs);
+}
+
+int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
+{
+	int err;
+
 	rtnl_lock();
-	err = tipc_enable_bearer(net, bearer, domain, prio, attrs);
-	if (err) {
-		rtnl_unlock();
-		return err;
-	}
+	err = __tipc_nl_bearer_enable(skb, info);
 	rtnl_unlock();
 
-	return 0;
+	return err;
 }
 
 int tipc_nl_bearer_add(struct sk_buff *skb, struct genl_info *info)
@@ -944,7 +954,7 @@  int tipc_nl_bearer_add(struct sk_buff *skb, struct genl_info *info)
 	return 0;
 }
 
-int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
+int __tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
 {
 	int err;
 	char *name;
@@ -965,22 +975,17 @@  int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
 		return -EINVAL;
 	name = nla_data(attrs[TIPC_NLA_BEARER_NAME]);
 
-	rtnl_lock();
 	b = tipc_bearer_find(net, name);
-	if (!b) {
-		rtnl_unlock();
+	if (!b)
 		return -EINVAL;
-	}
 
 	if (attrs[TIPC_NLA_BEARER_PROP]) {
 		struct nlattr *props[TIPC_NLA_PROP_MAX + 1];
 
 		err = tipc_nl_parse_link_prop(attrs[TIPC_NLA_BEARER_PROP],
 					      props);
-		if (err) {
-			rtnl_unlock();
+		if (err)
 			return err;
-		}
 
 		if (props[TIPC_NLA_PROP_TOL])
 			b->tolerance = nla_get_u32(props[TIPC_NLA_PROP_TOL]);
@@ -989,11 +994,21 @@  int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
 		if (props[TIPC_NLA_PROP_WIN])
 			b->window = nla_get_u32(props[TIPC_NLA_PROP_WIN]);
 	}
-	rtnl_unlock();
 
 	return 0;
 }
 
+int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info)
+{
+	int err;
+
+	rtnl_lock();
+	err = __tipc_nl_bearer_set(skb, info);
+	rtnl_unlock();
+
+	return err;
+}
+
 static int __tipc_nl_add_media(struct tipc_nl_msg *msg,
 			       struct tipc_media *media, int nlflags)
 {
@@ -1115,7 +1130,7 @@  int tipc_nl_media_get(struct sk_buff *skb, struct genl_info *info)
 	return err;
 }
 
-int tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info)
+int __tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info)
 {
 	int err;
 	char *name;
@@ -1133,22 +1148,17 @@  int tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info)
 		return -EINVAL;
 	name = nla_data(attrs[TIPC_NLA_MEDIA_NAME]);
 
-	rtnl_lock();
 	m = tipc_media_find(name);
-	if (!m) {
-		rtnl_unlock();
+	if (!m)
 		return -EINVAL;
-	}
 
 	if (attrs[TIPC_NLA_MEDIA_PROP]) {
 		struct nlattr *props[TIPC_NLA_PROP_MAX + 1];
 
 		err = tipc_nl_parse_link_prop(attrs[TIPC_NLA_MEDIA_PROP],
 					      props);
-		if (err) {
-			rtnl_unlock();
+		if (err)
 			return err;
-		}
 
 		if (props[TIPC_NLA_PROP_TOL])
 			m->tolerance = nla_get_u32(props[TIPC_NLA_PROP_TOL]);
@@ -1157,7 +1167,17 @@  int tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info)
 		if (props[TIPC_NLA_PROP_WIN])
 			m->window = nla_get_u32(props[TIPC_NLA_PROP_WIN]);
 	}
-	rtnl_unlock();
 
 	return 0;
 }
+
+int tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info)
+{
+	int err;
+
+	rtnl_lock();
+	err = __tipc_nl_media_set(skb, info);
+	rtnl_unlock();
+
+	return err;
+}
diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
index 42d6eee..a53613d 100644
--- a/net/tipc/bearer.h
+++ b/net/tipc/bearer.h
@@ -188,15 +188,19 @@  extern struct tipc_media udp_media_info;
 #endif
 
 int tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info);
+int __tipc_nl_bearer_disable(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info);
+int __tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_bearer_dump(struct sk_buff *skb, struct netlink_callback *cb);
 int tipc_nl_bearer_get(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info);
+int __tipc_nl_bearer_set(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_bearer_add(struct sk_buff *skb, struct genl_info *info);
 
 int tipc_nl_media_dump(struct sk_buff *skb, struct netlink_callback *cb);
 int tipc_nl_media_get(struct sk_buff *skb, struct genl_info *info);
 int tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info);
+int __tipc_nl_media_set(struct sk_buff *skb, struct genl_info *info);
 
 int tipc_media_set_priority(const char *name, u32 new_value);
 int tipc_media_set_window(const char *name, u32 new_value);
diff --git a/net/tipc/net.c b/net/tipc/net.c
index 719c592..1a2fde0 100644
--- a/net/tipc/net.c
+++ b/net/tipc/net.c
@@ -200,7 +200,7 @@  int tipc_nl_net_dump(struct sk_buff *skb, struct netlink_callback *cb)
 	return skb->len;
 }
 
-int tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info)
+int __tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info)
 {
 	struct net *net = sock_net(skb->sk);
 	struct tipc_net *tn = net_generic(net, tipc_net_id);
@@ -241,10 +241,19 @@  int tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info)
 		if (!tipc_addr_node_valid(addr))
 			return -EINVAL;
 
-		rtnl_lock();
 		tipc_net_start(net, addr);
-		rtnl_unlock();
 	}
 
 	return 0;
 }
+
+int tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info)
+{
+	int err;
+
+	rtnl_lock();
+	err = __tipc_nl_net_set(skb, info);
+	rtnl_unlock();
+
+	return err;
+}
diff --git a/net/tipc/net.h b/net/tipc/net.h
index c7c2549..c0306aa 100644
--- a/net/tipc/net.h
+++ b/net/tipc/net.h
@@ -47,5 +47,6 @@  void tipc_net_stop(struct net *net);
 
 int tipc_nl_net_dump(struct sk_buff *skb, struct netlink_callback *cb);
 int tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info);
+int __tipc_nl_net_set(struct sk_buff *skb, struct genl_info *info);
 
 #endif
diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index e48f0b2..4492cda 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -285,10 +285,6 @@  static int __tipc_nl_compat_doit(struct tipc_nl_compat_cmd_doit *cmd,
 	if (!trans_buf)
 		return -ENOMEM;
 
-	err = (*cmd->transcode)(cmd, trans_buf, msg);
-	if (err)
-		goto trans_out;
-
 	attrbuf = kmalloc((tipc_genl_family.maxattr + 1) *
 			sizeof(struct nlattr *), GFP_KERNEL);
 	if (!attrbuf) {
@@ -296,27 +292,34 @@  static int __tipc_nl_compat_doit(struct tipc_nl_compat_cmd_doit *cmd,
 		goto trans_out;
 	}
 
-	err = nla_parse(attrbuf, tipc_genl_family.maxattr,
-			(const struct nlattr *)trans_buf->data,
-			trans_buf->len, NULL, NULL);
-	if (err)
-		goto parse_out;
-
 	doit_buf = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
 	if (!doit_buf) {
 		err = -ENOMEM;
-		goto parse_out;
+		goto attrbuf_out;
 	}
 
-	doit_buf->sk = msg->dst_sk;
-
 	memset(&info, 0, sizeof(info));
 	info.attrs = attrbuf;
 
+	rtnl_lock();
+	err = (*cmd->transcode)(cmd, trans_buf, msg);
+	if (err)
+		goto doit_out;
+
+	err = nla_parse(attrbuf, tipc_genl_family.maxattr,
+			(const struct nlattr *)trans_buf->data,
+			trans_buf->len, NULL, NULL);
+	if (err)
+		goto doit_out;
+
+	doit_buf->sk = msg->dst_sk;
+
 	err = (*cmd->doit)(doit_buf, &info);
+doit_out:
+	rtnl_unlock();
 
 	kfree_skb(doit_buf);
-parse_out:
+attrbuf_out:
 	kfree(attrbuf);
 trans_out:
 	kfree_skb(trans_buf);
@@ -722,13 +725,13 @@  static int tipc_nl_compat_link_set(struct tipc_nl_compat_cmd_doit *cmd,
 
 	media = tipc_media_find(lc->name);
 	if (media) {
-		cmd->doit = &tipc_nl_media_set;
+		cmd->doit = &__tipc_nl_media_set;
 		return tipc_nl_compat_media_set(skb, msg);
 	}
 
 	bearer = tipc_bearer_find(msg->net, lc->name);
 	if (bearer) {
-		cmd->doit = &tipc_nl_bearer_set;
+		cmd->doit = &__tipc_nl_bearer_set;
 		return tipc_nl_compat_bearer_set(skb, msg);
 	}
 
@@ -1089,12 +1092,12 @@  static int tipc_nl_compat_handle(struct tipc_nl_compat_msg *msg)
 		return tipc_nl_compat_dumpit(&dump, msg);
 	case TIPC_CMD_ENABLE_BEARER:
 		msg->req_type = TIPC_TLV_BEARER_CONFIG;
-		doit.doit = tipc_nl_bearer_enable;
+		doit.doit = __tipc_nl_bearer_enable;
 		doit.transcode = tipc_nl_compat_bearer_enable;
 		return tipc_nl_compat_doit(&doit, msg);
 	case TIPC_CMD_DISABLE_BEARER:
 		msg->req_type = TIPC_TLV_BEARER_NAME;
-		doit.doit = tipc_nl_bearer_disable;
+		doit.doit = __tipc_nl_bearer_disable;
 		doit.transcode = tipc_nl_compat_bearer_disable;
 		return tipc_nl_compat_doit(&doit, msg);
 	case TIPC_CMD_SHOW_LINK_STATS:
@@ -1148,12 +1151,12 @@  static int tipc_nl_compat_handle(struct tipc_nl_compat_msg *msg)
 		return tipc_nl_compat_dumpit(&dump, msg);
 	case TIPC_CMD_SET_NODE_ADDR:
 		msg->req_type = TIPC_TLV_NET_ADDR;
-		doit.doit = tipc_nl_net_set;
+		doit.doit = __tipc_nl_net_set;
 		doit.transcode = tipc_nl_compat_net_set;
 		return tipc_nl_compat_doit(&doit, msg);
 	case TIPC_CMD_SET_NETID:
 		msg->req_type = TIPC_TLV_UNSIGNED;
-		doit.doit = tipc_nl_net_set;
+		doit.doit = __tipc_nl_net_set;
 		doit.transcode = tipc_nl_compat_net_set;
 		return tipc_nl_compat_doit(&doit, msg);
 	case TIPC_CMD_GET_NETID: