[net-next] tipc: fix uninit-value in tipc_nl_node_reset_link_stats

[net-next] tipc: fix uninit-value in tipc_nl_node_reset_link_stats

[Hoang Le]

syzbot found the following issue on:
==================================================================
BUG: KMSAN: uninit-value in strlen lib/string.c:495 [inline]
BUG: KMSAN: uninit-value in strstr+0xb4/0x2e0 lib/string.c:840
 strlen lib/string.c:495 [inline]
 strstr+0xb4/0x2e0 lib/string.c:840
 tipc_nl_node_reset_link_stats+0x41e/0xba0 net/tipc/node.c:2582
 genl_family_rcv_msg_doit net/netlink/genetlink.c:731 [inline]
 genl_family_rcv_msg net/netlink/genetlink.c:775 [inline]
 genl_rcv_msg+0x103f/0x1260 net/netlink/genetlink.c:792
 netlink_rcv_skb+0x3a5/0x6c0 net/netlink/af_netlink.c:2501
 genl_rcv+0x3c/0x50 net/netlink/genetlink.c:803
 netlink_unicast_kernel net/netlink/af_netlink.c:1319 [inline]
 netlink_unicast+0xf3b/0x1270 net/netlink/af_netlink.c:1345
 netlink_sendmsg+0x1288/0x1440 net/netlink/af_netlink.c:1921
 sock_sendmsg_nosec net/socket.c:714 [inline]
 sock_sendmsg net/socket.c:734 [inline]
 ____sys_sendmsg+0xabc/0xe90 net/socket.c:2492
 ___sys_sendmsg+0x2a5/0x350 net/socket.c:2546
 __sys_sendmsg net/socket.c:2575 [inline]
 __do_sys_sendmsg net/socket.c:2584 [inline]
 __se_sys_sendmsg net/socket.c:2582 [inline]
 __x64_sys_sendmsg+0x367/0x540 net/socket.c:2582
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x46/0xb0
==================================================================

This is because link name string is not validated before it's used
in calling strstr() and strlen().

Reported-by: syzbot+a73d24a22eeeebe5f244@syzkaller.appspotmail.com
Acked-by: Jon Maloy <jmaloy@redhat.com>
Signed-off-by: Hoang Le <hoang.h.le@dektech.com.au>
---
 net/tipc/node.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/tipc/node.c b/net/tipc/node.c
index b48d97cbbe29..23419a599471 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -2561,6 +2561,7 @@ int tipc_nl_node_reset_link_stats(struct sk_buff *skb, struct genl_info *info)
 	struct net *net = sock_net(skb->sk);
 	struct tipc_net *tn = tipc_net(net);
 	struct tipc_link_entry *le;
+	int len;
 
 	if (!info->attrs[TIPC_NLA_LINK])
 		return -EINVAL;
@@ -2574,7 +2575,14 @@ int tipc_nl_node_reset_link_stats(struct sk_buff *skb, struct genl_info *info)
 	if (!attrs[TIPC_NLA_LINK_NAME])
 		return -EINVAL;
 
+	len = nla_len(attrs[TIPC_NLA_LINK_NAME]);
+	if (len <= 0)
+		return -EINVAL;
+
 	link_name = nla_data(attrs[TIPC_NLA_LINK_NAME]);
