Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-2139-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 26 Feb 2024 13:22:01 +0800
Precedence: bulk
Mime-Version: 1.0
Message-ID: <20240226132201.Bluez.1.I8d368be0c7f86e8a999fccc33f8c9742b405bcea@changeid>
Subject: [Bluez PATCH] advertising: Fix assigning mgmt callback id when adding advertisement
From: Archie Pusaka <apusaka@google.com>
To: linux-bluetooth <linux-bluetooth@vger.kernel.org>, 
	Luiz Augusto von Dentz <luiz.dentz@gmail.com>, Johan Hedberg <johan.hedberg@gmail.com>, 
	Marcel Holtmann <marcel@holtmann.org>
Cc: CrosBT Upstreaming <chromeos-bluetooth-upstreaming@chromium.org>, 
	Archie Pusaka <apusaka@chromium.org>, Hsin-chen Chuang <chharry@google.com>
Content-Type: text/plain; charset="UTF-8"

From: Archie Pusaka <apusaka@chromium.org>

A struct member add_adv_id is used to track whether the adv client is
still needed for some mgmt callback. This is checked when freeing the
client to avoid UAF. We currently only set this member if we have a
callback after calling mgmt_send.

In case of extended advertisement, this is always a two-step process:
first to set the params, then the data. It is possible for the client
to be freed when we are pending on setting the params, and if we don't
set the add_adv_id (because we have no callback for setting the data),
the client on the 2nd step of the process will be invalid, leading to
UAF scenario.

This patch always sets the add_adv_id member on the 1st step of adding
an extended advertisement, and adjust the value accordingly on the 2nd
step. Additionally, this patch drops the 3rd parameter of the function
refresh_advertisement since it can always be derived from the 1st and
2nd parameter.

Reviewed-by: Hsin-chen Chuang <chharry@google.com>
---

 src/advertising.c | 37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/src/advertising.c b/src/advertising.c
index 2c9a5a4438..b85dbcb504 100644
--- a/src/advertising.c
+++ b/src/advertising.c
@@ -888,8 +888,7 @@ static int get_adv_flags(struct btd_adv_client *client)
 }
 
 static int refresh_legacy_adv(struct btd_adv_client *client,
-				mgmt_request_func_t func,
-				unsigned int *mgmt_id)
+				mgmt_request_func_t func)
 {
 	struct mgmt_cp_add_advertising *cp;
 	uint8_t param_len;
@@ -950,8 +949,8 @@ static int refresh_legacy_adv(struct btd_adv_client *client,
 		free(cp);
 		return -EINVAL;
 	}
-	if (mgmt_id)
-		*mgmt_id = mgmt_ret;
+	if (func)
+		client->add_adv_id = mgmt_ret;
 
 	free(cp);
 
@@ -962,7 +961,7 @@ static void add_adv_params_callback(uint8_t status, uint16_t length,
 				    const void *param, void *user_data);
 
 static int refresh_extended_adv(struct btd_adv_client *client,
-				mgmt_request_func_t func, unsigned int *mgmt_id)
+				mgmt_request_func_t func)
 {
 	struct mgmt_cp_add_ext_adv_params cp;
 	uint32_t flags = 0;
@@ -1015,21 +1014,18 @@ static int refresh_extended_adv(struct btd_adv_client *client,
 
 	/* Store callback, called after we set advertising data */
 	client->refresh_done_func = func;
-
-	if (mgmt_id)
-		*mgmt_id = mgmt_ret;
-
+	client->add_adv_id = mgmt_ret;
 
 	return 0;
 }
 
 static int refresh_advertisement(struct btd_adv_client *client,
-			mgmt_request_func_t func, unsigned int *mgmt_id)
+					mgmt_request_func_t func)
 {
 	if (client->manager->extended_add_cmds)
-		return refresh_extended_adv(client, func, mgmt_id);
+		return refresh_extended_adv(client, func);
 
-	return refresh_legacy_adv(client, func, mgmt_id);
+	return refresh_legacy_adv(client, func);
 }
 
 static bool client_discoverable_timeout(void *user_data)
@@ -1042,7 +1038,7 @@ static bool client_discoverable_timeout(void *user_data)
 
 	bt_ad_clear_flags(client->data);
 
-	refresh_advertisement(client, NULL, NULL);
+	refresh_advertisement(client, NULL);
 
 	return FALSE;
 }
@@ -1250,7 +1246,7 @@ static void properties_changed(GDBusProxy *proxy, const char *name,
 			continue;
 
 		if (parser->func(iter, client)) {
-			refresh_advertisement(client, NULL, NULL);
+			refresh_advertisement(client, NULL);
 
 			break;
 		}
@@ -1329,6 +1325,8 @@ static void add_adv_params_callback(uint8_t status, uint16_t length,
 	unsigned int mgmt_ret;
 	dbus_int16_t tx_power;
 
+	client->add_adv_id = 0;
+
 	if (status)
 		goto fail;
 
@@ -1391,6 +1389,9 @@ static void add_adv_params_callback(uint8_t status, uint16_t length,
 			     client->manager->mgmt_index, param_len, cp,
 			     client->refresh_done_func, client, NULL);
 
+	if (mgmt_ret && client->refresh_done_func)
+		client->add_adv_id = mgmt_ret;
+
 	/* Clear the callback */
 	client->refresh_done_func = NULL;
 
@@ -1399,9 +1400,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length,
 		goto fail;
 	}
 
-	if (client->add_adv_id)
-		client->add_adv_id = mgmt_ret;
-
 	free(cp);
 	cp = NULL;
 
@@ -1483,8 +1481,7 @@ static DBusMessage *parse_advertisement(struct btd_adv_client *client)
 		goto fail;
 	}
 
-	err = refresh_advertisement(client, add_adv_callback,
-					&client->add_adv_id);
+	err = refresh_advertisement(client, add_adv_callback);
 
 	if (!err)
 		return NULL;
@@ -2017,7 +2014,7 @@ static void manager_refresh(void *data, void *user_data)
 {
 	struct btd_adv_client *client = data;
 
-	refresh_advertisement(client, user_data, NULL);
+	refresh_advertisement(client, NULL);
 }
 
 void btd_adv_manager_refresh(struct btd_adv_manager *manager)
-- 
2.44.0.rc1.240.g4c46232300-goog