From 47de87263c88d3e23c7d7eb6d56ff75ddc8a02ef Mon Sep 17 00:00:00 2001 From: Thomas Lukaszewicz Date: Fri, 5 Jan 2024 00:50:49 +0000 Subject: Mitigate UAF crashes due to wl_client_destroy reentrancy There are situations in which a call into wl_client_destroy() can result in a reentrant call into wl_client_destroy() - which results in UAF / double free crashes. For example, this can occur in the following scenario. 1. Server receives a message notifying it that a client has disconnected (WL_EVENT_HANGUP [1]) 2. This beings client destruction with a call to wl_client_destroy() 3. wl_client_destroy() kicks off callbacks as client-associated resources are cleaned up and their destructors and destruction signals are invoked. 4. These callbacks eventually lead to an explicit call to wl_display_flush_clients() as the server attempts to flush events to other connected clients. 5. Since the client has already begun destruction, when it is reached in the iteration the flush fails wl_client_destroy() is called again [2]. This patch guards against this reentrant condition by removing the client from the display's client list when wl_client_destroy() is first called. This prevents access / iteration over the client after wl_client_destroy() is called. In the example above, wl_display_flush_clients() will pass over the client currently undergoing destruction and the reentrant call is avoided. [1] https://gitlab.freedesktop.org/wayland/wayland/-/blob/8f499bf4045f88f3a4b4b0a445befca467bebe20/src/wayland-server.c#L342 [2] https://gitlab.freedesktop.org/wayland/wayland/-/blob/8f499bf4045f88f3a4b4b0a445befca467bebe20/src/wayland-server.c#L1512 Signed-off-by: Thomas Lukaszewicz [thomaslukaszewicz@gmail.com](mailto:thomaslukaszewicz@gmail.com) --- src/wayland-server.c | 13 ++++++++++++- tests/client-test.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/src/wayland-server.c b/src/wayland-server.c index 1e079a3..78ff405 100644 --- a/src/wayland-server.c +++ b/src/wayland-server.c @@ -933,6 +933,18 @@ wl_client_get_destroy_late_listener(struct wl_client *client, WL_EXPORT void wl_client_destroy(struct wl_client *client) { + + /* wl_client_destroy() should not be called twice for the same client. */ + if (wl_list_empty(&client->link)) { + client->error = 1; + wl_log("wl_client_destroy: encountered re-entrant client destruction.\n"); + return; + } + + wl_list_remove(&client->link); + /* Keep the client link safe to inspect. */ + wl_list_init(&client->link); + wl_priv_signal_final_emit(&client->destroy_signal, client); wl_client_flush(client); @@ -943,7 +955,6 @@ wl_client_destroy(struct wl_client *client) wl_priv_signal_final_emit(&client->destroy_late_signal, client); - wl_list_remove(&client->link); wl_list_remove(&client->resource_created_signal.listener_list); if (client->data_dtor) diff --git a/tests/client-test.c b/tests/client-test.c index 659bac1..5585c0c 100644 --- a/tests/client-test.c +++ b/tests/client-test.c @@ -158,3 +158,50 @@ TEST(client_destroy_listener) wl_display_destroy(display); } +static void +client_destroy_remove_link_notify(struct wl_listener *l, void *data) +{ + struct wl_client *client = data; + struct client_destroy_listener *listener = + wl_container_of(l, listener, listener); + + /* The client destruction signal should not be emitted more than once. */ + assert(!listener->done); + listener->done = true; + + /* The client should have been removed from the display's list. */ + assert(wl_list_empty(wl_client_get_link(client))); +} + +/* + * Tests that wl_client_destroy() will remove the client from the display's + * client list to prevent client access during destruction. + */ +TEST(client_destroy_removes_link) +{ + struct wl_display *display; + struct wl_client *client; + struct client_destroy_listener destroy_listener; + int s[2]; + + assert(socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, s) == 0); + display = wl_display_create(); + assert(display); + client = wl_client_create(display, s[0]); + assert(client); + + destroy_listener.listener.notify = client_destroy_remove_link_notify; + destroy_listener.done = false; + wl_client_add_destroy_listener(client, &destroy_listener.listener); + + assert(wl_client_get_destroy_listener(client, + client_destroy_remove_link_notify) == &destroy_listener.listener); + + wl_client_destroy(client); + assert(destroy_listener.done); + + close(s[0]); + close(s[1]); + + wl_display_destroy(display); +} -- cgit v1.2.3-70-g09d2