+	len = min_t(int, len, TIPC_MAX_LINK_NAME);
+	if (!memchr(link_name, '\0', len))
+		return -EINVAL;
 
 	err = -EINVAL;
 	if (!strcmp(link_name, tipc_bclink_name)) {
-- 
2.30.2

Re: [net-next] tipc: fix uninit-value in tipc_nl_node_reset_link_stats

[Jakub Kicinski]

On Tue,  5 Jul 2022 07:50:57 +0700 Hoang Le wrote:
> Reported-by: syzbot+a73d24a22eeeebe5f244@syzkaller.appspotmail.com
> Acked-by: Jon Maloy <jmaloy@redhat.com>
> Signed-off-by: Hoang Le <hoang.h.le@dektech.com.au>

Can we get a Fixes tag please?

> diff --git a/net/tipc/node.c b/net/tipc/node.c
> index b48d97cbbe29..23419a599471 100644
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -2561,6 +2561,7 @@ int tipc_nl_node_reset_link_stats(struct sk_buff *skb, struct genl_info *info)
>  	struct net *net = sock_net(skb->sk);
>  	struct tipc_net *tn = tipc_net(net);
>  	struct tipc_link_entry *le;
> +	int len;
>  
>  	if (!info->attrs[TIPC_NLA_LINK])
>  		return -EINVAL;
> @@ -2574,7 +2575,14 @@ int tipc_nl_node_reset_link_stats(struct sk_buff *skb, struct genl_info *info)
>  	if (!attrs[TIPC_NLA_LINK_NAME])
>  		return -EINVAL;
>  
> +	len = nla_len(attrs[TIPC_NLA_LINK_NAME]);
> +	if (len <= 0)
> +		return -EINVAL;
> +
>  	link_name = nla_data(attrs[TIPC_NLA_LINK_NAME]);
> +	len = min_t(int, len, TIPC_MAX_LINK_NAME);
> +	if (!memchr(link_name, '\0', len))
> +		return -EINVAL;

Should we just change the netlink policy for this attribute to
NLA_NUL_STRING, then?

RE: [net-next] tipc: fix uninit-value in tipc_nl_node_reset_link_stats

[Hoang Huu Le]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, July 6, 2022 8:53 AM
> To: Hoang Huu Le <hoang.h.le@dektech.com.au>
> Cc: jmaloy@redhat.com; maloy@donjonn.com; ying.xue@windriver.com; Tung Quang Nguyen <tung.q.nguyen@dektech.com.au>;
> pabeni@redhat.com; edumazet@google.com; tipc-discussion@lists.sourceforge.net; netdev@vger.kernel.org;
> davem@davemloft.net; syzbot+a73d24a22eeeebe5f244@syzkaller.appspotmail.com
> Subject: Re: [net-next] tipc: fix uninit-value in tipc_nl_node_reset_link_stats
> 
> On Tue,  5 Jul 2022 07:50:57 +0700 Hoang Le wrote:
> > Reported-by: syzbot+a73d24a22eeeebe5f244@syzkaller.appspotmail.com
> > Acked-by: Jon Maloy <jmaloy@redhat.com>
> > Signed-off-by: Hoang Le <hoang.h.le@dektech.com.au>
> 
> Can we get a Fixes tag please?
> 
> > diff --git a/net/tipc/node.c b/net/tipc/node.c
> > index b48d97cbbe29..23419a599471 100644
> > --- a/net/tipc/node.c
> > +++ b/net/tipc/node.c
> > @@ -2561,6 +2561,7 @@ int tipc_nl_node_reset_link_stats(struct sk_buff *skb, struct genl_info *info)
> >  	struct net *net = sock_net(skb->sk);
> >  	struct tipc_net *tn = tipc_net(net);
> >  	struct tipc_link_entry *le;
> > +	int len;
> >
> >  	if (!info->attrs[TIPC_NLA_LINK])
> >  		return -EINVAL;
> > @@ -2574,7 +2575,14 @@ int tipc_nl_node_reset_link_stats(struct sk_buff *skb, struct genl_info *info)
> >  	if (!attrs[TIPC_NLA_LINK_NAME])
> >  		return -EINVAL;
> >
> > +	len = nla_len(attrs[TIPC_NLA_LINK_NAME]);
> > +	if (len <= 0)
> > +		return -EINVAL;
> > +
> >  	link_name = nla_data(attrs[TIPC_NLA_LINK_NAME]);
> > +	len = min_t(int, len, TIPC_MAX_LINK_NAME);
> > +	if (!memchr(link_name, '\0', len))
> > +		return -EINVAL;
> 
> Should we just change the netlink policy for this attribute to
> NLA_NUL_STRING, then?
I recognize this is redundant check. I will post v2 into net.