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

Subsystems: networking [general], the rest, tipc network layer

STALE3235d

11 messages, 3 authors, 2017-12-05 · open the first message on its own page

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

From: Cong Wang <hidden>
Date: 2017-12-04 18:31:54

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 <redacted>
Cc: Jon Maloy <redacted>
Cc: Ying Xue <redacted>
Signed-off-by: Cong Wang <redacted>
---
 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

From: David Miller <davem@davemloft.net>
Date: 2017-12-04 18:57:18

From: Cong Wang <redacted>
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 <redacted>
Cc: Jon Maloy <redacted>
Cc: Ying Xue <redacted>
Signed-off-by: Cong Wang <redacted>

...

quoted hunk

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

From: Cong Wang <hidden>
Date: 2017-12-04 19:23:34

On Mon, Dec 4, 2017 at 10:57 AM, David Miller [off-list ref] wrote:

From: Cong Wang <redacted>
Date: Mon,  4 Dec 2017 10:31:43 -0800

quoted

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:

...

quoted

Fixes: 14c04493cb77 ("tipc: add ability to order and receive topology events in driver")
Reported-by: syzbot <redacted>
Cc: Jon Maloy <redacted>
Cc: Ying Xue <redacted>
Signed-off-by: Cong Wang <redacted>

...

quoted

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

From: Cong Wang <hidden>
Date: 2017-12-04 19:41:36

On Mon, Dec 4, 2017 at 11:23 AM, Cong Wang [off-list ref] wrote:

On Mon, Dec 4, 2017 at 10:57 AM, David Miller [off-list ref] wrote:

quoted

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

From: Jon Maloy <hidden>
Date: 2017-12-04 19:44:28

-----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 [off-list ref]; Ying Xue [off-list ref]
Subject: Re: [Patch net v2] tipc: fix a null pointer deref on error path

From: Cong Wang <redacted>
Date: Mon,  4 Dec 2017 10:31:43 -0800

quoted

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:

...

quoted

Fixes: 14c04493cb77 ("tipc: add ability to order and receive topology
events in driver")
Reported-by: syzbot <redacted>
Cc: Jon Maloy <redacted>
Cc: Ying Xue <redacted>
Signed-off-by: Cong Wang <redacted>

...

quoted

@@ -511,7 +511,7 @@ bool tipc_topsrv_kern_subscr(struct net *net, u32

port, u32 type,

quoted

 	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

From: David Miller <davem@davemloft.net>
Date: 2017-12-04 19:48:05

From: Cong Wang <redacted>
Date: Mon, 4 Dec 2017 11:23:13 -0800

On Mon, Dec 4, 2017 at 10:57 AM, David Miller [off-list ref] wrote:

quoted

From: Cong Wang <redacted>
Date: Mon,  4 Dec 2017 10:31:43 -0800

quoted

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:

...

quoted

Fixes: 14c04493cb77 ("tipc: add ability to order and receive topology events in driver")
Reported-by: syzbot <redacted>
Cc: Jon Maloy <redacted>
Cc: Ying Xue <redacted>
Signed-off-by: Cong Wang <redacted>

...

quoted

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

From: Jon Maloy <hidden>
Date: 2017-12-04 19:49:58

-----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 <redacted>; tipc-
discussion@lists.sourceforge.net; Jon Maloy [off-list ref];
Ying Xue [off-list ref]
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 [off-list ref]
wrote:

quoted

On Mon, Dec 4, 2017 at 10:57 AM, David Miller [off-list ref]

wrote:

quoted

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

From: Jon Maloy <hidden>
Date: 2017-12-04 20:32:59

-----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 <redacted>; David Miller
[off-list ref]
Cc: Linux Kernel Network Developers <redacted>; tipc-
discussion@lists.sourceforge.net; Ying Xue [off-list ref]
Subject: RE: [Patch net v2] tipc: fix a null pointer deref on error path

quoted

-----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 <redacted>; tipc-
discussion@lists.sourceforge.net; Jon Maloy [off-list ref];
Ying Xue [off-list ref]
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

[off-list ref]

quoted

wrote:

quoted

On Mon, Dec 4, 2017 at 10:57 AM, David Miller [off-list ref]

wrote:

quoted

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

From: Cong Wang <hidden>
Date: 2017-12-04 22:34:48

On Mon, Dec 4, 2017 at 12:32 PM, Jon Maloy [off-list ref] wrote:

quoted

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

From: Jon Maloy <hidden>
Date: 2017-12-05 01:56:48

-----Original Message-----
From: Cong Wang [mailto:xiyou.wangcong@gmail.com]
Sent: Monday, December 04, 2017 17:34
To: Jon Maloy <redacted>
Cc: David Miller <davem@davemloft.net>; Linux Kernel Network Developers
[off-list ref]; tipc-discussion@lists.sourceforge.net; Ying Xue
[off-list ref]
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 [off-list ref]
wrote:

quoted

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.

quoted

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

From: David Miller <davem@davemloft.net>
Date: 2017-12-05 19:53:41

From: Cong Wang <redacted>
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 <redacted>
Cc: Jon Maloy <redacted>
Cc: Ying Xue <redacted>
Signed-off-by: Cong Wang <redacted>

Applied and queued up for -stable.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help