2021-02-25 14:38:50

by Johannes Berg

Subject: [PATCH] wmediumd: fix use-after-free in client removal

From: Johannes Berg <[email protected]>

If a client dies while we're waiting for an ACK from it, then
wmediumd_api_handler() will call wmediumd_remove_client() and
free the client, even while we're still waiting for it to
respond in wmediumd_wait_for_client_ack(). This leads then to
wmediumd_wait_for_client_ack() using the data after it was
already freed, and we also get stuck.

Fix this by deferring the free to the outermost loop, instead
just setting wait_for_ack=false whenever we find that we have
to free a client.

All client iterations (that can call back into the loop) then
must use _safe() in order to not use the 'next' pointer after
the client was removed (and the pointer poisoned).
---
wmediumd/wmediumd.c | 22 +++++++++++++++++-----
wmediumd/wmediumd.h | 2 +-
2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/wmediumd/wmediumd.c b/wmediumd/wmediumd.c
index 5304931fbf74..3b4aed35c981 100644
--- a/wmediumd/wmediumd.c
+++ b/wmediumd/wmediumd.c
@@ -279,7 +279,7 @@ static void wmediumd_notify_frame_start(struct usfstl_job *job)
{
struct frame *frame = container_of(job, struct frame, start_job);
struct wmediumd *ctx = job->data;
- struct client *client;
+ struct client *client, *tmp;
struct {
struct wmediumd_message_header hdr;
struct wmediumd_tx_start start;
@@ -292,7 +292,7 @@ static void wmediumd_notify_frame_start(struct usfstl_job *job)
if (ctx->ctrl)
usfstl_sched_ctrl_sync_to(ctx->ctrl);

- list_for_each_entry(client, &ctx->clients, list) {
+ list_for_each_entry_safe(client, tmp, &ctx->clients, list) {
if (!(client->flags & WMEDIUMD_CTL_NOTIFY_TX_START))
continue;

@@ -583,11 +583,12 @@ static void wmediumd_remove_client(struct wmediumd *ctx, struct client *client)

if (!list_empty(&client->list))
list_del(&client->list);
+ list_add(&client->list, &ctx->clients_to_free);

if (client->flags & WMEDIUMD_CTL_NOTIFY_TX_START)
ctx->need_start_notify--;

- free(client);
+ client->wait_for_ack = false;
}

/*
@@ -638,7 +639,7 @@ static void send_cloned_frame_msg(struct wmediumd *ctx, struct client *src,
int rate_idx, int signal, int freq,
uint64_t cookie)
{
- struct client *client;
+ struct client *client, *tmp;
struct nl_msg *msg, *cmsg = NULL;

msg = nlmsg_alloc();
@@ -670,7 +671,7 @@ static void send_cloned_frame_msg(struct wmediumd *ctx, struct client *src,
if (ctx->ctrl)
usfstl_sched_ctrl_sync_to(ctx->ctrl);

- list_for_each_entry(client, &ctx->clients, list) {
+ list_for_each_entry_safe(client, tmp, &ctx->clients, list) {
if (client->flags & WMEDIUMD_CTL_RX_ALL_FRAMES) {
if (src == client && !cmsg) {
struct nlmsghdr *nlh = nlmsg_hdr(msg);
@@ -1356,6 +1357,7 @@ int main(int argc, char *argv[])

INIT_LIST_HEAD(&ctx.stations);
INIT_LIST_HEAD(&ctx.clients);
+ INIT_LIST_HEAD(&ctx.clients_to_free);

if (load_config(&ctx, config_file, per_file))
return EXIT_FAILURE;
@@ -1415,6 +1417,16 @@ int main(int argc, char *argv[])
if (usfstl_sched_next_pending(&scheduler, NULL))
usfstl_sched_next(&scheduler);
}
+
+ while (!list_empty(&ctx.clients_to_free)) {
+ struct client *client;
+
+ client = list_first_entry(&ctx.clients_to_free,
+ struct client, list);
+
+ list_del(&client->list);
+ free(client);
+ }
}

free(ctx.sock);
diff --git a/wmediumd/wmediumd.h b/wmediumd/wmediumd.h
index 8619e28cbe9a..4b7ca6628ce9 100644
--- a/wmediumd/wmediumd.h
+++ b/wmediumd/wmediumd.h
@@ -196,7 +196,7 @@ struct wmediumd {

struct usfstl_sched_ctrl *ctrl;

- struct list_head clients;
+ struct list_head clients, clients_to_free;
struct client nl_client;

int num_stas;
--
2.26.2

2021-03-04 07:25:11

by Bob Copeland

Subject: Re: [PATCH] wmediumd: fix use-after-free in client removal

On Thu, Feb 25, 2021 at 03:37:08PM +0100, Johannes Berg wrote:
> From: Johannes Berg <[email protected]>
>
> If a client dies while we're waiting for an ACK from it, then
> wmediumd_api_handler() will call wmediumd_remove_client() and
> free the client, even while we're still waiting for it to
> respond in wmediumd_wait_for_client_ack(). This leads then to
> wmediumd_wait_for_client_ack() using the data after it was
> already freed, and we also get stuck.

Applied, thanks.