| Message ID | 20171204183143.7395-1-xiyou.wangcong@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,v2] tipc: fix a null pointer deref on error path | expand |
From: Cong Wang <xiyou.wangcong@gmail.com> Date: Mon, 4 Dec 2017 10:31:43 -0800 > In tipc_topsrv_kern_subscr() when s->tipc_conn_new() fails > we call tipc_close_conn() to clean up, but in this case > calling conn_put() is just enough. > > This fixes the folllowing crash: ... > Fixes: 14c04493cb77 ("tipc: add ability to order and receive topology events in driver") > Reported-by: syzbot <syzkaller@googlegroups.com> > Cc: Jon Maloy <jon.maloy@ericsson.com> > Cc: Ying Xue <ying.xue@windriver.com> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> ... > @@ -511,7 +511,7 @@ bool tipc_topsrv_kern_subscr(struct net *net, u32 port, u32 type, > s = con->server; > scbr = s->tipc_conn_new(*conid); > if (!scbr) { > - tipc_close_conn(con); > + conn_put(con); > return false; > } > > -- > 2.13.0 > It looks like tipc_accept_from_sock() has a similar problem? The tipc_close_conn() will get invoked indirectly from the sock_release() path right?
On Mon, Dec 4, 2017 at 10:57 AM, David Miller <davem@davemloft.net> wrote: > From: Cong Wang <xiyou.wangcong@gmail.com> > Date: Mon, 4 Dec 2017 10:31:43 -0800 > >> In tipc_topsrv_kern_subscr() when s->tipc_conn_new() fails >> we call tipc_close_conn() to clean up, but in this case >> calling conn_put() is just enough. >> >> This fixes the folllowing crash: > ... >> Fixes: 14c04493cb77 ("tipc: add ability to order and receive topology events in driver") >> Reported-by: syzbot <syzkaller@googlegroups.com> >> Cc: Jon Maloy <jon.maloy@ericsson.com> >> Cc: Ying Xue <ying.xue@windriver.com> >> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > ... >> @@ -511,7 +511,7 @@ bool tipc_topsrv_kern_subscr(struct net *net, u32 port, u32 type, >> s = con->server; >> scbr = s->tipc_conn_new(*conid); >> if (!scbr) { >> - tipc_close_conn(con); >> + conn_put(con); >> return false; >> } >> >> -- >> 2.13.0 >> > > It looks like tipc_accept_from_sock() has a similar problem? The > tipc_close_conn() will get invoked indirectly from the sock_release() > path right? Not sure, the sock_release() in tipc_accept_from_sock() is for kernel_accept(), not for tipc_alloc_conn(). Or maybe it is hiding deep in the call chain that I miss?
On Mon, Dec 4, 2017 at 11:23 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote: > On Mon, Dec 4, 2017 at 10:57 AM, David Miller <davem@davemloft.net> wrote: >> >> It looks like tipc_accept_from_sock() has a similar problem? The >> tipc_close_conn() will get invoked indirectly from the sock_release() >> path right? > > Not sure, the sock_release() in tipc_accept_from_sock() is for > kernel_accept(), not for tipc_alloc_conn(). Or maybe it is hiding > deep in the call chain that I miss? I see: tipc_release() -> tipc_sk_leave() -> tipc_group_delete() -> tipc_topsrv_kern_unsubscr() -> tipc_close_conn() Seems on this path we do need to skip NULL too.
> -----Original Message----- > From: netdev-owner@vger.kernel.org [mailto:netdev- > owner@vger.kernel.org] On Behalf Of David Miller > Sent: Monday, December 04, 2017 13:57 > To: xiyou.wangcong@gmail.com > Cc: netdev@vger.kernel.org; tipc-discussion@lists.sourceforge.net; Jon > Maloy <jon.maloy@ericsson.com>; Ying Xue <ying.xue@windriver.com> > Subject: Re: [Patch net v2] tipc: fix a null pointer deref on error path > > From: Cong Wang <xiyou.wangcong@gmail.com> > Date: Mon, 4 Dec 2017 10:31:43 -0800 > > > In tipc_topsrv_kern_subscr() when s->tipc_conn_new() fails we call > > tipc_close_conn() to clean up, but in this case calling conn_put() is > > just enough. > > > > This fixes the folllowing crash: > ... > > Fixes: 14c04493cb77 ("tipc: add ability to order and receive topology > > events in driver") > > Reported-by: syzbot <syzkaller@googlegroups.com> > > Cc: Jon Maloy <jon.maloy@ericsson.com> > > Cc: Ying Xue <ying.xue@windriver.com> > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > ... > > @@ -511,7 +511,7 @@ bool tipc_topsrv_kern_subscr(struct net *net, u32 > port, u32 type, > > s = con->server; > > scbr = s->tipc_conn_new(*conid); > > if (!scbr) { > > - tipc_close_conn(con); > > + conn_put(con); > > return false; > > } > > > > -- > > 2.13.0 > > > > It looks like tipc_accept_from_sock() has a similar problem? The > tipc_close_conn() will get invoked indirectly from the sock_release() > path right? No, it doesn't. There will be a 'leaked' conn instance which will remain in the reference table until it is flushed during module removal. We'll fix this in a separate patch. Cong's fix is correct. ///jon
From: Cong Wang <xiyou.wangcong@gmail.com> Date: Mon, 4 Dec 2017 11:23:13 -0800 > On Mon, Dec 4, 2017 at 10:57 AM, David Miller <davem@davemloft.net> wrote: >> From: Cong Wang <xiyou.wangcong@gmail.com> >> Date: Mon, 4 Dec 2017 10:31:43 -0800 >> >>> In tipc_topsrv_kern_subscr() when s->tipc_conn_new() fails >>> we call tipc_close_conn() to clean up, but in this case >>> calling conn_put() is just enough. >>> >>> This fixes the folllowing crash: >> ... >>> Fixes: 14c04493cb77 ("tipc: add ability to order and receive topology events in driver") >>> Reported-by: syzbot <syzkaller@googlegroups.com> >>> Cc: Jon Maloy <jon.maloy@ericsson.com> >>> Cc: Ying Xue <ying.xue@windriver.com> >>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> >> ... >>> @@ -511,7 +511,7 @@ bool tipc_topsrv_kern_subscr(struct net *net, u32 port, u32 type, >>> s = con->server; >>> scbr = s->tipc_conn_new(*conid); >>> if (!scbr) { >>> - tipc_close_conn(con); >>> + conn_put(con); >>> return false; >>> } >>> >>> -- >>> 2.13.0 >>> >> >> It looks like tipc_accept_from_sock() has a similar problem? The >> tipc_close_conn() will get invoked indirectly from the sock_release() >> path right? > > Not sure, the sock_release() in tipc_accept_from_sock() is for > kernel_accept(), not for tipc_alloc_conn(). Or maybe it is hiding > deep in the call chain that I miss? I guess I'm trying to figure out where 'newcon' is released when the call to s->tipc_conn_new() on it fails. It looks similar to the situation you are fixing here, which is why I am mentioning it.
> -----Original Message----- > From: Cong Wang [mailto:xiyou.wangcong@gmail.com] > Sent: Monday, December 04, 2017 14:41 > To: David Miller <davem@davemloft.net> > Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>; tipc- > discussion@lists.sourceforge.net; Jon Maloy <jon.maloy@ericsson.com>; > Ying Xue <ying.xue@windriver.com> > Subject: Re: [Patch net v2] tipc: fix a null pointer deref on error path > > On Mon, Dec 4, 2017 at 11:23 AM, Cong Wang <xiyou.wangcong@gmail.com> > wrote: > > On Mon, Dec 4, 2017 at 10:57 AM, David Miller <davem@davemloft.net> > wrote: > >> > >> It looks like tipc_accept_from_sock() has a similar problem? The > >> tipc_close_conn() will get invoked indirectly from the sock_release() > >> path right? > > > > Not sure, the sock_release() in tipc_accept_from_sock() is for > > kernel_accept(), not for tipc_alloc_conn(). Or maybe it is hiding deep > > in the call chain that I miss? > > I see: > > tipc_release() -> tipc_sk_leave() -> tipc_group_delete() > -> tipc_topsrv_kern_unsubscr() -> tipc_close_conn() > > Seems on this path we do need to skip NULL too. You are right. The right solution is to just call conn_put() twice here. I already have a patch ready for this, but it is part of a series that needs more review. I should probably post it separately... ///jon
> -----Original Message----- > From: netdev-owner@vger.kernel.org [mailto:netdev- > owner@vger.kernel.org] On Behalf Of Jon Maloy > Sent: Monday, December 04, 2017 14:50 > To: Cong Wang <xiyou.wangcong@gmail.com>; David Miller > <davem@davemloft.net> > Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>; tipc- > discussion@lists.sourceforge.net; Ying Xue <ying.xue@windriver.com> > Subject: RE: [Patch net v2] tipc: fix a null pointer deref on error path > > > > > -----Original Message----- > > From: Cong Wang [mailto:xiyou.wangcong@gmail.com] > > Sent: Monday, December 04, 2017 14:41 > > To: David Miller <davem@davemloft.net> > > Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>; tipc- > > discussion@lists.sourceforge.net; Jon Maloy <jon.maloy@ericsson.com>; > > Ying Xue <ying.xue@windriver.com> > > Subject: Re: [Patch net v2] tipc: fix a null pointer deref on error > > path > > > > On Mon, Dec 4, 2017 at 11:23 AM, Cong Wang > <xiyou.wangcong@gmail.com> > > wrote: > > > On Mon, Dec 4, 2017 at 10:57 AM, David Miller <davem@davemloft.net> > > wrote: > > >> > > >> It looks like tipc_accept_from_sock() has a similar problem? The > > >> tipc_close_conn() will get invoked indirectly from the > > >> sock_release() path right? > > > > > > Not sure, the sock_release() in tipc_accept_from_sock() is for > > > kernel_accept(), not for tipc_alloc_conn(). Or maybe it is hiding > > > deep in the call chain that I miss? > > > > I see: > > > > tipc_release() -> tipc_sk_leave() -> tipc_group_delete() > > -> tipc_topsrv_kern_unsubscr() -> tipc_close_conn() > > > > Seems on this path we do need to skip NULL too. > > You are right. The right solution is to just call conn_put() twice here. > I already have a patch ready for this, but it is part of a series that needs more > review. > I should probably post it separately... Well, calling conn_put() twice was ok in my series, but in the current upstream version it is not enough. I will find a different short term solution. ///jon > > ///jon
On Mon, Dec 4, 2017 at 12:32 PM, Jon Maloy <jon.maloy@ericsson.com> wrote: > >> You are right. The right solution is to just call conn_put() twice here. >> I already have a patch ready for this, but it is part of a series that needs more >> review. >> I should probably post it separately... > > Well, calling conn_put() twice was ok in my series, but in the current upstream version it is not enough. > I will find a different short term solution. > IMHO, for tipc_topsrv_kern_subscr() my v2 patch is more correct, and for tipc_accept_from_sock(), v1 is needed too. Of course v1 could fix both, but still v2 is better than v1 if we only consider tipc_topsrv_kern_subscr() case. So this depends if we want to consider these 2 paths as 2 cases or 1 case. _I think_ it is better to consider them separately since we already v2 is best fix for tipc_topsrv_kern_subscr(), while still not sure what is the best for tipc_accept_from_sock().
> -----Original Message----- > From: Cong Wang [mailto:xiyou.wangcong@gmail.com] > Sent: Monday, December 04, 2017 17:34 > To: Jon Maloy <jon.maloy@ericsson.com> > Cc: David Miller <davem@davemloft.net>; Linux Kernel Network Developers > <netdev@vger.kernel.org>; tipc-discussion@lists.sourceforge.net; Ying Xue > <ying.xue@windriver.com> > Subject: Re: [Patch net v2] tipc: fix a null pointer deref on error path > > On Mon, Dec 4, 2017 at 12:32 PM, Jon Maloy <jon.maloy@ericsson.com> > wrote: > > > >> You are right. The right solution is to just call conn_put() twice here. > >> I already have a patch ready for this, but it is part of a series > >> that needs more review. > >> I should probably post it separately... > > > > Well, calling conn_put() twice was ok in my series, but in the current > upstream version it is not enough. > > I will find a different short term solution. > > > > IMHO, for tipc_topsrv_kern_subscr() my v2 patch is more correct, and for > tipc_accept_from_sock(), v1 is needed too. Absolutely. I already acked v2, but maybe I wasn't clear enough about that. What I was referring to above was tipc_kern_unsubscr(), where you also, correctly, identified a problem. That problem is reduced, but not totally eliminated by your v1, so I am ok with that one too for now. We have a fully safe solution for this in the pipe. > > Of course v1 could fix both, but still v2 is better than v1 if we only consider > tipc_topsrv_kern_subscr() case. So this depends if we want to consider > these 2 paths as 2 cases or 1 case. _I think_ it is better to consider them > separately since we already v2 is best fix for tipc_topsrv_kern_subscr(), > while still not sure what is the best for tipc_accept_from_sock(). The tipc_accept_from_sock() problem is as far as I can see totally analogous to the tipc_kern_subscr() one, and requires the same solution. This is what my patch is providing. BR ///jon
From: Cong Wang <xiyou.wangcong@gmail.com> Date: Mon, 4 Dec 2017 10:31:43 -0800 > In tipc_topsrv_kern_subscr() when s->tipc_conn_new() fails > we call tipc_close_conn() to clean up, but in this case > calling conn_put() is just enough. > > This fixes the folllowing crash: ... > Fixes: 14c04493cb77 ("tipc: add ability to order and receive topology events in driver") > Reported-by: syzbot <syzkaller@googlegroups.com> > Cc: Jon Maloy <jon.maloy@ericsson.com> > Cc: Ying Xue <ying.xue@windriver.com> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> Applied and queued up for -stable.
diff --git a/net/tipc/server.c b/net/tipc/server.c index acaef80fb88c..2710101ba4c1 100644 --- a/net/tipc/server.c +++ b/net/tipc/server.c @@ -511,7 +511,7 @@ bool tipc_topsrv_kern_subscr(struct net *net, u32 port, u32 type, s = con->server; scbr = s->tipc_conn_new(*conid); if (!scbr) { - tipc_close_conn(con); + conn_put(con); return false; }
In tipc_topsrv_kern_subscr() when s->tipc_conn_new() fails we call tipc_close_conn() to clean up, but in this case calling conn_put() is just enough. This fixes the folllowing crash: kasan: GPF could be caused by NULL-ptr deref or user memory access general protection fault: 0000 [#1] SMP KASAN Dumping ftrace buffer: (ftrace buffer empty) Modules linked in: CPU: 0 PID: 3085 Comm: syzkaller064164 Not tainted 4.15.0-rc1+ #137 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 task: 00000000c24413a5 task.stack: 000000005e8160b5 RIP: 0010:__lock_acquire+0xd55/0x47f0 kernel/locking/lockdep.c:3378 RSP: 0018:ffff8801cb5474a8 EFLAGS: 00010002 RAX: dffffc0000000000 RBX: 0000000000000000 RCX: 0000000000000000 RDX: 0000000000000004 RSI: 0000000000000000 RDI: ffffffff85ecb400 RBP: ffff8801cb547830 R08: 0000000000000001 R09: 0000000000000000 R10: 0000000000000000 R11: ffffffff87489d60 R12: ffff8801cd2980c0 R13: 0000000000000000 R14: 0000000000000001 R15: 0000000000000020 FS: 00000000014ee880(0000) GS:ffff8801db400000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007ffee2426e40 CR3: 00000001cb85a000 CR4: 00000000001406f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: lock_acquire+0x1d5/0x580 kernel/locking/lockdep.c:4004 __raw_spin_lock_bh include/linux/spinlock_api_smp.h:135 [inline] _raw_spin_lock_bh+0x31/0x40 kernel/locking/spinlock.c:175 spin_lock_bh include/linux/spinlock.h:320 [inline] tipc_subscrb_subscrp_delete+0x8f/0x470 net/tipc/subscr.c:201 tipc_subscrb_delete net/tipc/subscr.c:238 [inline] tipc_subscrb_release_cb+0x17/0x30 net/tipc/subscr.c:316 tipc_close_conn+0x171/0x270 net/tipc/server.c:204 tipc_topsrv_kern_subscr+0x724/0x810 net/tipc/server.c:514 tipc_group_create+0x702/0x9c0 net/tipc/group.c:184 tipc_sk_join net/tipc/socket.c:2747 [inline] tipc_setsockopt+0x249/0xc10 net/tipc/socket.c:2861 SYSC_setsockopt net/socket.c:1851 [inline] SyS_setsockopt+0x189/0x360 net/socket.c:1830 entry_SYSCALL_64_fastpath+0x1f/0x96 Fixes: 14c04493cb77 ("tipc: add ability to order and receive topology events in driver") Reported-by: syzbot <syzkaller@googlegroups.com> Cc: Jon Maloy <jon.maloy@ericsson.com> Cc: Ying Xue <ying.xue@windriver.com> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- net/tipc/server.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)