From: Archie Pusaka <[email protected]>
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 <[email protected]>
---
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
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
Hello:
This patch was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <[email protected]>:
On Mon, 26 Feb 2024 13:22:01 +0800 you wrote:
> From: Archie Pusaka <[email protected]>
>
> 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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html