From 00c080174379a31b74f9b42abf20a29effd7b09f Mon Sep 17 00:00:00 2001 From: ligd Date: Fri, 22 Sep 2023 13:33:42 +0800 Subject: [PATCH] local socket: fix accept used after free ==1729315==ERROR: AddressSanitizer: heap-use-after-free on address 0xf0501d60 at pc 0x032ffe43 bp 0xef4ed158 sp 0xef4ed148 READ of size 2 at 0xf0501d60 thread T0 #0 0x32ffe42 in nxsem_wait semaphore/sem_wait.c:94 #1 0x3548cf5 in _net_timedwait utils/net_lock.c:97 #2 0x3548f48 in net_sem_timedwait utils/net_lock.c:236 #3 0x3548f8c in net_sem_wait utils/net_lock.c:318 #4 0x350124d in local_accept local/local_accept.c:246 #5 0x3492719 in psock_accept socket/accept.c:149 #6 0x3492bcc in accept4 socket/accept.c:280 #7 0x662dc04 in accept net/lib_accept.c:50 #8 0x55c81ab in kvdb_loop kvdb/server.c:415 #9 0x55c860a in kvdbd_main kvdb/server.c:458 #10 0x33d968b in nxtask_startup sched/task_startup.c:70 #11 0x32ec039 in nxtask_start task/task_start.c:134 #12 0x34109be in pre_start sim/sim_initialstate.c:52 0xf0501d60 is located 288 bytes inside of 420-byte region [0xf0501c40,0xf0501de4) freed by thread T0 here: #0 0xf7aa6a3f in __interceptor_free ../../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:127 #1 0x73aa06e in host_free sim/posix/sim_hostmemory.c:192 #2 0x34131d6 in mm_free sim/sim_heap.c:230 #3 0x3409388 in free umm_heap/umm_free.c:49 #4 0x35631f3 in local_free local/local_conn.c:225 #5 0x3563f75 in local_release local/local_release.c:129 #6 0x34f5a32 in local_close local/local_sockif.c:785 #7 0x3496ee8 in psock_close socket/net_close.c:102 #8 0x36500bc in sock_file_close socket/socket.c:115 #9 0x3635f6c in file_close vfs/fs_close.c:74 #10 0x3632439 in nx_close_from_tcb inode/fs_files.c:670 #11 0x36324f3 in nx_close inode/fs_files.c:697 #12 0x3632557 in close inode/fs_files.c:735 #13 0x55be289 in property_set_ kvdb/client.c:210 #14 0x55c0309 in property_set_int32_ kvdb/common.c:226 #15 0x55c03f5 in property_set_int32_oneway kvdb/common.c:236 Signed-off-by: ligd --- net/local/local.h | 34 +++++++++++++++++++++++++++++ net/local/local_accept.c | 7 +++++- net/local/local_conn.c | 47 ++++++++++++++++++++++++++++++++++++++++ net/local/local_sockif.c | 32 ++++++--------------------- 4 files changed, 94 insertions(+), 26 deletions(-) diff --git a/net/local/local.h b/net/local/local.h index cb9c00d5b8649..8804c783e1559 100644 --- a/net/local/local.h +++ b/net/local/local.h @@ -223,6 +223,40 @@ FAR struct local_conn_s *local_alloc(void); void local_free(FAR struct local_conn_s *conn); +/**************************************************************************** + * Name: local_addref + * + * Description: + * Increment the reference count on the underlying connection structure. + * + * Input Parameters: + * conn - Socket structure of the socket whose reference count will be + * incremented. + * + * Returned Value: + * None + * + ****************************************************************************/ + +void local_addref(FAR struct local_conn_s *conn); + +/**************************************************************************** + * Name: local_subref + * + * Description: + * Subtract the reference count on the underlying connection structure. + * + * Input Parameters: + * conn - Socket structure of the socket whose reference count will be + * incremented. + * + * Returned Value: + * None + * + ****************************************************************************/ + +void local_subref(FAR struct local_conn_s *conn); + /**************************************************************************** * Name: local_nextconn * diff --git a/net/local/local_accept.c b/net/local/local_accept.c index df727a34d205e..9621335c812b3 100644 --- a/net/local/local_accept.c +++ b/net/local/local_accept.c @@ -144,6 +144,8 @@ int local_accept(FAR struct socket *psock, FAR struct sockaddr *addr, client = container_of(waiter, struct local_conn_s, u.client.lc_waiter); + local_addref(client); + /* Decrement the number of pending clients */ DEBUGASSERT(server->u.server.lc_pending > 0); @@ -163,7 +165,8 @@ int local_accept(FAR struct socket *psock, FAR struct sockaddr *addr, { /* Initialize the new connection structure */ - conn->lc_crefs = 1; + local_addref(conn); + conn->lc_proto = SOCK_STREAM; conn->lc_type = LOCAL_TYPE_PATHNAME; conn->lc_state = LOCAL_STATE_CONNECTED; @@ -246,6 +249,8 @@ int local_accept(FAR struct socket *psock, FAR struct sockaddr *addr, ret = net_sem_wait(&client->lc_donesem); } + local_subref(client); + return ret; } diff --git a/net/local/local_conn.c b/net/local/local_conn.c index 6d8cef151853c..7a27c40661f47 100644 --- a/net/local/local_conn.c +++ b/net/local/local_conn.c @@ -225,4 +225,51 @@ void local_free(FAR struct local_conn_s *conn) kmm_free(conn); } +/**************************************************************************** + * Name: local_addref + * + * Description: + * Increment the reference count on the underlying connection structure. + * + * Input Parameters: + * psock - Socket structure of the socket whose reference count will be + * incremented. + * + * Returned Value: + * None + * + ****************************************************************************/ + +void local_addref(FAR struct local_conn_s *conn) +{ + DEBUGASSERT(conn->lc_crefs >= 0 && conn->lc_crefs < 255); + conn->lc_crefs++; +} + +/**************************************************************************** + * Name: local_subref + * + * Description: + * Subtract the reference count on the underlying connection structure. + * + * Input Parameters: + * psock - Socket structure of the socket whose reference count will be + * incremented. + * + * Returned Value: + * None + * + ****************************************************************************/ + +void local_subref(FAR struct local_conn_s *conn) +{ + DEBUGASSERT(conn->lc_crefs > 0 && conn->lc_crefs < 255); + + conn->lc_crefs--; + if (conn->lc_crefs == 0) + { + local_release(conn); + } +} + #endif /* CONFIG_NET && CONFIG_NET_LOCAL */ diff --git a/net/local/local_sockif.c b/net/local/local_sockif.c index 500b3ee6f0259..77b5abb55e51b 100644 --- a/net/local/local_sockif.c +++ b/net/local/local_sockif.c @@ -49,7 +49,7 @@ static int local_setup(FAR struct socket *psock); static sockcaps_t local_sockcaps(FAR struct socket *psock); -static void local_addref(FAR struct socket *psock); +static void local_sockaddref(FAR struct socket *psock); static int local_bind(FAR struct socket *psock, FAR const struct sockaddr *addr, socklen_t addrlen); static int local_getsockname(FAR struct socket *psock, @@ -80,7 +80,7 @@ const struct sock_intf_s g_local_sockif = { local_setup, /* si_setup */ local_sockcaps, /* si_sockcaps */ - local_addref, /* si_addref */ + local_sockaddref, /* si_addref */ local_bind, /* si_bind */ local_getsockname, /* si_getsockname */ local_getpeername, /* si_getpeername */ @@ -137,8 +137,7 @@ static int local_sockif_alloc(FAR struct socket *psock) * count will be incremented only if the socket is dup'ed */ - DEBUGASSERT(conn->lc_crefs == 0); - conn->lc_crefs = 1; + local_addref(conn); /* Save the pre-allocated connection in the socket structure */ @@ -243,7 +242,7 @@ static sockcaps_t local_sockcaps(FAR struct socket *psock) } /**************************************************************************** - * Name: local_addref + * Name: local_sockaddref * * Description: * Increment the reference count on the underlying connection structure. @@ -257,15 +256,10 @@ static sockcaps_t local_sockcaps(FAR struct socket *psock) * ****************************************************************************/ -static void local_addref(FAR struct socket *psock) +static void local_sockaddref(FAR struct socket *psock) { - FAR struct local_conn_s *conn; - DEBUGASSERT(psock->s_domain == PF_LOCAL); - - conn = psock->s_conn; - DEBUGASSERT(conn->lc_crefs > 0 && conn->lc_crefs < 255); - conn->lc_crefs++; + local_addref(psock->s_conn); } /**************************************************************************** @@ -773,23 +767,11 @@ static int local_close(FAR struct socket *psock) #endif case SOCK_CTRL: { - FAR struct local_conn_s *conn = psock->s_conn; - /* Is this the last reference to the connection structure (there * could be more if the socket was dup'ed). */ - if (conn->lc_crefs <= 1) - { - conn->lc_crefs = 0; - local_release(conn); - } - else - { - /* No.. Just decrement the reference count */ - - conn->lc_crefs--; - } + local_subref(psock->s_conn); return OK; }