[PATCH net v2] tipc: fix missing RTNL lock protection during setting link properties
From: Ying Xue <hidden>
Date: 2018-02-12 09:00:02
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 <redacted>
Reported-by: syzbot <redacted>
---
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; } +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(); 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:
--
2.7.4