[PATCH net] tipc: fix a null-ptr-deref in tipc_topsrv_accept

[PATCH net] tipc: fix a null-ptr-deref in tipc_topsrv_accept

[Xin Long]

syzbot found a crash in tipc_topsrv_accept:

  KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
  Workqueue: tipc_rcv tipc_topsrv_accept
  RIP: 0010:kernel_accept+0x22d/0x350 net/socket.c:3487
  Call Trace:
   <TASK>
   tipc_topsrv_accept+0x197/0x280 net/tipc/topsrv.c:460
   process_one_work+0x991/0x1610 kernel/workqueue.c:2289
   worker_thread+0x665/0x1080 kernel/workqueue.c:2436
   kthread+0x2e4/0x3a0 kernel/kthread.c:376
   ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306

It was caused by srv->listener that might be set to null by
tipc_topsrv_stop() in net .exit whereas it's still used in
tipc_topsrv_accept() worker.

srv->listener is protected by srv->idr_lock in tipc_topsrv_stop(), so add
a check for srv->listener under srv->idr_lock in tipc_topsrv_accept() to
avoid the null-ptr-deref. To ensure the lsock is not released during the
tipc_topsrv_accept(), move sock_release() after tipc_topsrv_work_stop()
where it's waiting until the tipc_topsrv_accept worker to be done.

Note that sk_callback_lock is used to protect sk->sk_user_data instead of
srv->listener, and it should check srv in tipc_topsrv_listener_data_ready()
instead. This also ensures that no more tipc_topsrv_accept worker will be
started after tipc_conn_close() is called in tipc_topsrv_stop() where it
sets sk->sk_user_data to null.

Fixes: 0ef897be12b8 ("tipc: separate topology server listener socket from subcsriber sockets")
Reported-by: syzbot+c5ce866a8d30f4be0651@syzkaller.appspotmail.com
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/tipc/topsrv.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index 14fd05fd6107..d92ec92f0b71 100644
--- a/net/tipc/topsrv.c
+++ b/net/tipc/topsrv.c
@@ -450,12 +450,19 @@ static void tipc_conn_data_ready(struct sock *sk)
 static void tipc_topsrv_accept(struct work_struct *work)
 {
 	struct tipc_topsrv *srv = container_of(work, struct tipc_topsrv, awork);
-	struct socket *lsock = srv->listener;
-	struct socket *newsock;
+	struct socket *newsock, *lsock;
 	struct tipc_conn *con;
 	struct sock *newsk;
 	int ret;
 
+	spin_lock_bh(&srv->idr_lock);
+	if (!srv->listener) {
+		spin_unlock_bh(&srv->idr_lock);
+		return;
+	}
+	lsock = srv->listener;
+	spin_unlock_bh(&srv->idr_lock);
+
 	while (1) {
 		ret = kernel_accept(lsock, &newsock, O_NONBLOCK);
 		if (ret < 0)
@@ -489,7 +496,7 @@ static void tipc_topsrv_listener_data_ready(struct sock *sk)
 
 	read_lock_bh(&sk->sk_callback_lock);
 	srv = sk->sk_user_data;
-	if (srv->listener)
+	if (srv)
 		queue_work(srv->rcv_wq, &srv->awork);
 	read_unlock_bh(&sk->sk_callback_lock);
 }
@@ -699,8 +706,9 @@ static void tipc_topsrv_stop(struct net *net)
 	__module_get(lsock->sk->sk_prot_creator->owner);
 	srv->listener = NULL;
 	spin_unlock_bh(&srv->idr_lock);
-	sock_release(lsock);
+
 	tipc_topsrv_work_stop(srv);
+	sock_release(lsock);
 	idr_destroy(&srv->conn_idr);
 	kfree(srv);
 }
-- 
2.31.1

Re: [PATCH net] tipc: fix a null-ptr-deref in tipc_topsrv_accept

[Jon Maloy]

