CVE-2023-1382: fix two race issues in tipc_conn_alloc

* [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 [email protected] Signed-off-by: Xin Long [email protected]

---

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 [email protected]

---

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 [email protected]

^ 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 [email protected]:

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).