Skip to content

Bluetooth: TMAP: Add role validation for bt_tmap_register #81279

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kartben merged 1 commit into from
Dec 27, 2024
Merged

Bluetooth: TMAP: Add role validation for bt_tmap_register #81279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions subsys/bluetooth/audio/Kconfig.tmap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,25 @@
#
# SPDX-License-Identifier: Apache-2.0
#
config BT_TMAP_CG_SUPPORTED
def_bool (BT_CAP_INITIATOR && BT_CAP_COMMANDER && BT_BAP_UNICAST_CLIENT && BT_AUDIO_RX && \
BT_AUDIO_TX && BT_VCP_VOL_CTLR && BT_TBS)

config BT_TMAP_CT_SUPPORTED
def_bool BT_CAP_ACCEPTOR && BT_BAP_UNICAST_SERVER

config BT_TMAP_UMS_SUPPORTED
def_bool (BT_CAP_INITIATOR && BT_CAP_COMMANDER && BT_BAP_UNICAST_CLIENT && BT_AUDIO_TX && \
BT_VCP_VOL_CTLR && BT_MCS)

config BT_TMAP_UMR_SUPPORTED
def_bool BT_CAP_ACCEPTOR && BT_BAP_UNICAST_SERVER && BT_VCP_VOL_REND

config BT_TMAP_BMS_SUPPORTED
def_bool BT_CAP_INITIATOR && BT_BAP_BROADCAST_SOURCE

config BT_TMAP_BMR_SUPPORTED
def_bool BT_CAP_ACCEPTOR && BT_BAP_BROADCAST_SINK && BT_VCP_VOL_REND

config BT_TMAP
bool "Telephony and Media Audio Profile"
Expand Down
58 changes: 57 additions & 1 deletion subsys/bluetooth/audio/tmap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/*
* Copyright 2023 NXP
* Copyright (c) 2024 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand All @@ -22,14 +23,18 @@
#include <zephyr/kernel.h>
#include <zephyr/logging/log.h>
#include <zephyr/sys/byteorder.h>
#include <zephyr/sys/check.h>
#include <zephyr/sys/util_macro.h>
#include <zephyr/types.h>

#include "audio_internal.h"

LOG_MODULE_REGISTER(bt_tmap, CONFIG_BT_TMAP_LOG_LEVEL);

/* Hex value if all TMAP role bits are set */
#define TMAP_ALL_ROLES 0x3F
#define TMAP_ALL_ROLES \
(BT_TMAP_ROLE_CG | BT_TMAP_ROLE_CT | BT_TMAP_ROLE_UMS | BT_TMAP_ROLE_UMR | \
BT_TMAP_ROLE_BMS | BT_TMAP_ROLE_BMR)

static uint16_t tmap_role;
static const struct bt_tmap_cb *cb;
Expand Down Expand Up @@ -155,10 +160,61 @@ static ssize_t read_role(struct bt_conn *conn,
static struct bt_gatt_attr svc_attrs[] = { BT_TMAS_SERVICE_DEFINITION };
static struct bt_gatt_service tmas;

static bool valid_tmap_role(enum bt_tmap_role role)
{
if (role == 0 || (role & TMAP_ALL_ROLES) != role) {
LOG_DBG("Invalid role %d", role);
}

if ((role & BT_TMAP_ROLE_CG) != 0 && !IS_ENABLED(CONFIG_BT_TMAP_CG_SUPPORTED)) {
LOG_DBG("Device does not support the CG role");

return false;
}

if ((role & BT_TMAP_ROLE_CT) != 0 && !IS_ENABLED(CONFIG_BT_TMAP_CT_SUPPORTED)) {
LOG_DBG("Device does not support the CT role");

return false;
}

if ((role & BT_TMAP_ROLE_UMS) != 0 && !IS_ENABLED(CONFIG_BT_TMAP_UMS_SUPPORTED)) {
LOG_DBG("Device does not support the UMS role");

return false;
}

if ((role & BT_TMAP_ROLE_UMR) != 0 && !IS_ENABLED(CONFIG_BT_TMAP_UMR_SUPPORTED)) {
LOG_DBG("Device does not support the UMR role");

return false;
}

if ((role & BT_TMAP_ROLE_BMS) != 0 && !IS_ENABLED(CONFIG_BT_TMAP_BMS_SUPPORTED)) {
LOG_DBG("Device does not support the BMS role");

return false;
}

if ((role & BT_TMAP_ROLE_BMR) != 0 && !IS_ENABLED(CONFIG_BT_TMAP_BMR_SUPPORTED)) {
LOG_DBG("Device does not support the BMR role");

return false;
}

return true;
}

int bt_tmap_register(enum bt_tmap_role role)
{
int err;

CHECKIF(!valid_tmap_role(role)) {
Copy link

@kruithofa kruithofa Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not specific for this PR, but also shows up other places where CHECKIF is used, and will be compiler dependent:
If the KCONFIG option NO_RUNTIME_CHECKS is set to 'y' a compiler may emit the following warning: static bool valid_tmap_role defined but not used

suggestions for fixing this?

Copy link
Contributor Author

@Thalley Thalley Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two ways we can apply is to guard this call with an IS_ENABLED, or instead of having CHECKIF in the call, we can change all the checks in the functions to use CHECKIF to that if NO_RUNTIME_CHECKS then the function is just a no-op.

But you have a good point that we should do that here

Copy link
Contributor Author

@Thalley Thalley Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the macro it is defined as

#elif defined(CONFIG_NO_RUNTIME_CHECKS)
#define CHECKIF(...) \
	if (0)
#else

With if (0) the code is still compiled, but not linked, and should thus not have any issues. Will leave as is

Copy link

@kruithofa kruithofa Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the code compiles and works fine as is, the only issue is that we get a compiler warning, that shouldn't be there.

LOG_DBG("Invalid role: %d", role);

return -EINVAL;
}

tmas = (struct bt_gatt_service)BT_GATT_SERVICE(svc_attrs);

err = bt_gatt_service_register(&tmas);
Expand Down
Loading