* [net] tipc: fix uninit-value in tipc_nl_node_reset_link_stats
@ 2022-07-06 3:47 Hoang Le
2022-07-06 20:33 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Hoang Le @ 2022-07-06 3:47 UTC (permalink / raw)
To: jmaloy, maloy, ying.xue, tung.q.nguyen, kuba, pabeni, edumazet,
tipc-discussion, netdev, davem
Cc: syzbot+a73d24a22eeeebe5f244
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(-)
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));
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [net] tipc: fix uninit-value in tipc_nl_node_reset_link_stats
2022-07-06 3:47 [net] tipc: fix uninit-value in tipc_nl_node_reset_link_stats Hoang Le
@ 2022-07-06 20:33 ` Jakub Kicinski
2022-07-06 20:34 ` Jakub Kicinski
2022-07-07 0:22 ` Hoang Huu Le
0 siblings, 2 replies; 6+ messages in thread
From: Jakub Kicinski @ 2022-07-06 20:33 UTC (permalink / raw)
To: Hoang Le
Cc: jmaloy, maloy, ying.xue, tung.q.nguyen, pabeni, edumazet,
tipc-discussion, netdev, davem, syzbot+a73d24a22eeeebe5f244
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)).
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net] tipc: fix uninit-value in tipc_nl_node_reset_link_stats
2022-07-06 20:33 ` Jakub Kicinski
@ 2022-07-06 20:34 ` Jakub Kicinski
2022-07-07 0:22 ` Hoang Huu Le
1 sibling, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2022-07-06 20:34 UTC (permalink / raw)
To: Hoang Le
Cc: jmaloy, maloy, ying.xue, tung.q.nguyen, pabeni, edumazet,
tipc-discussion, netdev, davem, syzbot+a73d24a22eeeebe5f244
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [net] tipc: fix uninit-value in tipc_nl_node_reset_link_stats
2022-07-06 20:33 ` Jakub Kicinski
2022-07-06 20:34 ` Jakub Kicinski
@ 2022-07-07 0:22 ` Hoang Huu Le
2022-07-07 0:57 ` Jakub Kicinski
1 sibling, 1 reply; 6+ messages in thread
From: Hoang Huu Le @ 2022-07-07 0:22 UTC (permalink / raw)
To: Jakub Kicinski
Cc: jmaloy, maloy, ying.xue, Tung Quang Nguyen, pabeni, edumazet,
tipc-discussion, netdev, davem, syzbot+a73d24a22eeeebe5f244
> -----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)).
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net] tipc: fix uninit-value in tipc_nl_node_reset_link_stats
2022-07-07 0:22 ` Hoang Huu Le
@ 2022-07-07 0:57 ` Jakub Kicinski
2022-07-07 6:36 ` Hoang Huu Le
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2022-07-07 0:57 UTC (permalink / raw)
To: Hoang Huu Le
Cc: jmaloy, maloy, ying.xue, Tung Quang Nguyen, pabeni, edumazet,
tipc-discussion, netdev, davem, syzbot+a73d24a22eeeebe5f244
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [net] tipc: fix uninit-value in tipc_nl_node_reset_link_stats
2022-07-07 0:57 ` Jakub Kicinski
@ 2022-07-07 6:36 ` Hoang Huu Le
0 siblings, 0 replies; 6+ messages in thread
From: Hoang Huu Le @ 2022-07-07 6:36 UTC (permalink / raw)
To: Jakub Kicinski
Cc: jmaloy, maloy, ying.xue, Tung Quang Nguyen, pabeni, edumazet,
tipc-discussion, netdev, davem, syzbot+a73d24a22eeeebe5f244
> -----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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-07-07 6:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-06 3:47 [net] tipc: fix uninit-value in tipc_nl_node_reset_link_stats Hoang Le
2022-07-06 20:33 ` Jakub Kicinski
2022-07-06 20:34 ` Jakub Kicinski
2022-07-07 0:22 ` Hoang Huu Le
2022-07-07 0:57 ` Jakub Kicinski
2022-07-07 6:36 ` Hoang Huu Le
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.