netdev - Re: [PATCH v1 net] can: bcm: Remove proc entry when dev is unregistered.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240723183805.31201-1-kuniyu@amazon.com>
Date: Tue, 23 Jul 2024 11:38:05 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <horms@...nel.org>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<kuni1840@...il.com>, <kuniyu@...zon.com>, <linux-can@...r.kernel.org>,
	<mkl@...gutronix.de>, <netdev@...r.kernel.org>, <pabeni@...hat.com>,
	<socketcan@...tkopp.net>, <syzkaller@...glegroups.com>
Subject: Re: [PATCH v1 net] can: bcm: Remove proc entry when dev is unregistered.

From: Simon Horman <horms@...nel.org>
Date: Tue, 23 Jul 2024 10:04:05 +0100
> On Mon, Jul 22, 2024 at 12:28:42PM -0700, Kuniyuki Iwashima wrote:
> > syzkaller reported a warning in bcm_connect() below. [0]
> > 
> > The repro calls connect() to vxcan1, removes vxcan1, and calls
> > connect() with ifindex == 0.
> > 
> > Calling connect() for a BCM socket allocates a proc entry.
> > Then, bcm_sk(sk)->bound is set to 1 to prevent further connect().
> > 
> > However, removing the bound device resets bcm_sk(sk)->bound to 0
> > in bcm_notify().
> > 
> > The 2nd connect() tries to allocate a proc entry with the same
> > name and sets NULL to bcm_sk(sk)->bcm_proc_read, leaking the
> > original proc entry.
> > 
> > Since the proc entry is available only for connect()ed sockets,
> > let's clean up the entry when the bound netdev is unregistered.
> > 
> > [0]:
> > proc_dir_entry 'can-bcm/2456' already registered
> > WARNING: CPU: 1 PID: 394 at fs/proc/generic.c:376 proc_register+0x645/0x8f0 fs/proc/generic.c:375
> > Modules linked in:
> > CPU: 1 PID: 394 Comm: syz-executor403 Not tainted 6.10.0-rc7-g852e42cc2dd4
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
> > RIP: 0010:proc_register+0x645/0x8f0 fs/proc/generic.c:375
> > Code: 00 00 00 00 00 48 85 ed 0f 85 97 02 00 00 4d 85 f6 0f 85 9f 02 00 00 48 c7 c7 9b cb cf 87 48 89 de 4c 89 fa e8 1c 6f eb fe 90 <0f> 0b 90 90 48 c7 c7 98 37 99 89 e8 cb 7e 22 05 bb 00 00 00 10 48
> > RSP: 0018:ffa0000000cd7c30 EFLAGS: 00010246
> > RAX: 9e129be1950f0200 RBX: ff1100011b51582c RCX: ff1100011857cd80
> > RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000002
> > RBP: 0000000000000000 R08: ffd400000000000f R09: ff1100013e78cac0
> > R10: ffac800000cd7980 R11: ff1100013e12b1f0 R12: 0000000000000000
> > R13: 0000000000000000 R14: 0000000000000000 R15: ff1100011a99a2ec
> > FS:  00007fbd7086f740(0000) GS:ff1100013fd00000(0000) knlGS:0000000000000000
> > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 00000000200071c0 CR3: 0000000118556004 CR4: 0000000000771ef0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
> > PKRU: 55555554
> > Call Trace:
> >  <TASK>
> >  proc_create_net_single+0x144/0x210 fs/proc/proc_net.c:220
> >  bcm_connect+0x472/0x840 net/can/bcm.c:1673
> >  __sys_connect_file net/socket.c:2049 [inline]
> >  __sys_connect+0x5d2/0x690 net/socket.c:2066
> >  __do_sys_connect net/socket.c:2076 [inline]
> >  __se_sys_connect net/socket.c:2073 [inline]
> >  __x64_sys_connect+0x8f/0x100 net/socket.c:2073
> >  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> >  do_syscall_64+0xd9/0x1c0 arch/x86/entry/common.c:83
> >  entry_SYSCALL_64_after_hwframe+0x4b/0x53
> > RIP: 0033:0x7fbd708b0e5d
> > Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 73 9f 1b 00 f7 d8 64 89 01 48
> > RSP: 002b:00007fff8cd33f08 EFLAGS: 00000246 ORIG_RAX: 000000000000002a
> > RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00007fbd708b0e5d
> > RDX: 0000000000000010 RSI: 0000000020000040 RDI: 0000000000000003
> > RBP: 0000000000000000 R08: 0000000000000040 R09: 0000000000000040
> > R10: 0000000000000040 R11: 0000000000000246 R12: 00007fff8cd34098
> > R13: 0000000000401280 R14: 0000000000406de8 R15: 00007fbd70ab9000
> >  </TASK>
> > remove_proc_entry: removing non-empty directory 'net/can-bcm', leaking at least '2456'
> > 
> > Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
> > Reported-by: syzkaller <syzkaller@...glegroups.com>
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> 
> Thanks,
> 
> I agree that the problem was introduced by the cited commit
> and is resolved by this patch.
> 
> Reviewed-by: Simon Horman <horms@...nel.org>
> 
> > ---
> >  net/can/bcm.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/net/can/bcm.c b/net/can/bcm.c
> > index 27d5fcf0eac9..46d3ec3aa44b 100644
> > --- a/net/can/bcm.c
> > +++ b/net/can/bcm.c
> > @@ -1470,6 +1470,10 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
> >  
> >  		/* remove device reference, if this is our bound device */
> >  		if (bo->bound && bo->ifindex == dev->ifindex) {
> > +#if IS_ENABLED(CONFIG_PROC_FS)
> > +			if (sock_net(sk)->can.bcmproc_dir && bo->bcm_proc_read)
> > +				remove_proc_entry(bo->procname, sock_net(sk)->can.bcmproc_dir);
> > +#endif
> 
> As a fix this looks good. But I wonder if it is worth following up
> with a helper for the above as it inlines #if logic and now appears twice.

Other places also have #if guard for CONFIG_PROC_FS, so if needed,
I'd move all of them into helper functions under a single #if in -next.