On 10/18/22 15:19, Xin Long wrote:
> syzbot found a crash in tipc_topsrv_accept:
>
>    KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
>    Workqueue: tipc_rcv tipc_topsrv_accept
>    RIP: 0010:kernel_accept+0x22d/0x350 net/socket.c:3487
>    Call Trace:
>     <TASK>
>     tipc_topsrv_accept+0x197/0x280 net/tipc/topsrv.c:460
>     process_one_work+0x991/0x1610 kernel/workqueue.c:2289
>     worker_thread+0x665/0x1080 kernel/workqueue.c:2436
>     kthread+0x2e4/0x3a0 kernel/kthread.c:376
>     ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
>
> It was caused by srv->listener that might be set to null by
> tipc_topsrv_stop() in net .exit whereas it's still used in
> tipc_topsrv_accept() worker.
>
> srv->listener is protected by srv->idr_lock in tipc_topsrv_stop(), so add
> a check for srv->listener under srv->idr_lock in tipc_topsrv_accept() to
> avoid the null-ptr-deref. To ensure the lsock is not released during the
> tipc_topsrv_accept(), move sock_release() after tipc_topsrv_work_stop()
> where it's waiting until the tipc_topsrv_accept worker to be done.
>
> Note that sk_callback_lock is used to protect sk->sk_user_data instead of
> srv->listener, and it should check srv in tipc_topsrv_listener_data_ready()
> instead. This also ensures that no more tipc_topsrv_accept worker will be
> started after tipc_conn_close() is called in tipc_topsrv_stop() where it
> sets sk->sk_user_data to null.
>
> Fixes: 0ef897be12b8 ("tipc: separate topology server listener socket from subcsriber sockets")
> Reported-by: syzbot+c5ce866a8d30f4be0651@syzkaller.appspotmail.com
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>   net/tipc/topsrv.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index 14fd05fd6107..d92ec92f0b71 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c
> @@ -450,12 +450,19 @@ static void tipc_conn_data_ready(struct sock *sk)
>   static void tipc_topsrv_accept(struct work_struct *work)
>   {
>   	struct tipc_topsrv *srv = container_of(work, struct tipc_topsrv, awork);
> -	struct socket *lsock = srv->listener;
> -	struct socket *newsock;
> +	struct socket *newsock, *lsock;
>   	struct tipc_conn *con;
>   	struct sock *newsk;
>   	int ret;
>   
> +	spin_lock_bh(&srv->idr_lock);
> +	if (!srv->listener) {
> +		spin_unlock_bh(&srv->idr_lock);
> +		return;
> +	}
> +	lsock = srv->listener;
> +	spin_unlock_bh(&srv->idr_lock);
> +
>   	while (1) {
>   		ret = kernel_accept(lsock, &newsock, O_NONBLOCK);
>   		if (ret < 0)
> @@ -489,7 +496,7 @@ static void tipc_topsrv_listener_data_ready(struct sock *sk)
>   
>   	read_lock_bh(&sk->sk_callback_lock);
>   	srv = sk->sk_user_data;
> -	if (srv->listener)
> +	if (srv)
>   		queue_work(srv->rcv_wq, &srv->awork);
>   	read_unlock_bh(&sk->sk_callback_lock);
>   }
> @@ -699,8 +706,9 @@ static void tipc_topsrv_stop(struct net *net)
>   	__module_get(lsock->sk->sk_prot_creator->owner);
>   	srv->listener = NULL;
>   	spin_unlock_bh(&srv->idr_lock);
> -	sock_release(lsock);
> +
>   	tipc_topsrv_work_stop(srv);
> +	sock_release(lsock);
>   	idr_destroy(&srv->conn_idr);
>   	kfree(srv);
>   }
Acked-by: Jon Maloy <jmaloy@redhat.com>

Re: [PATCH net] tipc: fix a null-ptr-deref in tipc_topsrv_accept

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 18 Oct 2022 15:19:50 -0400 you wrote:
> syzbot found a crash in tipc_topsrv_accept:
> 
>   KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
>   Workqueue: tipc_rcv tipc_topsrv_accept
>   RIP: 0010:kernel_accept+0x22d/0x350 net/socket.c:3487
>   Call Trace:
>    <TASK>
>    tipc_topsrv_accept+0x197/0x280 net/tipc/topsrv.c:460
>    process_one_work+0x991/0x1610 kernel/workqueue.c:2289
>    worker_thread+0x665/0x1080 kernel/workqueue.c:2436
>    kthread+0x2e4/0x3a0 kernel/kthread.c:376
>    ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
> 
> [...]

Here is the summary with links:
  - [net] tipc: fix a null-ptr-deref in tipc_topsrv_accept
    https://git.kernel.org/netdev/net/c/82cb4e4612c6

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html