diff mbox series

[net] tipc: change to check tipc_own_id to return in tipc_net_stop

Message ID 81fb8639e3850b5daa1c6300c618299929d493d6.1553359702.git.lucien.xin@gmail.com
State Accepted
Delegated to: David Miller
Headers show
Series [net] tipc: change to check tipc_own_id to return in tipc_net_stop | expand

Commit Message

Xin Long March 23, 2019, 4:48 p.m. UTC
When running a syz script, a panic occurred:

[  156.088228] BUG: KASAN: use-after-free in tipc_disc_timeout+0x9c9/0xb20 [tipc]
[  156.094315] Call Trace:
[  156.094844]  <IRQ>
[  156.095306]  dump_stack+0x7c/0xc0
[  156.097346]  print_address_description+0x65/0x22e
[  156.100445]  kasan_report.cold.3+0x37/0x7a
[  156.102402]  tipc_disc_timeout+0x9c9/0xb20 [tipc]
[  156.106517]  call_timer_fn+0x19a/0x610
[  156.112749]  run_timer_softirq+0xb51/0x1090

It was caused by the netns freed without deleting the discoverer timer,
while later on the netns would be accessed in the timer handler.

The timer should have been deleted by tipc_net_stop() when cleaning up a
netns. However, tipc has been able to enable a bearer and start d->timer
without the local node_addr set since Commit 52dfae5c85a4 ("tipc: obtain
node identity from interface by default"), which caused the timer not to
be deleted in tipc_net_stop() then.

So fix it in tipc_net_stop() by changing to check local node_id instead
of local node_addr, as Jon suggested.

While at it, remove the calling of tipc_nametbl_withdraw() there, since
tipc_nametbl_stop() will take of the nametbl's freeing after.

Fixes: 52dfae5c85a4 ("tipc: obtain node identity from interface by default")
Reported-by: syzbot+a25307ad099309f1c2b9@syzkaller.appspotmail.com
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/tipc/net.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Comments

Xue, Ying March 24, 2019, 7:32 a.m. UTC | #1
On 3/24/19 12:48 AM, Xin Long wrote:
> When running a syz script, a panic occurred:
> 
> [  156.088228] BUG: KASAN: use-after-free in tipc_disc_timeout+0x9c9/0xb20 [tipc]
> [  156.094315] Call Trace:
> [  156.094844]  <IRQ>
> [  156.095306]  dump_stack+0x7c/0xc0
> [  156.097346]  print_address_description+0x65/0x22e
> [  156.100445]  kasan_report.cold.3+0x37/0x7a
> [  156.102402]  tipc_disc_timeout+0x9c9/0xb20 [tipc]
> [  156.106517]  call_timer_fn+0x19a/0x610
> [  156.112749]  run_timer_softirq+0xb51/0x1090
> 
> It was caused by the netns freed without deleting the discoverer timer,
> while later on the netns would be accessed in the timer handler.
> 
> The timer should have been deleted by tipc_net_stop() when cleaning up a
> netns. However, tipc has been able to enable a bearer and start d->timer
> without the local node_addr set since Commit 52dfae5c85a4 ("tipc: obtain
> node identity from interface by default"), which caused the timer not to
> be deleted in tipc_net_stop() then.
> 
> So fix it in tipc_net_stop() by changing to check local node_id instead
> of local node_addr, as Jon suggested.
> 
> While at it, remove the calling of tipc_nametbl_withdraw() there, since
> tipc_nametbl_stop() will take of the nametbl's freeing after.
> 
> Fixes: 52dfae5c85a4 ("tipc: obtain node identity from interface by default")
> Reported-by: syzbot+a25307ad099309f1c2b9@syzkaller.appspotmail.com
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Acked-by: Ying Xue <ying.xue@windriver.com>

> ---
>  net/tipc/net.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/net/tipc/net.c b/net/tipc/net.c
> index f076edb..7ce1e86 100644
> --- a/net/tipc/net.c
> +++ b/net/tipc/net.c
> @@ -163,12 +163,9 @@ void tipc_sched_net_finalize(struct net *net, u32 addr)
>  
>  void tipc_net_stop(struct net *net)
>  {
> -	u32 self = tipc_own_addr(net);
> -
> -	if (!self)
> +	if (!tipc_own_id(net))
>  		return;
>  
> -	tipc_nametbl_withdraw(net, TIPC_CFG_SRV, self, self, self);
>  	rtnl_lock();
>  	tipc_bearer_stop(net);
>  	tipc_node_stop(net);
>
Jon Maloy March 25, 2019, 4:24 p.m. UTC | #2
Acked-by: Jon Maloy <jon.maloy@ericsson.com>

> -----Original Message-----
> From: Xin Long <lucien.xin@gmail.com>
> Sent: 23-Mar-19 17:48
> To: network dev <netdev@vger.kernel.org>
> Cc: davem@davemloft.net; Jon Maloy <jon.maloy@ericsson.com>; Ying Xue
> <ying.xue@windriver.com>; tipc-discussion@lists.sourceforge.net;
> syzkaller@googlegroups.com
> Subject: [PATCH net] tipc: change to check tipc_own_id to return in
> tipc_net_stop
> 
> When running a syz script, a panic occurred:
> 
> [  156.088228] BUG: KASAN: use-after-free in tipc_disc_timeout+0x9c9/0xb20
> [tipc] [  156.094315] Call Trace:
> [  156.094844]  <IRQ>
> [  156.095306]  dump_stack+0x7c/0xc0
> [  156.097346]  print_address_description+0x65/0x22e
> [  156.100445]  kasan_report.cold.3+0x37/0x7a [  156.102402]
> tipc_disc_timeout+0x9c9/0xb20 [tipc] [  156.106517]
> call_timer_fn+0x19a/0x610 [  156.112749]  run_timer_softirq+0xb51/0x1090
> 
> It was caused by the netns freed without deleting the discoverer timer, while
> later on the netns would be accessed in the timer handler.
> 
> The timer should have been deleted by tipc_net_stop() when cleaning up a
> netns. However, tipc has been able to enable a bearer and start d->timer
> without the local node_addr set since Commit 52dfae5c85a4 ("tipc: obtain
> node identity from interface by default"), which caused the timer not to be
> deleted in tipc_net_stop() then.
> 
> So fix it in tipc_net_stop() by changing to check local node_id instead of local
> node_addr, as Jon suggested.
> 
> While at it, remove the calling of tipc_nametbl_withdraw() there, since
> tipc_nametbl_stop() will take of the nametbl's freeing after.
> 
> Fixes: 52dfae5c85a4 ("tipc: obtain node identity from interface by default")
> Reported-by: syzbot+a25307ad099309f1c2b9@syzkaller.appspotmail.com
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/tipc/net.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/net/tipc/net.c b/net/tipc/net.c index f076edb..7ce1e86 100644
> --- a/net/tipc/net.c
> +++ b/net/tipc/net.c
> @@ -163,12 +163,9 @@ void tipc_sched_net_finalize(struct net *net, u32
> addr)
> 
>  void tipc_net_stop(struct net *net)
>  {
> -	u32 self = tipc_own_addr(net);
> -
> -	if (!self)
> +	if (!tipc_own_id(net))
>  		return;
> 
> -	tipc_nametbl_withdraw(net, TIPC_CFG_SRV, self, self, self);
>  	rtnl_lock();
>  	tipc_bearer_stop(net);
>  	tipc_node_stop(net);
> --
> 2.1.0
David Miller March 26, 2019, 6:21 p.m. UTC | #3
From: Xin Long <lucien.xin@gmail.com>
Date: Sun, 24 Mar 2019 00:48:22 +0800

> When running a syz script, a panic occurred:
 ...
> It was caused by the netns freed without deleting the discoverer timer,
> while later on the netns would be accessed in the timer handler.
> 
> The timer should have been deleted by tipc_net_stop() when cleaning up a
> netns. However, tipc has been able to enable a bearer and start d->timer
> without the local node_addr set since Commit 52dfae5c85a4 ("tipc: obtain
> node identity from interface by default"), which caused the timer not to
> be deleted in tipc_net_stop() then.
> 
> So fix it in tipc_net_stop() by changing to check local node_id instead
> of local node_addr, as Jon suggested.
> 
> While at it, remove the calling of tipc_nametbl_withdraw() there, since
> tipc_nametbl_stop() will take of the nametbl's freeing after.
> 
> Fixes: 52dfae5c85a4 ("tipc: obtain node identity from interface by default")
> Reported-by: syzbot+a25307ad099309f1c2b9@syzkaller.appspotmail.com
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Applied and queued up for -stable, anks Xin!
diff mbox series

Patch

diff --git a/net/tipc/net.c b/net/tipc/net.c
index f076edb..7ce1e86 100644
--- a/net/tipc/net.c
+++ b/net/tipc/net.c
@@ -163,12 +163,9 @@  void tipc_sched_net_finalize(struct net *net, u32 addr)
 
 void tipc_net_stop(struct net *net)
 {
-	u32 self = tipc_own_addr(net);
-
-	if (!self)
+	if (!tipc_own_id(net))
 		return;
 
-	tipc_nametbl_withdraw(net, TIPC_CFG_SRV, self, self, self);
 	rtnl_lock();
 	tipc_bearer_stop(net);
 	tipc_node_stop(net);