| 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
| ||
|
Message-ID: <147fc60b-df6b-c8ab-8a10-e57e310f65bf@virtuozzo.com>
Date: Mon, 12 Feb 2018 13:10:34 +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 v2] tipc: fix missing RTNL lock protection during
setting link properties
Hi, Ying,
On 12.02.2018 11:56, 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>
> ---
> v2: The whole operation of setting bearer/media properties has been
> protected under RTNL, as per feedback from David M.
>
> net/tipc/bearer.c | 84 ++++++++++++++++++++++++++++++-----------------
> net/tipc/bearer.h | 4 +++
> net/tipc/net.c | 15 +++++++--
> net/tipc/net.h | 1 +
> net/tipc/netlink_compat.c | 14 ++++----
> 5 files changed, 79 insertions(+), 39 deletions(-)
>
> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> index c800147..e12d7eb 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,17 +897,24 @@ int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
> prio = nla_get_u32(props[TIPC_NLA_PROP_PRIO]);
> }
>
> - rtnl_lock();
> err = tipc_enable_bearer(net, bearer, domain, prio, attrs);
> - if (err) {
> - rtnl_unlock();
> + if (err)
> return err;
> - }
> - rtnl_unlock();
>
> return 0;
This err branch looks excess. It was before your patch, but in case of you change this place,
can't we stop having it? it looks like we can simply do the below here:
err = tipc_enable_bearer(net, bearer, domain, prio, attrs);
return err;
> }
>
> +int tipc_nl_bearer_enable(struct sk_buff *skb, struct genl_info *info)
> +{
> + int err;
> +
> + rtnl_lock();
> + err = __tipc_nl_bearer_enable(skb, info);
> + rtnl_unlock();
> +
> + return err;
> +}
> +
> int tipc_nl_bearer_add(struct sk_buff *skb, struct genl_info *info)
> {
> int err;
> @@ -944,7 +958,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 +979,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 +998,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 +1134,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 +1152,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 +1171,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..750e37c 100644
> --- a/net/tipc/netlink_compat.c
> +++ b/net/tipc/netlink_compat.c
> @@ -332,7 +332,9 @@ static int tipc_nl_compat_doit(struct tipc_nl_compat_cmd_doit *cmd,
> if (msg->req_type && !TLV_CHECK_TYPE(msg->req, msg->req_type))
> return -EINVAL;
>
> + rtnl_lock();
> err = __tipc_nl_compat_doit(cmd, msg);
> + rtnl_unlock();
rtnl_lock() is frequently accessed mutex used everywhere in network subsystem,
and it would be good to minimize the time it's held. Function __tipc_nl_compat_doit()
allocates memory and these allocations may envoke memory reclain, which may
take much time. The lock will be held all the time, and it's bad for other
rtnl users.
Since __tipc_nl_compat_doit() allocates constant amount of memory, we can do
all the allocations before taking rtnl_lock, and this may reduce the time
we are owning the lock in significant way. Can't we do this in this patchset?
(I don't think we can do this in the same patch, but one more pre-patch may
be introduced). The same for kfree*() and memset(), they also do not need
rtnl_lock().
> if (err)
> return err;
>
> @@ -722,13 +724,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 +1091,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 +1150,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:
>
Thanks,
Kirill
Powered by blists - more mailing lists