lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 14 Feb 2018 13:16:06 +0300
From:   Kirill Tkhai <ktkhai@...tuozzo.com>
To:     Ying Xue <ying.xue@...driver.com>, davem@...emloft.net,
        jon.maloy@...csson.com
Cc:     netdev@...r.kernel.org, syzkaller-bugs@...glegroups.com,
        tipc-discussion@...ts.sourceforge.net
Subject: Re: [PATCH net v4 7/7] tipc: Fix missing RTNL lock protection during
 setting link properties

On 14.02.2018 08:38, 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@...driver.com>
> Reported-by: syzbot <syzbot+6345fd433db009b29413@...kaller.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 9741690..4492cda 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:

It looks like this matches the idea, David suggested in reply to v1.
Also, I don't see more .doit acquiring rtnl_lock(), all are converted.

For the whole series:

Reviewed-by: Kirill Tkhai <ktkhai@...tuozzo.com>

Thanks.

Powered by blists - more mailing lists