[PATCH net] tipc: fix missing rtnl lock protection during setting link properties

[PATCH net] tipc: fix missing rtnl lock protection during setting link properties

[Ying Xue]

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

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

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index e48f0b2..0fb12a4 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -720,17 +720,21 @@ static int tipc_nl_compat_link_set(struct tipc_nl_compat_cmd_doit *cmd,
 
 	lc = (struct tipc_link_config *)TLV_DATA(msg->req);
 
+	rtnl_lock();
 	media = tipc_media_find(lc->name);
 	if (media) {
 		cmd->doit = &tipc_nl_media_set;
+		rtnl_unlock();
 		return tipc_nl_compat_media_set(skb, msg);
 	}
 
 	bearer = tipc_bearer_find(msg->net, lc->name);
 	if (bearer) {
 		cmd->doit = &tipc_nl_bearer_set;
+		rtnl_unlock();
 		return tipc_nl_compat_bearer_set(skb, msg);
 	}
+	rtnl_unlock();
 
 	return __tipc_nl_compat_link_set(skb, msg);
 }
-- 
2.7.4


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH net] tipc: fix missing rtnl lock protection during setting link properties

[David Miller]

From: Ying Xue <ying.xue@windriver.com>
Date: Mon, 1 Jan 2018 18:24:01 +0800

> diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
> index e48f0b2..0fb12a4 100644
> --- a/net/tipc/netlink_compat.c
> +++ b/net/tipc/netlink_compat.c
> @@ -720,17 +720,21 @@ static int tipc_nl_compat_link_set(struct tipc_nl_compat_cmd_doit *cmd,
>  
>  	lc = (struct tipc_link_config *)TLV_DATA(msg->req);
>  
> +	rtnl_lock();
>  	media = tipc_media_find(lc->name);
>  	if (media) {
>  		cmd->doit = &tipc_nl_media_set;
> +		rtnl_unlock();
>  		return tipc_nl_compat_media_set(skb, msg);
>  	}
>  
>  	bearer = tipc_bearer_find(msg->net, lc->name);
>  	if (bearer) {
>  		cmd->doit = &tipc_nl_bearer_set;
> +		rtnl_unlock();
>  		return tipc_nl_compat_bearer_set(skb, msg);
>  	}
> +	rtnl_unlock();
>  
>  	return __tipc_nl_compat_link_set(skb, msg);
>  }

As soon as you drop the RTNL lock, the media or bearer entry can be
removed from the tables.

This invalidates what you do next, whether it's
tipc_nl_compat_media_set(), tipc_nl_compat_bearer_set(), etc.

Therefore, you have to lock down the tipc configuration state around
this entire operation, from media/bearer probe to the building of the
netlink message(s).

Either this entire code path must execute with the bearer/media entry
present, or without.  If you drop the RTNL mutex in the middle, this
invariant is not held.

Re: [PATCH net] tipc: fix missing rtnl lock protection during setting link properties

[Ying Xue]

On 01/03/2018 11:48 PM, David Miller wrote:
> As soon as you drop the RTNL lock, the media or bearer entry can be
> removed from the tables.
> 

Thanks for the review. Yes, you are right. But even if we temporarily
release RTNL lock, it's still safe for us because when we set
media/bearer properties in __tipc_nl_compat_doit(), tipc_nl_media_set()
and tipc_nl_bearer_set() will probe media or bearer again within RTNL
lock protection.

> This invalidates what you do next, whether it's
> tipc_nl_compat_media_set(), tipc_nl_compat_bearer_set(), etc.

In fact tipc_nl_compat_media_set() and tipc_nl_compat_bearer_set() don't
really change media or bearer's properties, instead they only format the
contents pointed by their "msg" parameter.

> 
> Therefore, you have to lock down the tipc configuration state around
> this entire operation, from media/bearer probe to the building of the
> netlink message(s).
> 

Sorry, we cannot hold RTNL lock in the entire operation path because
TIPC now supports two different sets of netlink APIs:

One set of API's execution path:

genl_family_rcv_msg()
  tipc_nl_media_set()
    rtnl_lock()
    tipc_media_find()
    //set media properties

genl_family_rcv_msg()
  tipc_nl_bearer_set()
    rtnl_lock()
    tipc_bearer_find()
    //set bearer properties

Another set of API's execution path:

genl_family_rcv_msg()
  tipc_nl_compat_recv()
    tipc_nl_compat_handle net()
      __tipc_nl_compat_doit net()
        tipc_nl_compat_link_set()
        tipc_nl_media_set()

genl_family_rcv_msg()
  tipc_nl_compat_recv()
    tipc_nl_compat_handle net()
      __tipc_nl_compat_doit net()
        tipc_nl_compat_link_set()
        tipc_nl_bearer_set()

As we see in above call chains, tipc_nl_media_set() and
tipc_nl_bearer_set() are shared by the two sets of netlink APIs. If we
hold RTNL lock from tipc_nl_compat_recv(), it means we cannot directly
call tipc_nl_media_set() or tipc_nl_bearer_set() in
__tipc_nl_compat_doit net().

> Either this entire code path must execute with the bearer/media entry
> present, or without.  If you drop the RTNL mutex in the middle, this
> invariant is not held.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH net] tipc: fix missing rtnl lock protection during setting link properties

[David Miller]

From: Ying Xue <ying.xue@windriver.com>
Date: Thu, 4 Jan 2018 15:30:52 +0800

> On 01/03/2018 11:48 PM, David Miller wrote:
>> As soon as you drop the RTNL lock, the media or bearer entry can be
>> removed from the tables.
>> 
> 
> Thanks for the review. Yes, you are right. But even if we temporarily
> release RTNL lock, it's still safe for us because when we set
> media/bearer properties in __tipc_nl_compat_doit(), tipc_nl_media_set()
> and tipc_nl_bearer_set() will probe media or bearer again within RTNL
> lock protection.
> 
>> This invalidates what you do next, whether it's
>> tipc_nl_compat_media_set(), tipc_nl_compat_bearer_set(), etc.
> 
> In fact tipc_nl_compat_media_set() and tipc_nl_compat_bearer_set() don't
> really change media or bearer's properties, instead they only format the
> contents pointed by their "msg" parameter.

However, those values are only valid to place into the netlink message if
in fact we successfully found a media or bearer entry.

You have to hold RTNL across this whole construct, from the discovery of
the media or bearer entry all the way to the completion of filling in
the netlink message, one way or another.