| Message ID | 4eee264380c409c61c6451af1059b7fb271a7e7b.1666120790.git.lucien.xin@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 82cb4e4612c633a9ce320e1773114875604a3cce |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] tipc: fix a null-ptr-deref in tipc_topsrv_accept | expand |
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>
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!
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); }
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(-)