| Message ID | 20220706034752.5729-1-hoang.h.le@dektech.com.au (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] tipc: fix uninit-value in tipc_nl_node_reset_link_stats | expand |
On Wed, 6 Jul 2022 10:47:52 +0700 Hoang Le wrote: > diff --git a/net/tipc/node.c b/net/tipc/node.c > index b48d97cbbe29..80885780caa2 100644 > --- a/net/tipc/node.c > +++ b/net/tipc/node.c > @@ -2574,8 +2574,10 @@ int tipc_nl_node_reset_link_stats(struct sk_buff *skb, struct genl_info *info) > if (!attrs[TIPC_NLA_LINK_NAME]) > return -EINVAL; > > - link_name = nla_data(attrs[TIPC_NLA_LINK_NAME]); > + if (nla_len(attrs[TIPC_NLA_LINK_NAME]) <= 0) > + return -EINVAL; > > + link_name = nla_data(attrs[TIPC_NLA_LINK_NAME]); > err = -EINVAL; > if (!strcmp(link_name, tipc_bclink_name)) { > err = tipc_bclink_reset_stats(net, tipc_bc_sndlink(net)); I think you misunderstood me. Let me try to explain in more detail. AFAICT the attrs in this function get validated using the tipc_nl_link_policy: https://elixir.bootlin.com/linux/v5.19-rc4/source/net/tipc/node.c#L2567 This policy specifies the type for TIPC_NLA_LINK_NAME as NLA_STRING: https://elixir.bootlin.com/linux/v5.19-rc4/source/net/tipc/netlink.c#L91 Therefore we can assume that the attribute is a valid (but not necessarily null-terminated) string. Otherwise nla_parse_nested_deprecated() would have returned an error. The code for NLA_STRING validation is here: https://elixir.bootlin.com/linux/v5.19-rc4/source/lib/nlattr.c#L437 So we can already assume that the attribute is not empty. The bug you're fixing is that the string is not null-terminated, so strcmp() can read past the end of the attribute. What I was trying to suggest is that you change the policy from NLA_STRING to NLA_NUL_STRING, which also checks that the string is NULL-terminated. Please note that it'd be a slight uAPI change, and could break applications which expect kernel to not require null-termination. Perhaps tipc developers can guide us on how likely that is. Alternative is to use strncmp(..., nla_len(attr)).
On Wed, 6 Jul 2022 13:33:34 -0700 Jakub Kicinski wrote:
> Alternative is to use strncmp(..., nla_len(attr)).
Correction, we have helpers for this - nla_strcmp etc.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Thursday, July 7, 2022 3:34 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] tipc: fix uninit-value in tipc_nl_node_reset_link_stats > > On Wed, 6 Jul 2022 10:47:52 +0700 Hoang Le wrote: > > diff --git a/net/tipc/node.c b/net/tipc/node.c > > index b48d97cbbe29..80885780caa2 100644 > > --- a/net/tipc/node.c > > +++ b/net/tipc/node.c > > @@ -2574,8 +2574,10 @@ int tipc_nl_node_reset_link_stats(struct sk_buff *skb, struct genl_info *info) > > if (!attrs[TIPC_NLA_LINK_NAME]) > > return -EINVAL; > > > > - link_name = nla_data(attrs[TIPC_NLA_LINK_NAME]); > > + if (nla_len(attrs[TIPC_NLA_LINK_NAME]) <= 0) > > + return -EINVAL; > > > > + link_name = nla_data(attrs[TIPC_NLA_LINK_NAME]); > > err = -EINVAL; > > if (!strcmp(link_name, tipc_bclink_name)) { > > err = tipc_bclink_reset_stats(net, tipc_bc_sndlink(net)); > > I think you misunderstood me. Let me try to explain in more detail. > > AFAICT the attrs in this function get validated using the > tipc_nl_link_policy: > > https://elixir.bootlin.com/linux/v5.19-rc4/source/net/tipc/node.c#L2567 > > This policy specifies the type for TIPC_NLA_LINK_NAME as NLA_STRING: > > https://elixir.bootlin.com/linux/v5.19-rc4/source/net/tipc/netlink.c#L91 > > Therefore we can assume that the attribute is a valid (but not > necessarily null-terminated) string. Otherwise > nla_parse_nested_deprecated() would have returned an error. > > The code for NLA_STRING validation is here: > > https://elixir.bootlin.com/linux/v5.19-rc4/source/lib/nlattr.c#L437 > > So we can already assume that the attribute is not empty. > > The bug you're fixing is that the string is not null-terminated, > so strcmp() can read past the end of the attribute. > No, I'm trying to fix below issues: 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 I assume the link name attribute is empty as root cause. So, just checking length is enough for fixing this issue. > What I was trying to suggest is that you change the policy from > NLA_STRING to NLA_NUL_STRING, which also checks that the string > is NULL-terminated. > > Please note that it'd be a slight uAPI change, and could break > applications which expect kernel to not require null-termination. > Perhaps tipc developers can guide us on how likely that is. > Alternative is to use strncmp(..., nla_len(attr)).
On Thu, 7 Jul 2022 00:22:06 +0000 Hoang Huu Le wrote: > > The bug you're fixing is that the string is not null-terminated, > > so strcmp() can read past the end of the attribute. > > > No, I'm trying to fix below issues: > > 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 > > I assume the link name attribute is empty as root cause. > So, just checking length is enough for fixing this issue. I pointed you to the code that does NLA_STRING validation and that already checks: if (attrlen < 1) what am I missing?
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Thursday, July 7, 2022 7:58 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] tipc: fix uninit-value in tipc_nl_node_reset_link_stats > > On Thu, 7 Jul 2022 00:22:06 +0000 Hoang Huu Le wrote: > > > The bug you're fixing is that the string is not null-terminated, > > > so strcmp() can read past the end of the attribute. > > > > > No, I'm trying to fix below issues: > > > > 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 > > > > I assume the link name attribute is empty as root cause. > > So, just checking length is enough for fixing this issue. > > I pointed you to the code that does NLA_STRING validation and that > already checks: > > if (attrlen < 1) > > what am I missing? You're correct! I will post v3 when I find out a better solution to solve the problem.
diff --git a/net/tipc/node.c b/net/tipc/node.c index b48d97cbbe29..80885780caa2 100644 --- a/net/tipc/node.c +++ b/net/tipc/node.c @@ -2574,8 +2574,10 @@ int tipc_nl_node_reset_link_stats(struct sk_buff *skb, struct genl_info *info) if (!attrs[TIPC_NLA_LINK_NAME]) return -EINVAL; - link_name = nla_data(attrs[TIPC_NLA_LINK_NAME]); + if (nla_len(attrs[TIPC_NLA_LINK_NAME]) <= 0) + return -EINVAL; + link_name = nla_data(attrs[TIPC_NLA_LINK_NAME]); err = -EINVAL; if (!strcmp(link_name, tipc_bclink_name)) { err = tipc_bclink_reset_stats(net, tipc_bc_sndlink(net));
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 Fixes: 03b6fefd9bb4 ("tipc: add support for broadcast rcv stats dumping") Acked-by: Jon Maloy <jmaloy@redhat.com> Signed-off-by: Hoang Le <hoang.h.le@dektech.com.au> --- v2: remove redundant check --- net/tipc/node.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)