| Message ID | 1546860592-6039-1-git-send-email-ying.xue@windriver.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] tipc: fix uninit-value in tipc_nl_compat_link_set | expand |
From: Ying Xue <ying.xue@windriver.com> Date: Mon, 7 Jan 2019 19:29:52 +0800 > This is because lc->name string is not validated before it's used. It looks like we have several situations like this, not just this one. For example, tipc_nl_compat_bearer_{enable,disable}() with b->name. Next, tipc_nl_compat_media_set() and tipc_nl_compat_bearer_set(). On input, tipc_nl_compat_link_stat_dump() blindly does a strcmp() on one of these strings. In fact, this entire file is full of errors of this sort. Can you please address all of them, perhaps using a helper of some kind to consolidate the logic? Thank you.
On Mon, Jan 7, 2019 at 2:38 PM David Miller <davem@davemloft.net> wrote: > > From: Ying Xue <ying.xue@windriver.com> > Date: Mon, 7 Jan 2019 19:29:52 +0800 > > > This is because lc->name string is not validated before it's used. > > It looks like we have several situations like this, not just this one. > > For example, tipc_nl_compat_bearer_{enable,disable}() with b->name. > > Next, tipc_nl_compat_media_set() and tipc_nl_compat_bearer_set(). > > On input, tipc_nl_compat_link_stat_dump() blindly does a strcmp() > on one of these strings. > > In fact, this entire file is full of errors of this sort. > > Can you please address all of them, perhaps using a helper of > some kind to consolidate the logic? > > Thank you. There 9 uninits in tipc currently open on syzbot. If you extend the scope of this change, please update Reported-by tags accordingly. Thanks KMSAN: uninit-value in tipc_nl_compat_doit https://syzkaller.appspot.com/bug?id=d64b3c48a3e81dca017e61a50d15880bc677cbb5 KMSAN: uninit-value in tipc_nl_compat_dumpit https://syzkaller.appspot.com/bug?id=330d750e403fbaf78e3b631b389443c9153e3804 KMSAN: uninit-value in tipc_nl_compat_bearer_enable https://syzkaller.appspot.com/bug?id=a9dce514eeddaa78ca8e04676c19097939db1fc5 KMSAN: uninit-value in tipc_nl_compat_name_table_dump https://syzkaller.appspot.com/bug?id=c40c98888fb6ebdb3f66d99dfa2c1b8df9601a34 KMSAN: uninit-value in tipc_nl_compat_link_set (2) https://syzkaller.appspot.com/bug?id=ab4ee03213b801410f0194d6b5435e8e3c95af94 KMSAN: uninit-value in __tipc_nl_bearer_enable https://syzkaller.appspot.com/bug?id=bf95da1c68b889380395af19f1953e91b772d3ea KMSAN: uninit-value in tipc_nl_compat_link_reset_stats https://syzkaller.appspot.com/bug?id=a675bbf645ff6a5810e2af2f35752d93f80dd95b KMSAN: uninit-value in tipc_subscrb_rcv_cb https://syzkaller.appspot.com/bug?id=ec45249073777e4fc8fa9d8f1732f594bc2f4023 KMSAN: uninit-value in tipc_conn_rcv_sub https://syzkaller.appspot.com/bug?id=0df95bc3a80b97dfd33dc7f47ebfd7802d03dfc3
On 1/7/19 9:38 PM, David Miller wrote: > From: Ying Xue <ying.xue@windriver.com> > Date: Mon, 7 Jan 2019 19:29:52 +0800 > >> This is because lc->name string is not validated before it's used. > > It looks like we have several situations like this, not just this one. > > For example, tipc_nl_compat_bearer_{enable,disable}() with b->name. > > Next, tipc_nl_compat_media_set() and tipc_nl_compat_bearer_set(). > > On input, tipc_nl_compat_link_stat_dump() blindly does a strcmp() > on one of these strings. > > In fact, this entire file is full of errors of this sort. > > Can you please address all of them, perhaps using a helper of > some kind to consolidate the logic? > Thank you for your good suggestions. I will solve them as soon as possible. Regards, Ying > Thank you. >
diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c index 21f6ccc..bbf3f5a 100644 --- a/net/tipc/netlink_compat.c +++ b/net/tipc/netlink_compat.c @@ -705,6 +705,9 @@ static int __tipc_nl_compat_link_set(struct sk_buff *skb, if (!link) return -EMSGSIZE; + if (!memchr(lc->name, '\0', TIPC_MAX_LINK_NAME)) + return -EINVAL; + if (nla_put_string(skb, TIPC_NLA_LINK_NAME, lc->name)) return -EMSGSIZE;
syzbot reports following splat: BUG: KMSAN: uninit-value in strlen+0x3b/0xa0 lib/string.c:486 CPU: 1 PID: 9306 Comm: syz-executor172 Not tainted 4.20.0-rc7+ #2 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x173/0x1d0 lib/dump_stack.c:113 kmsan_report+0x12e/0x2a0 mm/kmsan/kmsan.c:613 __msan_warning+0x82/0xf0 mm/kmsan/kmsan_instr.c:313 strlen+0x3b/0xa0 lib/string.c:486 nla_put_string include/net/netlink.h:1154 [inline] __tipc_nl_compat_link_set net/tipc/netlink_compat.c:708 [inline] tipc_nl_compat_link_set+0x929/0x1220 net/tipc/netlink_compat.c:744 __tipc_nl_compat_doit net/tipc/netlink_compat.c:311 [inline] tipc_nl_compat_doit+0x3aa/0xaf0 net/tipc/netlink_compat.c:344 tipc_nl_compat_handle net/tipc/netlink_compat.c:1107 [inline] tipc_nl_compat_recv+0x14d7/0x2760 net/tipc/netlink_compat.c:1210 genl_family_rcv_msg net/netlink/genetlink.c:601 [inline] genl_rcv_msg+0x185f/0x1a60 net/netlink/genetlink.c:626 netlink_rcv_skb+0x444/0x640 net/netlink/af_netlink.c:2477 genl_rcv+0x63/0x80 net/netlink/genetlink.c:637 netlink_unicast_kernel net/netlink/af_netlink.c:1310 [inline] netlink_unicast+0xf40/0x1020 net/netlink/af_netlink.c:1336 netlink_sendmsg+0x127f/0x1300 net/netlink/af_netlink.c:1917 sock_sendmsg_nosec net/socket.c:621 [inline] sock_sendmsg net/socket.c:631 [inline] ___sys_sendmsg+0xdb9/0x11b0 net/socket.c:2116 __sys_sendmsg net/socket.c:2154 [inline] __do_sys_sendmsg net/socket.c:2163 [inline] __se_sys_sendmsg+0x305/0x460 net/socket.c:2161 __x64_sys_sendmsg+0x4a/0x70 net/socket.c:2161 do_syscall_64+0xbc/0xf0 arch/x86/entry/common.c:291 entry_SYSCALL_64_after_hwframe+0x63/0xe7 The uninitialised access happened in nla_put_string(skb, TIPC_NLA_LINK_NAME, lc->name) This is because lc->name string is not validated before it's used. Reported-by: syzbot+d78b8a29241a195aefb8@syzkaller.appspotmail.com Signed-off-by: Ying Xue <ying.xue@windriver.com> --- net/tipc/netlink_compat.c | 3 +++ 1 file changed, 3 insertions(+)