diff mbox series

[Bluez] advertising: Fix assigning mgmt callback id when adding advertisement

Message ID 20240226132201.Bluez.1.I8d368be0c7f86e8a999fccc33f8c9742b405bcea@changeid (mailing list archive)
State Accepted
Commit c01c40498cfb770d4282f31edd9d75bb53646efa
Headers show
Series [Bluez] advertising: Fix assigning mgmt callback id when adding advertisement | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch success CheckPatch PASS
tedd_an/GitLint success Gitlint PASS
tedd_an/BuildEll success Build ELL PASS
tedd_an/BluezMake success Bluez Make PASS
tedd_an/MakeCheck success Bluez Make Check PASS
tedd_an/MakeDistcheck success Make Distcheck PASS
tedd_an/CheckValgrind success Check Valgrind PASS
tedd_an/CheckSmatch success CheckSparse PASS
tedd_an/bluezmakeextell success Make External ELL PASS
tedd_an/IncrementalBuild success Incremental Build PASS
tedd_an/ScanBuild success Scan Build PASS

Commit Message

Archie Pusaka Feb. 26, 2024, 5:22 a.m. UTC 
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(-)

Comments

bluez.test.bot@gmail.com Feb. 26, 2024, 6:31 a.m. UTC | #1 
This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=829696

---Test result---

Test Summary:
CheckPatch                    PASS      0.44 seconds
GitLint                       PASS      0.30 seconds
BuildEll                      PASS      24.07 seconds
BluezMake                     PASS      733.87 seconds
MakeCheck                     PASS      12.40 seconds
MakeDistcheck                 PASS      165.43 seconds
CheckValgrind                 PASS      228.77 seconds
CheckSmatch                   PASS      331.69 seconds
bluezmakeextell               PASS      108.28 seconds
IncrementalBuild              PASS      689.10 seconds
ScanBuild                     PASS      974.91 seconds



---
Regards,
Linux Bluetooth
patchwork-bot+bluetooth@kernel.org Feb. 26, 2024, 10:30 p.m. UTC | #2 
Hello:

This patch was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Mon, 26 Feb 2024 13:22:01 +0800 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [Bluez] advertising: Fix assigning mgmt callback id when adding advertisement
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=c01c40498cfb

You are awesome, thank you!
diff mbox series

Patch

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)