From 2362721e6b7f044279bd947d95dd50025c205d58 Mon Sep 17 00:00:00 2001 From: Thomas Lukaszewicz Date: Mon, 8 Jan 2024 00:36:10 +0000 Subject: [PATCH] Mitigate UAF crashes due to iteration over freed wl_resources Currently it is possible to iterate over client-owned resources during client destruction that have had their associated memory released. This can occur when client code calls wl_client_destroy(). The following sequence illustrates how this may occur. 1. The server initiates destruction of the connected client via call to wl_client_destroy(). 2. Resource destroy listeners / destructors are invoked and resource memory is freed one resource at a time [1]. 3. If a listener / destructor for a resource results in a call to wl_client_for_each_resource(), the iteration will proceed over resources that have been previously freed in step 2, resulting in UAFs / crashes. The issue is that resources remain in the client's object map even after they have had their memory freed, and are removed from the map only after each individual resource has had its memory released. This patch corrects this by ensuring resource destruction first invokes listeners / destructors and then removing them from the client's object map before releasing the associated memory. [1] https://gitlab.freedesktop.org/wayland/wayland/-/blob/main/src/wayland-server.c?ref_type=heads#L928 Change-Id: Idb48ea9d4ea5edf8572185436f15138bab4f27f3 Signed-off-by: Thomas Lukaszewicz thomaslukaszewicz@gmail.com --- src/wayland-server.c | 64 ++++++++++++++++++++++++++++---------------------- tests/resources-test.c | 57 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 28 deletions(-) diff --git a/src/wayland-server.c b/src/wayland-server.c index 4603770..a953ddd 100644 --- a/src/wayland-server.c +++ b/src/wayland-server.c @@ -907,10 +907,23 @@ resource_is_deprecated(struct wl_resource *resource) return ret; } +/** Removes the wl_resource from the client's object map and deletes it. + * + * Triggers the destroy signal and destructor for the resource before + * removing it from the client's object map and releasing the resource's + * memory. + * + * This order is important to ensure listeners and destruction code can + * find the resource before it has been destroyed whilst ensuring the + * resource is not accessible via the object map after memory has been + * freed. + */ static enum wl_iterator_result -destroy_resource(void *element, void *data, uint32_t flags) +remove_and_destroy_resource(void *element, void *data, uint32_t flags) { struct wl_resource *resource = element; + struct wl_client *client = resource->client; + uint32_t id = resource->object.id;; wl_signal_emit(&resource->deprecated_destroy_signal, resource); /* Don't emit the new signal for deprecated resources, as that would @@ -921,30 +934,7 @@ destroy_resource(void *element, void *data, uint32_t flags) if (resource->destroy) resource->destroy(resource); - if (!(flags & WL_MAP_ENTRY_LEGACY)) - free(resource); - - return WL_ITERATOR_CONTINUE; -} - -WL_EXPORT void -wl_resource_destroy(struct wl_resource *resource) -{ - struct wl_client *client = resource->client; - uint32_t id; - uint32_t flags; - - id = resource->object.id; - -//TIZEN_ONLY(20230518) : Add mutex for wl_client's resources - pthread_mutex_lock(&client->objects_mutex); -//TIZEN_ONLY : END - flags = wl_map_lookup_flags(&client->objects, id); -//TIZEN_ONLY(20230518) : Add mutex for wl_client's resources - pthread_mutex_unlock(&client->objects_mutex); -//TIZEN_ONLY : END - destroy_resource(resource, NULL, flags); - + /* The resource should be cleared from the map before memory is freed. */ if (id < WL_SERVER_ID_START) { if (client->display_resource) { wl_resource_queue_event(client->display_resource, @@ -966,6 +956,26 @@ wl_resource_destroy(struct wl_resource *resource) pthread_mutex_unlock(&client->objects_mutex); //TIZEN_ONLY : END } + + if (!(flags & WL_MAP_ENTRY_LEGACY)) + free(resource); + + return WL_ITERATOR_CONTINUE; +} + +WL_EXPORT void +wl_resource_destroy(struct wl_resource *resource) +{ + struct wl_client *client = resource->client; +//TIZEN_ONLY(20230518) : Add mutex for wl_client's resources + pthread_mutex_lock(&client->objects_mutex); +//TIZEN_ONLY : END + uint32_t flags = wl_map_lookup_flags(&client->objects, resource->object.id); +//TIZEN_ONLY(20230518) : Add mutex for wl_client's resources + pthread_mutex_unlock(&client->objects_mutex); +//TIZEN_ONLY : END + + remove_and_destroy_resource(resource, NULL, flags); } WL_EXPORT uint32_t @@ -1098,12 +1108,10 @@ wl_client_get_destroy_listener(struct wl_client *client, WL_EXPORT void wl_client_destroy(struct wl_client *client) { - uint32_t serial = 0; - wl_priv_signal_final_emit(&client->destroy_signal, client); wl_client_flush(client); - wl_map_for_each(&client->objects, destroy_resource, &serial); + wl_map_for_each(&client->objects, remove_and_destroy_resource, NULL); wl_map_release(&client->objects); wl_event_source_remove(client->source); close(wl_connection_destroy(client->connection)); diff --git a/tests/resources-test.c b/tests/resources-test.c index 30a6d26..0d4eecf 100644 --- a/tests/resources-test.c +++ b/tests/resources-test.c @@ -208,3 +208,60 @@ TEST(free_without_remove) assert(a.link.next == a.link.prev && a.link.next == NULL); assert(b.link.next == b.link.prev && b.link.next == NULL); } + +static enum wl_iterator_result +client_resource_check(struct wl_resource* resource, void* data) +{ + /* Ensure there is no iteration over already freed resources. */ + assert(!wl_resource_get_user_data(resource)); + return WL_ITERATOR_CONTINUE; +} + +static void +resource_destroy_notify(struct wl_listener *l, void *data) +{ + struct wl_resource* resource = data; + struct wl_client* client = resource->client; + + wl_client_for_each_resource(client, client_resource_check, NULL); + + /* Set user data to flag the resource has been deleted. The resource should + * not be accessible from this point forward. */ + wl_resource_set_user_data(resource, client); +} + +TEST(resource_destroy_iteration) +{ + struct wl_display *display; + struct wl_client *client; + struct wl_resource *resource1; + struct wl_resource *resource2; + int s[2]; + + struct wl_listener destroy_listener1 = { + .notify = &resource_destroy_notify + }; + struct wl_listener destroy_listener2 = { + .notify = &resource_destroy_notify + }; + + 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); + resource1 = wl_resource_create(client, &wl_callback_interface, 1, 0); + resource2 = wl_resource_create(client, &wl_callback_interface, 1, 0); + assert(resource1); + assert(resource2); + + wl_resource_add_destroy_listener(resource1, &destroy_listener1); + wl_resource_add_destroy_listener(resource2, &destroy_listener2); + + wl_client_destroy(client); + + close(s[0]); + close(s[1]); + + wl_display_destroy(display); +} -- 2.7.4