[Patch net v2] tipc: fix a null pointer deref on error path

[Patch net v2] tipc: fix a null pointer deref on error path

[Cong Wang]

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(-)

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;
 	}
 
-- 
2.13.0

Re: [Patch net v2] tipc: fix a null pointer deref on error path

[David Miller]

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?

Re: [Patch net v2] tipc: fix a null pointer deref on error path

[Cong Wang]

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?

Re: [Patch net v2] tipc: fix a null pointer deref on error path

[Cong Wang]

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.

RE: [Patch net v2] tipc: fix a null pointer deref on error path

[Jon Maloy]

> -----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

Re: [Patch net v2] tipc: fix a null pointer deref on error path

[David Miller]

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.

RE: [Patch net v2] tipc: fix a null pointer deref on error path

[Jon Maloy]

> -----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

RE: [Patch net v2] tipc: fix a null pointer deref on error path

[Jon Maloy]

> -----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

Re: [Patch net v2] tipc: fix a null pointer deref on error path

[Cong Wang]

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().

Re: [Patch net v2] tipc: fix a null pointer deref on error path

[Jon Maloy]

> -----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
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [Patch net v2] tipc: fix a null pointer deref on error path

[David Miller]

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.