* [PATCH net 0/2] tipc: fix two race issues in tipc_conn_alloc
@ 2022-11-18 21:44 Xin Long
2022-11-18 21:45 ` [PATCH net 1/2] tipc: set con sock " Xin Long
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Xin Long @ 2022-11-18 21:44 UTC (permalink / raw)
To: network dev, tipc-discussion
Cc: davem, kuba, Eric Dumazet, Paolo Abeni, Jon Maloy, Ying Xue, Wei Chen
The race exists beteen tipc_topsrv_accept() and tipc_conn_close(),
one is allocating the con while the other is freeing it and there
is no proper lock protecting it. Therefore, a null-pointer-defer
and a use-after-free may be triggered, see details on each patch.
Xin Long (2):
tipc: set con sock in tipc_conn_alloc
tipc: add an extra conn_get in tipc_conn_alloc
net/tipc/topsrv.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH net 1/2] tipc: set con sock in tipc_conn_alloc
2022-11-18 21:44 [PATCH net 0/2] tipc: fix two race issues in tipc_conn_alloc Xin Long
@ 2022-11-18 21:45 ` Xin Long
2022-11-18 21:45 ` [PATCH net 2/2] tipc: add an extra conn_get " Xin Long
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Xin Long @ 2022-11-18 21:45 UTC (permalink / raw)
To: network dev, tipc-discussion
Cc: davem, kuba, Eric Dumazet, Paolo Abeni, Jon Maloy, Ying Xue, Wei Chen
A crash was reported by Wei Chen:
BUG: kernel NULL pointer dereference, address: 0000000000000018
RIP: 0010:tipc_conn_close+0x12/0x100
Call Trace:
tipc_topsrv_exit_net+0x139/0x320
ops_exit_list.isra.9+0x49/0x80
cleanup_net+0x31a/0x540
process_one_work+0x3fa/0x9f0
worker_thread+0x42/0x5c0
It was caused by !con->sock in tipc_conn_close(). In tipc_topsrv_accept(),
con is allocated in conn_idr then its sock is set:
con = tipc_conn_alloc();
... <----[1]
con->sock = newsock;
If tipc_conn_close() is called in anytime of [1], the null-pointer-def
is triggered by con->sock->sk due to con->sock is not yet set.
This patch fixes it by moving the con->sock setting to tipc_conn_alloc()
under s->idr_lock. So that con->sock can never be NULL when getting the
con from s->conn_idr. It will be also safer to move con->server and flag
CF_CONNECTED setting under s->idr_lock, as they should all be set before
tipc_conn_alloc() is called.
Fixes: c5fa7b3cf3cb ("tipc: introduce new TIPC server infrastructure")
Reported-by: Wei Chen <harperchen1110@gmail.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/tipc/topsrv.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index d92ec92f0b71..b0f9aa521670 100644
--- a/net/tipc/topsrv.c
+++ b/net/tipc/topsrv.c
@@ -176,7 +176,7 @@ static void tipc_conn_close(struct tipc_conn *con)
conn_put(con);
}
-static struct tipc_conn *tipc_conn_alloc(struct tipc_topsrv *s)
+static struct tipc_conn *tipc_conn_alloc(struct tipc_topsrv *s, struct socket *sock)
{
struct tipc_conn *con;
int ret;
@@ -202,10 +202,11 @@ static struct tipc_conn *tipc_conn_alloc(struct tipc_topsrv *s)
}
con->conid = ret;
s->idr_in_use++;
- spin_unlock_bh(&s->idr_lock);
set_bit(CF_CONNECTED, &con->flags);
con->server = s;
+ con->sock = sock;
+ spin_unlock_bh(&s->idr_lock);
return con;
}
@@ -467,7 +468,7 @@ static void tipc_topsrv_accept(struct work_struct *work)
ret = kernel_accept(lsock, &newsock, O_NONBLOCK);
if (ret < 0)
return;
- con = tipc_conn_alloc(srv);
+ con = tipc_conn_alloc(srv, newsock);
if (IS_ERR(con)) {
ret = PTR_ERR(con);
sock_release(newsock);
@@ -479,7 +480,6 @@ static void tipc_topsrv_accept(struct work_struct *work)
newsk->sk_data_ready = tipc_conn_data_ready;
newsk->sk_write_space = tipc_conn_write_space;
newsk->sk_user_data = con;
- con->sock = newsock;
write_unlock_bh(&newsk->sk_callback_lock);
/* Wake up receive process in case of 'SYN+' message */
@@ -577,12 +577,11 @@ bool tipc_topsrv_kern_subscr(struct net *net, u32 port, u32 type, u32 lower,
sub.filter = filter;
*(u64 *)&sub.usr_handle = (u64)port;
- con = tipc_conn_alloc(tipc_topsrv(net));
+ con = tipc_conn_alloc(tipc_topsrv(net), NULL);
if (IS_ERR(con))
return false;
*conid = con->conid;
- con->sock = NULL;
rc = tipc_conn_rcv_sub(tipc_topsrv(net), con, &sub);
if (rc >= 0)
return true;
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH net 2/2] tipc: add an extra conn_get in tipc_conn_alloc
2022-11-18 21:44 [PATCH net 0/2] tipc: fix two race issues in tipc_conn_alloc Xin Long
2022-11-18 21:45 ` [PATCH net 1/2] tipc: set con sock " Xin Long
@ 2022-11-18 21:45 ` Xin Long
2022-11-22 0:47 ` [PATCH net 0/2] tipc: fix two race issues " Jon Maloy
2022-11-22 5:00 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 5+ messages in thread
From: Xin Long @ 2022-11-18 21:45 UTC (permalink / raw)
To: network dev, tipc-discussion
Cc: davem, kuba, Eric Dumazet, Paolo Abeni, Jon Maloy, Ying Xue, Wei Chen
One extra conn_get() is needed in tipc_conn_alloc(), as after
tipc_conn_alloc() is called, tipc_conn_close() may free this
con before deferencing it in tipc_topsrv_accept():
tipc_conn_alloc();
newsk = newsock->sk;
<---- tipc_conn_close();
write_lock_bh(&sk->sk_callback_lock);
newsk->sk_data_ready = tipc_conn_data_ready;
Then an uaf issue can be triggered:
BUG: KASAN: use-after-free in tipc_topsrv_accept+0x1e7/0x370 [tipc]
Call Trace:
<TASK>
dump_stack_lvl+0x33/0x46
print_report+0x178/0x4b0
kasan_report+0x8c/0x100
kasan_check_range+0x179/0x1e0
tipc_topsrv_accept+0x1e7/0x370 [tipc]
process_one_work+0x6a3/0x1030
worker_thread+0x8a/0xdf0
This patch fixes it by holding it in tipc_conn_alloc(), then after
all accessing in tipc_topsrv_accept() releasing it. Note when does
this in tipc_topsrv_kern_subscr(), as tipc_conn_rcv_sub() returns
0 or -1 only, we don't need to check for "> 0".
Fixes: c5fa7b3cf3cb ("tipc: introduce new TIPC server infrastructure")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/tipc/topsrv.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index b0f9aa521670..e3b427a70398 100644
--- a/net/tipc/topsrv.c
+++ b/net/tipc/topsrv.c
@@ -206,6 +206,7 @@ static struct tipc_conn *tipc_conn_alloc(struct tipc_topsrv *s, struct socket *s
set_bit(CF_CONNECTED, &con->flags);
con->server = s;
con->sock = sock;
+ conn_get(con);
spin_unlock_bh(&s->idr_lock);
return con;
@@ -484,6 +485,7 @@ static void tipc_topsrv_accept(struct work_struct *work)
/* Wake up receive process in case of 'SYN+' message */
newsk->sk_data_ready(newsk);
+ conn_put(con);
}
}
@@ -583,10 +585,11 @@ bool tipc_topsrv_kern_subscr(struct net *net, u32 port, u32 type, u32 lower,
*conid = con->conid;
rc = tipc_conn_rcv_sub(tipc_topsrv(net), con, &sub);
- if (rc >= 0)
- return true;
+ if (rc)
+ conn_put(con);
+
conn_put(con);
- return false;
+ return !rc;
}
void tipc_topsrv_kern_unsubscr(struct net *net, int conid)
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net 0/2] tipc: fix two race issues in tipc_conn_alloc
2022-11-18 21:44 [PATCH net 0/2] tipc: fix two race issues in tipc_conn_alloc Xin Long
2022-11-18 21:45 ` [PATCH net 1/2] tipc: set con sock " Xin Long
2022-11-18 21:45 ` [PATCH net 2/2] tipc: add an extra conn_get " Xin Long
@ 2022-11-22 0:47 ` Jon Maloy
2022-11-22 5:00 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 5+ messages in thread
From: Jon Maloy @ 2022-11-22 0:47 UTC (permalink / raw)
To: Xin Long, network dev, tipc-discussion
Cc: davem, kuba, Eric Dumazet, Paolo Abeni, Ying Xue, Wei Chen
On 11/18/22 16:44, Xin Long wrote:
> The race exists beteen tipc_topsrv_accept() and tipc_conn_close(),
> one is allocating the con while the other is freeing it and there
> is no proper lock protecting it. Therefore, a null-pointer-defer
> and a use-after-free may be triggered, see details on each patch.
>
> Xin Long (2):
> tipc: set con sock in tipc_conn_alloc
> tipc: add an extra conn_get in tipc_conn_alloc
>
> net/tipc/topsrv.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
Series
Acked-by: Jon Maloy <jmaloy@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net 0/2] tipc: fix two race issues in tipc_conn_alloc
2022-11-18 21:44 [PATCH net 0/2] tipc: fix two race issues in tipc_conn_alloc Xin Long
` (2 preceding siblings ...)
2022-11-22 0:47 ` [PATCH net 0/2] tipc: fix two race issues " Jon Maloy
@ 2022-11-22 5:00 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-11-22 5:00 UTC (permalink / raw)
To: Xin Long
Cc: netdev, tipc-discussion, davem, kuba, edumazet, pabeni, jmaloy,
ying.xue, harperchen1110
Hello:
This series was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:
On Fri, 18 Nov 2022 16:44:59 -0500 you wrote:
> The race exists beteen tipc_topsrv_accept() and tipc_conn_close(),
> one is allocating the con while the other is freeing it and there
> is no proper lock protecting it. Therefore, a null-pointer-defer
> and a use-after-free may be triggered, see details on each patch.
>
> Xin Long (2):
> tipc: set con sock in tipc_conn_alloc
> tipc: add an extra conn_get in tipc_conn_alloc
>
> [...]
Here is the summary with links:
- [net,1/2] tipc: set con sock in tipc_conn_alloc
https://git.kernel.org/netdev/net/c/0e5d56c64afc
- [net,2/2] tipc: add an extra conn_get in tipc_conn_alloc
https://git.kernel.org/netdev/net/c/a7b42969d63f
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-11-22 5:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-18 21:44 [PATCH net 0/2] tipc: fix two race issues in tipc_conn_alloc Xin Long
2022-11-18 21:45 ` [PATCH net 1/2] tipc: set con sock " Xin Long
2022-11-18 21:45 ` [PATCH net 2/2] tipc: add an extra conn_get " Xin Long
2022-11-22 0:47 ` [PATCH net 0/2] tipc: fix two race issues " Jon Maloy
2022-11-22 5:00 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).