Skip to content

Commit

Permalink
net: conn: Improve thread safety in connection module
Browse files Browse the repository at this point in the history
Iterating over connection list w/o mutex lock could lead to a crash on
constant incoming packet flow. Fix this by:

1. Adding mutex lock when iterating over an active connection list, to
   prevent list corruption.
2. Create a copy of the callback and user data pointers before releasing
   lock, to prevent NULL pointer dereference in case connection is
   released before callback is executed.

Signed-off-by: Robert Lubos <[email protected]>
  • Loading branch information
rlubos authored and fabiobaltieri committed Dec 6, 2023
1 parent 4ab2dde commit 5f6b447
Showing 1 changed file with 17 additions and 4 deletions.
21 changes: 17 additions & 4 deletions subsys/net/ip/connection.c
Original file line number Diff line number Diff line change
Expand Up @@ -640,6 +640,8 @@ enum net_verdict net_conn_input(struct net_pkt *pkt,
bool raw_pkt_delivered = false;
bool raw_pkt_continue = false;
struct net_conn *conn;
net_conn_cb_t cb = NULL;
void *user_data = NULL;

if (IS_ENABLED(CONFIG_NET_IP)) {
/* If we receive a packet with multicast destination address, we might
Expand All @@ -657,6 +659,8 @@ enum net_verdict net_conn_input(struct net_pkt *pkt,
}
}

k_mutex_lock(&conn_lock, K_FOREVER);

SYS_SLIST_FOR_EACH_CONTAINER(&conn_used, conn, node) {
/* Is the candidate connection matching the packet's interface? */
if (conn->context != NULL &&
Expand Down Expand Up @@ -731,6 +735,7 @@ enum net_verdict net_conn_input(struct net_pkt *pkt,
enum net_verdict ret = conn_raw_socket(pkt, conn, proto);

if (ret == NET_DROP) {
k_mutex_unlock(&conn_lock);
goto drop;
} else if (ret == NET_OK) {
raw_pkt_delivered = true;
Expand Down Expand Up @@ -805,6 +810,7 @@ enum net_verdict net_conn_input(struct net_pkt *pkt,

mcast_pkt = net_pkt_clone(pkt, CLONE_TIMEOUT);
if (!mcast_pkt) {
k_mutex_unlock(&conn_lock);
goto drop;
}

Expand All @@ -823,6 +829,13 @@ enum net_verdict net_conn_input(struct net_pkt *pkt,
}
} /* loop end */

if (best_match) {
cb = best_match->cb;
user_data = best_match->user_data;
}

k_mutex_unlock(&conn_lock);

if (IS_ENABLED(CONFIG_NET_SOCKETS_PACKET) && pkt_family == AF_PACKET) {
if (raw_pkt_continue) {
/* When there is open connection different than
Expand Down Expand Up @@ -850,11 +863,11 @@ enum net_verdict net_conn_input(struct net_pkt *pkt,
return NET_OK;
}

if (best_match) {
NET_DBG("[%p] match found cb %p ud %p rank 0x%02x", best_match, best_match->cb,
best_match->user_data, NET_CONN_RANK(best_match->flags));
if (cb) {
NET_DBG("[%p] match found cb %p ud %p rank 0x%02x", best_match, cb,
user_data, NET_CONN_RANK(best_match->flags));

if (best_match->cb(best_match, pkt, ip_hdr, proto_hdr, best_match->user_data)
if (cb(best_match, pkt, ip_hdr, proto_hdr, user_data)
== NET_DROP) {
goto drop;
}
Expand Down

0 comments on commit 5f6b447

Please sign in to comment.