From: Antonio Quartulli <[email protected]> ovpn-dco shared a single netlink socket for both synchronous request/reply transactions (peer create/delete/get, key operations, stats) and the asynchronous multicast notification group (peer del/float, key swap). Because every ovpn_nl_msg_send() drains its reply with nl_recvmsgs() on that same socket, a notification queued by the kernel could be dispatched re-entrantly in the middle of an unrelated request/reply: the NL_CB_VALID handler runs ovpn_handle_msg() -> multi_process_incoming_dco() -> multi_close_instance() while the caller is still walking an instance collection. For example multi_print_status() refreshes stats with dco_get_peer_stats_multi() and then iterates m->hash, and multi_delete_dup() iterates m->instances[] while closing duplicates; an instance freed underneath either iterator becomes a use-after-free and the server crashes (observed under mass simultaneous client reconnects).
Subscribe the multicast group on a dedicated socket (nl_sock_notify) and read it only from dco_read_and_process(), the top-level event-loop point where closing an instance is safe. The request/reply socket is no longer a member of the group, so parsing a command or stats reply can never dispatch a notification. The event loop now monitors the notification socket; the request/reply socket keeps being drained synchronously by ovpn_nl_msg_send(). Change-Id: Ia7894fc360b12c5a0f63fad2abb9bbb1ad9c2bd5 GitHub: closes OpenVPN/openvpn#1067 Signed-off-by: Antonio Quartulli <[email protected]> Acked-by: Razvan Cojocaru <[email protected]> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1737 --- This change was reviewed on Gerrit and approved by at least one developer. I request to merge it to master. Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1737 This mail reflects revision 1 of this Change. Acked-by according to Gerrit (reflected above): Razvan Cojocaru <[email protected]> diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 40746bd..7a0f620 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -134,12 +134,8 @@ } static int -ovpn_nl_recvmsgs(dco_context_t *dco, const char *prefix) +ovpn_nl_recvmsgs_report(int ret, const char *prefix) { - __is_locked = true; - int ret = nl_recvmsgs(dco->nl_sock, dco->nl_cb); - __is_locked = false; - switch (ret) { case -NLE_INTR: @@ -174,6 +170,16 @@ return ret; } +static int +ovpn_nl_recvmsgs(dco_context_t *dco, const char *prefix) +{ + __is_locked = true; + int ret = nl_recvmsgs(dco->nl_sock, dco->nl_cb); + __is_locked = false; + + return ovpn_nl_recvmsgs_report(ret, prefix); +} + /** * Send a prepared netlink message. * @@ -398,9 +404,11 @@ } /* Register for ovpn-dco specific multicast messages that the kernel may - * send + * send. These are subscribed on the dedicated notification socket only, so + * that they are never delivered while a request/reply transaction on + * dco->nl_sock is being parsed. */ - int ret = nl_socket_add_membership(dco->nl_sock, dco->ovpn_dco_mcast_id); + int ret = nl_socket_add_membership(dco->nl_sock_notify, dco->ovpn_dco_mcast_id); if (ret) { msg(M_FATAL, "%s: failed to join groups: %d", __func__, ret); @@ -449,17 +457,49 @@ nl_cb_set(dco->nl_cb, NL_CB_ACK, NL_CB_CUSTOM, ovpn_nl_cb_finish, &dco->status); nl_cb_set(dco->nl_cb, NL_CB_VALID, NL_CB_CUSTOM, ovpn_handle_msg, dco); + /* Set up the dedicated multicast notification socket. It shares the message + * handler (ovpn_handle_msg) with the request/reply socket, but is read only + * from dco_read_and_process() in the event loop, so notifications never + * fire while a request/reply transaction is being parsed. */ + dco->nl_sock_notify = nl_socket_alloc(); + if (!dco->nl_sock_notify) + { + msg(M_FATAL, "Cannot create netlink notification socket"); + } + + ret = genl_connect(dco->nl_sock_notify); + if (ret) + { + msg(M_FATAL, "Cannot connect to generic netlink (notify): %s", nl_geterror(ret)); + } + + set_cloexec(nl_socket_get_fd(dco->nl_sock_notify)); + set_nonblock(nl_socket_get_fd(dco->nl_sock_notify)); + + dco->nl_cb_notify = nl_cb_alloc(NL_CB_DEFAULT); + if (!dco->nl_cb_notify) + { + msg(M_FATAL, "failed to allocate netlink notification callback"); + } + + nl_socket_set_cb(dco->nl_sock_notify, dco->nl_cb_notify); + nl_cb_err(dco->nl_cb_notify, NL_CB_CUSTOM, ovpn_nl_cb_error, &dco->status); + nl_cb_set(dco->nl_cb_notify, NL_CB_VALID, NL_CB_CUSTOM, ovpn_handle_msg, dco); + ovpn_dco_register(dco); /* The async PACKET messages confuse libnl and it will drop them with * wrong sequence numbers (NLE_SEQ_MISMATCH), so disable libnl's sequence - * number check */ + * number check. Multicast notifications are unsolicited and likewise carry + * no matching sequence number, so disable the check on both sockets. */ nl_socket_disable_seq_check(dco->nl_sock); + nl_socket_disable_seq_check(dco->nl_sock_notify); /* nl library sets the buffer size to 32k/32k by default which is sometimes * overrun with very fast connecting/disconnecting clients. * TODO: fix this in a better and more reliable way */ ASSERT(!nl_socket_set_buffer_size(dco->nl_sock, 1024 * 1024, 1024 * 1024)); + ASSERT(!nl_socket_set_buffer_size(dco->nl_sock_notify, 1024 * 1024, 1024 * 1024)); } bool @@ -495,8 +535,12 @@ nl_socket_free(dco->nl_sock); dco->nl_sock = NULL; + nl_socket_free(dco->nl_sock_notify); + dco->nl_sock_notify = NULL; + /* Decrease reference count */ nl_cb_put(dco->nl_cb); + nl_cb_put(dco->nl_cb_notify); CLEAR(dco); } @@ -1164,7 +1208,13 @@ { msg(D_DCO_DEBUG, __func__); - return ovpn_nl_recvmsgs(dco, __func__); + /* Drain the multicast notification socket. This is the only place where + * asynchronous notifications (peer del/float, key swap) are processed, so + * the multi_close_instance() they may trigger happens at a safe point in + * the event loop rather than re-entrantly inside a request/reply. */ + int ret = nl_recvmsgs(dco->nl_sock_notify, dco->nl_cb_notify); + + return ovpn_nl_recvmsgs_report(ret, __func__); } static int @@ -1326,9 +1376,12 @@ void dco_event_set(dco_context_t *dco, struct event_set *es, void *arg) { - if (dco && dco->nl_sock) + if (dco && dco->nl_sock_notify) { - event_ctl(es, nl_socket_get_fd(dco->nl_sock), EVENT_READ, arg); + /* Only the notification socket is monitored by the event loop. The + * request/reply socket (dco->nl_sock) is drained synchronously by + * ovpn_nl_msg_send(). */ + event_ctl(es, nl_socket_get_fd(dco->nl_sock_notify), EVENT_READ, arg); } } diff --git a/src/openvpn/dco_linux.h b/src/openvpn/dco_linux.h index e3e4824..c487581 100644 --- a/src/openvpn/dco_linux.h +++ b/src/openvpn/dco_linux.h @@ -62,8 +62,18 @@ typedef struct { + /* socket used for synchronous request/reply transactions (commands and + * stats queries). It is NOT subscribed to the multicast group, so its + * recvmsgs never dispatch an asynchronous notification. */ struct nl_sock *nl_sock; struct nl_cb *nl_cb; + /* socket dedicated to multicast notifications (peer del/float, key swap). + * It is read only at the top-level dco_read_and_process() point, where it + * is safe to close instances. Keeping it separate from nl_sock prevents + * notifications from being processed re-entrantly while a request/reply is + * in flight. */ + struct nl_sock *nl_sock_notify; + struct nl_cb *nl_cb_notify; int status; struct context *c; _______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
