From ab1667ac8181f5cd7593cc4fbc380cacfd71883b Mon Sep 17 00:00:00 2001 From: Ivan Iushkov Date: Tue, 6 Feb 2024 13:08:30 +0100 Subject: [PATCH 1/3] [nrf fromtree] Bluetooth: Fixing UBSAN warning in CTE field parsing in adv.c/scan.c during local testling, UBSAN reported the following warnings: - bluetooth/host/adv.c:2067:19: runtime error: shift exponent 255 is too large for 32-bit type 'long unsigned int' - bluetooth/host/scan.c:828:18: runtime error: shift exponent 255 is too large for 32-bit type 'long unsigned int' It turned out that we can't use BIT() macro directly on bt_hci_evt_le_per_advertising_report::cte_type field. According to Core Spec, `cte_type = 0xFF` corresponds to `No contstant tone extension`. Added separate function to convert CTE bit field from HCI format to bt_df_cte_type Signed-off-by: Ivan Iushkov (cherry picked from commit b1e9f86378c1c2f591aa2d6882da9a1a8ca4ac78) Signed-off-by: Ivan Iushkov --- subsys/bluetooth/host/adv.c | 2 +- subsys/bluetooth/host/hci_core.c | 16 ++++++++++++++++ subsys/bluetooth/host/hci_core.h | 9 ++++++++- subsys/bluetooth/host/scan.c | 2 +- 4 files changed, 26 insertions(+), 3 deletions(-) diff --git a/subsys/bluetooth/host/adv.c b/subsys/bluetooth/host/adv.c index 734c835a841..7a312d5e05c 100644 --- a/subsys/bluetooth/host/adv.c +++ b/subsys/bluetooth/host/adv.c @@ -2064,7 +2064,7 @@ void bt_hci_le_per_adv_response_report(struct net_buf *buf) response = net_buf_pull_mem(buf, sizeof(struct bt_hci_evt_le_per_adv_response)); info.tx_power = response->tx_power; info.rssi = response->rssi; - info.cte_type = BIT(response->cte_type); + info.cte_type = bt_get_df_cte_type(response->cte_type); info.response_slot = response->response_slot; if (buf->len < response->data_length) { diff --git a/subsys/bluetooth/host/hci_core.c b/subsys/bluetooth/host/hci_core.c index b37cd45e1ae..ee1c822d9f9 100644 --- a/subsys/bluetooth/host/hci_core.c +++ b/subsys/bluetooth/host/hci_core.c @@ -422,6 +422,22 @@ uint8_t bt_get_phy(uint8_t hci_phy) } } +int bt_get_df_cte_type(uint8_t hci_cte_type) +{ + switch (hci_cte_type) { + case BT_HCI_LE_AOA_CTE: + return BT_DF_CTE_TYPE_AOA; + case BT_HCI_LE_AOD_CTE_1US: + return BT_DF_CTE_TYPE_AOD_1US; + case BT_HCI_LE_AOD_CTE_2US: + return BT_DF_CTE_TYPE_AOD_2US; + case BT_HCI_LE_NO_CTE: + return BT_DF_CTE_TYPE_NONE; + default: + return BT_DF_CTE_TYPE_NONE; + } +} + #if defined(CONFIG_BT_CONN_TX) static void hci_num_completed_packets(struct net_buf *buf) { diff --git a/subsys/bluetooth/host/hci_core.h b/subsys/bluetooth/host/hci_core.h index ce4fc5a9b63..1a3c06414a0 100644 --- a/subsys/bluetooth/host/hci_core.h +++ b/subsys/bluetooth/host/hci_core.h @@ -434,7 +434,14 @@ int bt_le_set_data_len(struct bt_conn *conn, uint16_t tx_octets, uint16_t tx_tim int bt_le_set_phy(struct bt_conn *conn, uint8_t all_phys, uint8_t pref_tx_phy, uint8_t pref_rx_phy, uint8_t phy_opts); uint8_t bt_get_phy(uint8_t hci_phy); - +/** + * @brief Convert CTE type value from HCI format to @ref bt_df_cte_type format. + * + * @param hci_cte_type CTE type in an HCI format. + * + * @return CTE type (@ref bt_df_cte_type). + */ +int bt_get_df_cte_type(uint8_t hci_cte_type); int bt_le_scan_update(bool fast_scan); int bt_le_create_conn(const struct bt_conn *conn); diff --git a/subsys/bluetooth/host/scan.c b/subsys/bluetooth/host/scan.c index 168d7f6ead3..0b61305e2c3 100644 --- a/subsys/bluetooth/host/scan.c +++ b/subsys/bluetooth/host/scan.c @@ -825,7 +825,7 @@ static void bt_hci_le_per_adv_report_common(struct net_buf *buf) info.tx_power = evt->tx_power; info.rssi = evt->rssi; - info.cte_type = BIT(evt->cte_type); + info.cte_type = bt_get_df_cte_type(evt->cte_type); info.addr = &per_adv_sync->addr; info.sid = per_adv_sync->sid; From d558462fc8100bcd741ff6b68c570695a723343b Mon Sep 17 00:00:00 2001 From: Ivan Iushkov Date: Mon, 5 Feb 2024 16:30:36 +0100 Subject: [PATCH 2/3] [nrf fromtree] Bluetooth: fixing UBSAN warnings related to Codec Configuration During local testing with UBSAN enabled, warning was reported: bluetooth/host/iso.c:237:2: runtime error: null pointer passed as argument 2, which is declared to never be null It turned out that when datapath doesn't contain codec information, cc_len is 0 and cc is NULL In order to avoid UB, now we call memcpy only when cp->codec_config_len > 0 Signed-off-by: Ivan Iushkov (cherry picked from commit e8d090011b6fa9d7e37e05dd03eb457e801b2e97) Signed-off-by: Ivan Iushkov --- subsys/bluetooth/host/iso.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/subsys/bluetooth/host/iso.c b/subsys/bluetooth/host/iso.c index 4d6366ead67..05b5cc2f613 100644 --- a/subsys/bluetooth/host/iso.c +++ b/subsys/bluetooth/host/iso.c @@ -233,9 +233,10 @@ static int hci_le_setup_iso_data_path(const struct bt_conn *iso, uint8_t dir, cp->codec_id.vs_codec_id = sys_cpu_to_le16(path->vid); sys_put_le24(path->delay, cp->controller_delay); cp->codec_config_len = path->cc_len; - cc = net_buf_add(buf, cp->codec_config_len); - memcpy(cc, path->cc, cp->codec_config_len); - + cc = net_buf_add(buf, path->cc_len); + if (path->cc_len) { + memcpy(cc, path->cc, path->cc_len); + } err = bt_hci_cmd_send_sync(BT_HCI_OP_LE_SETUP_ISO_PATH, buf, &rsp); if (err) { return err; From 3087c5cc76c1dff97ce3924e16b1b7b12aaf18d4 Mon Sep 17 00:00:00 2001 From: Ivan Iushkov Date: Tue, 6 Feb 2024 09:34:16 +0100 Subject: [PATCH 3/3] [nrf fromtree] Bluetooth: fixing null-pointer dereference in l2cap channel destroyer During local testing with UBSAN enabled, warning was reported: bluetooth/host/l2cap.c:980:25: runtime error: member access within null pointer of type 'struct k_work_q' It turned out that le_chan->rtx_work.queue can be NULL. Since null-pointer dereference is a UB, additional check was added to ensure we don't access `le_chan->rtx_work.queue->thread` when `le_chan->rtx_work.queue == NULL` The same changes applied to l2cap_br.c Signed-off-by: Ivan Iushkov (cherry picked from commit a3cbf8e2aca97548c9a7380c76b8bf8e0161006a) Signed-off-by: Ivan Iushkov --- subsys/bluetooth/host/l2cap.c | 4 +++- subsys/bluetooth/host/l2cap_br.c | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/subsys/bluetooth/host/l2cap.c b/subsys/bluetooth/host/l2cap.c index e6fbe2144f8..b8faa1efcae 100644 --- a/subsys/bluetooth/host/l2cap.c +++ b/subsys/bluetooth/host/l2cap.c @@ -977,7 +977,9 @@ static void l2cap_chan_destroy(struct bt_l2cap_chan *chan) * In the case where we are in the context of executing the rtx_work * item, we don't sync as it will deadlock the workqueue. */ - if (k_current_get() != &le_chan->rtx_work.queue->thread) { + struct k_work_q *rtx_work_queue = le_chan->rtx_work.queue; + + if (rtx_work_queue == NULL || k_current_get() != &rtx_work_queue->thread) { k_work_cancel_delayable_sync(&le_chan->rtx_work, &le_chan->rtx_sync); } else { k_work_cancel_delayable(&le_chan->rtx_work); diff --git a/subsys/bluetooth/host/l2cap_br.c b/subsys/bluetooth/host/l2cap_br.c index e9a10ff8d72..3a33ff293c6 100644 --- a/subsys/bluetooth/host/l2cap_br.c +++ b/subsys/bluetooth/host/l2cap_br.c @@ -165,7 +165,9 @@ static void l2cap_br_chan_destroy(struct bt_l2cap_chan *chan) * In the case where we are in the context of executing the rtx_work * item, we don't sync as it will deadlock the workqueue. */ - if (k_current_get() != &br_chan->rtx_work.queue->thread) { + struct k_work_q *rtx_work_queue = br_chan->rtx_work.queue; + + if (rtx_work_queue == NULL || k_current_get() != &rtx_work_queue->thread) { k_work_cancel_delayable_sync(&br_chan->rtx_work, &br_chan->rtx_sync); } else { k_work_cancel_delayable(&br_chan->rtx_work);