[PATCH net] net, neigh: clear whole pneigh_entry at alloc time

[PATCH net] net, neigh: clear whole pneigh_entry at alloc time

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Commit 2c611ad97a82 ("net, neigh: Extend neigh->flags to 32 bit
to allow for extensions") enables a new KMSAM warning [1]

I think the bug is actually older, because the following intruction
only occurred if ndm->ndm_flags had NTF_PROXY set.

	pn->flags = ndm->ndm_flags;

Let's clear all pneigh_entry fields at alloc time.

[1]
BUG: KMSAN: uninit-value in pneigh_fill_info+0x986/0xb30 net/core/neighbour.c:2593
 pneigh_fill_info+0x986/0xb30 net/core/neighbour.c:2593
 pneigh_dump_table net/core/neighbour.c:2715 [inline]
 neigh_dump_info+0x1e3f/0x2c60 net/core/neighbour.c:2832
 netlink_dump+0xaca/0x16a0 net/netlink/af_netlink.c:2265
 __netlink_dump_start+0xd1c/0xee0 net/netlink/af_netlink.c:2370
 netlink_dump_start include/linux/netlink.h:254 [inline]
 rtnetlink_rcv_msg+0x181b/0x18c0 net/core/rtnetlink.c:5534
 netlink_rcv_skb+0x447/0x800 net/netlink/af_netlink.c:2491
 rtnetlink_rcv+0x50/0x60 net/core/rtnetlink.c:5589
 netlink_unicast_kernel net/netlink/af_netlink.c:1319 [inline]
 netlink_unicast+0x1095/0x1360 net/netlink/af_netlink.c:1345
 netlink_sendmsg+0x16f3/0x1870 net/netlink/af_netlink.c:1916
 sock_sendmsg_nosec net/socket.c:704 [inline]
 sock_sendmsg net/socket.c:724 [inline]
 sock_write_iter+0x594/0x690 net/socket.c:1057
 call_write_iter include/linux/fs.h:2162 [inline]
 new_sync_write fs/read_write.c:503 [inline]
 vfs_write+0x1318/0x2030 fs/read_write.c:590
 ksys_write+0x28c/0x520 fs/read_write.c:643
 __do_sys_write fs/read_write.c:655 [inline]
 __se_sys_write fs/read_write.c:652 [inline]
 __x64_sys_write+0xdb/0x120 fs/read_write.c:652
 do_syscall_x64 arch/x86/entry/common.c:51 [inline]
 do_syscall_64+0x54/0xd0 arch/x86/entry/common.c:82
 entry_SYSCALL_64_after_hwframe+0x44/0xae

Uninit was created at:
 slab_post_alloc_hook mm/slab.h:524 [inline]
 slab_alloc_node mm/slub.c:3251 [inline]
 slab_alloc mm/slub.c:3259 [inline]
 __kmalloc+0xc3c/0x12d0 mm/slub.c:4437
 kmalloc include/linux/slab.h:595 [inline]
 pneigh_lookup+0x60f/0xd70 net/core/neighbour.c:766
 arp_req_set_public net/ipv4/arp.c:1016 [inline]
 arp_req_set+0x430/0x10a0 net/ipv4/arp.c:1032
 arp_ioctl+0x8d4/0xb60 net/ipv4/arp.c:1232
 inet_ioctl+0x4ef/0x820 net/ipv4/af_inet.c:947
 sock_do_ioctl net/socket.c:1118 [inline]
 sock_ioctl+0xa3f/0x13e0 net/socket.c:1235
 vfs_ioctl fs/ioctl.c:51 [inline]
 __do_sys_ioctl fs/ioctl.c:874 [inline]
 __se_sys_ioctl+0x2df/0x4a0 fs/ioctl.c:860
 __x64_sys_ioctl+0xd8/0x110 fs/ioctl.c:860
 do_syscall_x64 arch/x86/entry/common.c:51 [inline]
 do_syscall_64+0x54/0xd0 arch/x86/entry/common.c:82
 entry_SYSCALL_64_after_hwframe+0x44/0xae

CPU: 1 PID: 20001 Comm: syz-executor.0 Not tainted 5.16.0-rc3-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011

Fixes: 62dd93181aaa ("[IPV6] NDISC: Set per-entry is_router flag in Proxy NA.")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Roopa Prabhu <roopa@nvidia.com>
---
 net/core/neighbour.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 72ba027c34cfea6f38a9e78927c35048ebfe7a7f..dda12fbd177ba6ad2798ea2b07733fa3f03441ab 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -763,11 +763,10 @@ struct pneigh_entry * pneigh_lookup(struct neigh_table *tbl,
 
 	ASSERT_RTNL();
 
-	n = kmalloc(sizeof(*n) + key_len, GFP_KERNEL);
+	n = kzalloc(sizeof(*n) + key_len, GFP_KERNEL);
 	if (!n)
 		goto out;
 
-	n->protocol = 0;
 	write_pnet(&n->net, net);
 	memcpy(n->key, pkey, key_len);
 	n->dev = dev;
-- 
2.34.1.400.ga245620fadb-goog

Re: [PATCH net] net, neigh: clear whole pneigh_entry at alloc time

[David Ahern]

On 12/6/21 9:53 AM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> Commit 2c611ad97a82 ("net, neigh: Extend neigh->flags to 32 bit
> to allow for extensions") enables a new KMSAM warning [1]
> 
> I think the bug is actually older, because the following intruction
> only occurred if ndm->ndm_flags had NTF_PROXY set.
> 
> 	pn->flags = ndm->ndm_flags;
> 
> Let's clear all pneigh_entry fields at alloc time.
> 

All of the fields - except the new flags field - are initialized after
the alloc. Why do you think the bug is older?

...

> Fixes: 62dd93181aaa ("[IPV6] NDISC: Set per-entry is_router flag in Proxy NA.")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Roopa Prabhu <roopa@nvidia.com>
> ---
>  net/core/neighbour.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 72ba027c34cfea6f38a9e78927c35048ebfe7a7f..dda12fbd177ba6ad2798ea2b07733fa3f03441ab 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -763,11 +763,10 @@ struct pneigh_entry * pneigh_lookup(struct neigh_table *tbl,
>  
>  	ASSERT_RTNL();
>  
> -	n = kmalloc(sizeof(*n) + key_len, GFP_KERNEL);
> +	n = kzalloc(sizeof(*n) + key_len, GFP_KERNEL);
>  	if (!n)
>  		goto out;
>  
> -	n->protocol = 0;
>  	write_pnet(&n->net, net);
>  	memcpy(n->key, pkey, key_len);
>  	n->dev = dev;
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net] net, neigh: clear whole pneigh_entry at alloc time

[Eric Dumazet]

On Wed, Dec 8, 2021 at 7:54 AM David Ahern <dsahern@gmail.com> wrote:
>
> On 12/6/21 9:53 AM, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > Commit 2c611ad97a82 ("net, neigh: Extend neigh->flags to 32 bit
> > to allow for extensions") enables a new KMSAM warning [1]
> >
> > I think the bug is actually older, because the following intruction
> > only occurred if ndm->ndm_flags had NTF_PROXY set.
> >
> >       pn->flags = ndm->ndm_flags;
> >
> > Let's clear all pneigh_entry fields at alloc time.
> >
>
> All of the fields - except the new flags field - are initialized after
> the alloc. Why do you think the bug is older?

Because the flags field was added earlier, in the commit I pointed in Fixes: tag

For some reason, syzbot found the issue only after Roopa patch went in.
But we need to backport to older versions I think, or risk a kernel-info-leak

>
> ...
>
> > Fixes: 62dd93181aaa ("[IPV6] NDISC: Set per-entry is_router flag in Proxy NA.")
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Roopa Prabhu <roopa@nvidia.com>
> > ---
> >  net/core/neighbour.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> > index 72ba027c34cfea6f38a9e78927c35048ebfe7a7f..dda12fbd177ba6ad2798ea2b07733fa3f03441ab 100644
> > --- a/net/core/neighbour.c
> > +++ b/net/core/neighbour.c
> > @@ -763,11 +763,10 @@ struct pneigh_entry * pneigh_lookup(struct neigh_table *tbl,
> >
> >       ASSERT_RTNL();
> >
> > -     n = kmalloc(sizeof(*n) + key_len, GFP_KERNEL);
> > +     n = kzalloc(sizeof(*n) + key_len, GFP_KERNEL);
> >       if (!n)
> >               goto out;
> >
> > -     n->protocol = 0;
> >       write_pnet(&n->net, net);
> >       memcpy(n->key, pkey, key_len);
> >       n->dev = dev;
> >
>
> Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net] net, neigh: clear whole pneigh_entry at alloc time

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Mon,  6 Dec 2021 08:53:29 -0800 you wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> Commit 2c611ad97a82 ("net, neigh: Extend neigh->flags to 32 bit
> to allow for extensions") enables a new KMSAM warning [1]
> 
> I think the bug is actually older, because the following intruction
> only occurred if ndm->ndm_flags had NTF_PROXY set.
> 
> [...]

Here is the summary with links:
  - [net] net, neigh: clear whole pneigh_entry at alloc time
    https://git.kernel.org/netdev/net/c/e195e9b5dee6

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html