From 58908aa5d006ef24cfe75ef44b416dc62e9ee566 Mon Sep 17 00:00:00 2001 From: Joakim Andersson Date: Wed, 19 Feb 2020 18:48:53 +0100 Subject: [PATCH] Bluetooth: host: Fix missing connection id checks Fix issue where a new connection with the same peer would use the CCC from from first connection, despite different local identity. Since there is no CCC for the new connection yet this caused the application to think that CCC was enabled but the remote device had not yet subscribed. Fix this issue by making the id as an input to the peer address check function. This will force us to make the check every time. This commit might also fix similar issues not yes discovered as the ID check was missing in a few other places as well. Signed-off-by: Joakim Andersson --- subsys/bluetooth/host/conn.c | 28 ++++---- subsys/bluetooth/host/conn_internal.h | 8 +-- subsys/bluetooth/host/gatt.c | 94 ++++++++++++++++++--------- subsys/bluetooth/host/hci_core.c | 32 +++++---- 4 files changed, 103 insertions(+), 59 deletions(-) diff --git a/subsys/bluetooth/host/conn.c b/subsys/bluetooth/host/conn.c index 6f6f37bd455..5471dfdc6f4 100644 --- a/subsys/bluetooth/host/conn.c +++ b/subsys/bluetooth/host/conn.c @@ -1803,19 +1803,24 @@ struct bt_conn *bt_conn_lookup_handle(u16_t handle) return NULL; } -int bt_conn_addr_le_cmp(const struct bt_conn *conn, const bt_addr_le_t *peer) +bool bt_conn_is_peer_addr_le(const struct bt_conn *conn, u8_t id, + const bt_addr_le_t *peer) { + if (id != conn->id) { + return false; + } + /* Check against conn dst address as it may be the identity address */ if (!bt_addr_le_cmp(peer, &conn->le.dst)) { - return 0; + return true; } /* Check against initial connection address */ if (conn->role == BT_HCI_ROLE_MASTER) { - return bt_addr_le_cmp(peer, &conn->le.resp_addr); + return bt_addr_le_cmp(peer, &conn->le.resp_addr) == 0; } - return bt_addr_le_cmp(peer, &conn->le.init_addr); + return bt_addr_le_cmp(peer, &conn->le.init_addr) == 0; } struct bt_conn *bt_conn_lookup_addr_le(u8_t id, const bt_addr_le_t *peer) @@ -1831,8 +1836,7 @@ struct bt_conn *bt_conn_lookup_addr_le(u8_t id, const bt_addr_le_t *peer) continue; } - if (conns[i].id == id && - !bt_conn_addr_le_cmp(&conns[i], peer)) { + if (bt_conn_is_peer_addr_le(&conns[i], id, peer)) { return bt_conn_ref(&conns[i]); } } @@ -1840,7 +1844,7 @@ struct bt_conn *bt_conn_lookup_addr_le(u8_t id, const bt_addr_le_t *peer) return NULL; } -struct bt_conn *bt_conn_lookup_state_le(const bt_addr_le_t *peer, +struct bt_conn *bt_conn_lookup_state_le(u8_t id, const bt_addr_le_t *peer, const bt_conn_state_t state) { int i; @@ -1854,11 +1858,11 @@ struct bt_conn *bt_conn_lookup_state_le(const bt_addr_le_t *peer, continue; } - if (peer && bt_conn_addr_le_cmp(&conns[i], peer)) { + if (peer && !bt_conn_is_peer_addr_le(&conns[i], id, peer)) { continue; } - if (conns[i].state == state) { + if (conns[i].state == state && conns[i].id == id) { return bt_conn_ref(&conns[i]); } } @@ -2114,7 +2118,8 @@ int bt_conn_create_auto_le(const struct bt_le_conn_param *param) return -EINVAL; } - conn = bt_conn_lookup_state_le(BT_ADDR_LE_NONE, BT_CONN_CONNECT_AUTO); + conn = bt_conn_lookup_state_le(BT_ID_DEFAULT, BT_ADDR_LE_NONE, + BT_CONN_CONNECT_AUTO); if (conn) { bt_conn_unref(conn); return -EALREADY; @@ -2169,7 +2174,8 @@ int bt_conn_create_auto_stop(void) return -EINVAL; } - conn = bt_conn_lookup_state_le(BT_ADDR_LE_NONE, BT_CONN_CONNECT_AUTO); + conn = bt_conn_lookup_state_le(BT_ID_DEFAULT, BT_ADDR_LE_NONE, + BT_CONN_CONNECT_AUTO); if (!conn) { return -EINVAL; } diff --git a/subsys/bluetooth/host/conn_internal.h b/subsys/bluetooth/host/conn_internal.h index f0ac76f984a..904fe17de31 100644 --- a/subsys/bluetooth/host/conn_internal.h +++ b/subsys/bluetooth/host/conn_internal.h @@ -200,9 +200,9 @@ void bt_conn_disconnect_all(u8_t id); /* Look up an existing connection */ struct bt_conn *bt_conn_lookup_handle(u16_t handle); -/* Compare an address with bt_conn destination address */ -int bt_conn_addr_le_cmp(const struct bt_conn *conn, const bt_addr_le_t *peer); - +/* Check if the connection is with the given peer. */ +bool bt_conn_is_peer_addr_le(const struct bt_conn *conn, u8_t id, + const bt_addr_le_t *peer); /* Helpers for identifying & looking up connections based on the the index to * the connection list. This is useful for O(1) lookups, but can't be used @@ -214,7 +214,7 @@ struct bt_conn *bt_conn_lookup_index(u8_t index); /* Look up a connection state. For BT_ADDR_LE_ANY, returns the first connection * with the specific state */ -struct bt_conn *bt_conn_lookup_state_le(const bt_addr_le_t *peer, +struct bt_conn *bt_conn_lookup_state_le(u8_t id, const bt_addr_le_t *peer, const bt_conn_state_t state); /* Set connection object in certain state and perform action related to state */ diff --git a/subsys/bluetooth/host/gatt.c b/subsys/bluetooth/host/gatt.c index 813962b8d5d..9b0e2049f15 100644 --- a/subsys/bluetooth/host/gatt.c +++ b/subsys/bluetooth/host/gatt.c @@ -59,6 +59,7 @@ struct ccc_store { }; struct gatt_sub { + u8_t id; bt_addr_le_t peer; sys_slist_t list; }; @@ -430,12 +431,14 @@ static struct gatt_cf_cfg *find_cf_cfg(struct bt_conn *conn) int i; for (i = 0; i < ARRAY_SIZE(cf_cfg); i++) { + struct gatt_cf_cfg *cfg = &cf_cfg[i]; + if (!conn) { - if (!bt_addr_le_cmp(&cf_cfg[i].peer, BT_ADDR_LE_ANY)) { - return &cf_cfg[i]; + if (!bt_addr_le_cmp(&cfg->peer, BT_ADDR_LE_ANY)) { + return cfg; } - } else if (!bt_conn_addr_le_cmp(conn, &cf_cfg[i].peer)) { - return &cf_cfg[i]; + } else if (bt_conn_is_peer_addr_le(conn, cfg->id, &cfg->peer)) { + return cfg; } } @@ -518,6 +521,7 @@ static ssize_t cf_write(struct bt_conn *conn, const struct bt_gatt_attr *attr, } bt_addr_le_copy(&cfg->peer, &conn->le.dst); + cfg->id = conn->id; atomic_set_bit(cfg->flags, CF_CHANGE_AWARE); return len; @@ -1406,13 +1410,15 @@ static struct bt_gatt_ccc_cfg *find_ccc_cfg(const struct bt_conn *conn, struct _bt_gatt_ccc *ccc) { for (size_t i = 0; i < ARRAY_SIZE(ccc->cfg); i++) { + struct bt_gatt_ccc_cfg *cfg = &ccc->cfg[i]; + if (conn) { - if (conn->id == ccc->cfg[i].id && - !bt_conn_addr_le_cmp(conn, &ccc->cfg[i].peer)) { - return &ccc->cfg[i]; + if (bt_conn_is_peer_addr_le(conn, cfg->id, + &cfg->peer)) { + return cfg; } - } else if (!bt_addr_le_cmp(&ccc->cfg[i].peer, BT_ADDR_LE_ANY)) { - return &ccc->cfg[i]; + } else if (!bt_addr_le_cmp(&cfg->peer, BT_ADDR_LE_ANY)) { + return cfg; } } @@ -1729,7 +1735,7 @@ static u8_t notify_cb(const struct bt_gatt_attr *attr, void *user_data) continue; } - conn = bt_conn_lookup_state_le(&cfg->peer, + conn = bt_conn_lookup_state_le(cfg->id, &cfg->peer, BT_CONN_CONNECTED); if (!conn) { struct sc_data *sc; @@ -1740,6 +1746,7 @@ static u8_t notify_cb(const struct bt_gatt_attr *attr, void *user_data) sys_le16_to_cpu(sc->end)); continue; } + bt_conn_unref(conn); } } @@ -2054,9 +2061,11 @@ static u8_t update_ccc(const struct bt_gatt_attr *attr, void *user_data) ccc = attr->user_data; for (i = 0; i < ARRAY_SIZE(ccc->cfg); i++) { + struct bt_gatt_ccc_cfg *cfg = &ccc->cfg[i]; + /* Ignore configuration for different peer or not active */ - if (!ccc->cfg[i].value || - bt_conn_addr_le_cmp(conn, &ccc->cfg[i].peer)) { + if (!cfg->value || + !bt_conn_is_peer_addr_le(conn, cfg->id, &cfg->peer)) { continue; } @@ -2129,8 +2138,7 @@ static u8_t disconnected_cb(const struct bt_gatt_attr *attr, void *user_data) continue; } - if (conn->id != cfg->id || - bt_conn_addr_le_cmp(conn, &cfg->peer)) { + if (!bt_conn_is_peer_addr_le(conn, cfg->id, &cfg->peer)) { struct bt_conn *tmp; /* Skip if there is another peer connected */ @@ -2209,8 +2217,9 @@ bool bt_gatt_is_subscribed(struct bt_conn *conn, /* Check if the connection is subscribed */ for (size_t i = 0; i < BT_GATT_CCC_MAX; i++) { - if (conn->id == ccc->cfg[i].id && - !bt_conn_addr_le_cmp(conn, &ccc->cfg[i].peer) && + const struct bt_gatt_ccc_cfg *cfg = &ccc->cfg[i]; + + if (bt_conn_is_peer_addr_le(conn, cfg->id, &cfg->peer) && (ccc_value & ccc->cfg[i].value)) { return true; } @@ -2249,7 +2258,7 @@ static struct gatt_sub *gatt_sub_find_free(struct bt_conn *conn, for (i = 0; i < ARRAY_SIZE(subscriptions); i++) { struct gatt_sub *sub = &subscriptions[i]; - if (!bt_conn_addr_le_cmp(conn, &sub->peer)) { + if (bt_conn_is_peer_addr_le(conn, sub->id, &sub->peer)) { return sub; } else if (free_sub && !bt_addr_le_cmp(BT_ADDR_LE_ANY, &sub->peer)) { @@ -2274,6 +2283,7 @@ static struct gatt_sub *gatt_sub_add(struct bt_conn *conn) if (free_sub) { bt_addr_le_copy(&free_sub->peer, &conn->le.dst); + free_sub->id = conn->id; return free_sub; } @@ -4076,13 +4086,15 @@ void bt_gatt_disconnected(struct bt_conn *conn) #endif } -static struct gatt_cf_cfg *find_cf_cfg_by_addr(const bt_addr_le_t *addr) +static struct gatt_cf_cfg *find_cf_cfg_by_addr(u8_t id, + const bt_addr_le_t *addr) { if (IS_ENABLED(CONFIG_BT_GATT_CACHING)) { int i; for (i = 0; i < ARRAY_SIZE(cf_cfg); i++) { - if (!bt_addr_le_cmp(addr, &cf_cfg[i].peer)) { + if (id == cf_cfg[i].id && + !bt_addr_le_cmp(addr, &cf_cfg[i].peer)) { return &cf_cfg[i]; } } @@ -4262,7 +4274,9 @@ static int cf_set(const char *name, size_t len_rd, settings_read_cb read_cb, { struct gatt_cf_cfg *cfg; bt_addr_le_t addr; + const char *next; int len, err; + u8_t id; if (!name) { BT_ERR("Insufficient number of arguments"); @@ -4275,7 +4289,15 @@ static int cf_set(const char *name, size_t len_rd, settings_read_cb read_cb, return -EINVAL; } - cfg = find_cf_cfg_by_addr(&addr); + settings_name_next(name, &next); + + if (!next) { + id = BT_ID_DEFAULT; + } else { + id = strtol(next, NULL, 10); + } + + cfg = find_cf_cfg_by_addr(id, &addr); if (!cfg) { cfg = find_cf_cfg(NULL); if (!cfg) { @@ -4417,7 +4439,7 @@ static int bt_gatt_clear_cf(u8_t id, const bt_addr_le_t *addr) { struct gatt_cf_cfg *cfg; - cfg = find_cf_cfg_by_addr(addr); + cfg = find_cf_cfg_by_addr(id, addr); if (cfg) { clear_cf_cfg(cfg); } @@ -4456,20 +4478,28 @@ static int sc_clear_by_addr(u8_t id, const bt_addr_le_t *addr) return 0; } -static void bt_gatt_clear_subscriptions(const bt_addr_le_t *addr) -{ - struct gatt_sub *sub = NULL; - struct bt_gatt_subscribe_params *params, *tmp; - sys_snode_t *prev = NULL; - int i; - for (i = 0; i < ARRAY_SIZE(subscriptions); i++) { - if (!bt_addr_le_cmp(addr, &subscriptions[i].peer)) { - sub = &subscriptions[i]; - break; +static struct gatt_sub *find_gatt_sub(u8_t id, const bt_addr_le_t *addr) +{ + for (int i = 0; i < ARRAY_SIZE(subscriptions); i++) { + struct gatt_sub *sub = &subscriptions[i]; + + if (id == sub->id && + !bt_addr_le_cmp(addr, &sub->peer)) { + return sub; } } + return NULL; +} + +static void bt_gatt_clear_subscriptions(u8_t id, const bt_addr_le_t *addr) +{ + struct gatt_sub *sub; + struct bt_gatt_subscribe_params *params, *tmp; + sys_snode_t *prev = NULL; + + sub = find_gatt_sub(id, addr); if (!sub) { return; } @@ -4505,7 +4535,7 @@ int bt_gatt_clear(u8_t id, const bt_addr_le_t *addr) } if (IS_ENABLED(CONFIG_BT_GATT_CLIENT)) { - bt_gatt_clear_subscriptions(addr); + bt_gatt_clear_subscriptions(id, addr); } return 0; diff --git a/subsys/bluetooth/host/hci_core.c b/subsys/bluetooth/host/hci_core.c index d94222ffc34..e33873fdebe 100644 --- a/subsys/bluetooth/host/hci_core.c +++ b/subsys/bluetooth/host/hci_core.c @@ -596,7 +596,8 @@ static void rpa_timeout(struct k_work *work) if (IS_ENABLED(CONFIG_BT_CENTRAL)) { struct bt_conn *conn = - bt_conn_lookup_state_le(NULL, BT_CONN_CONNECT_SCAN); + bt_conn_lookup_state_le(BT_ID_DEFAULT, NULL, + BT_CONN_CONNECT_SCAN); if (conn) { bt_conn_unref(conn); @@ -1215,9 +1216,11 @@ static struct bt_conn *find_pending_connect(u8_t role, bt_addr_le_t *peer_addr) * CONNECT or DIR_ADV state associated with passed peer LE address. */ if (IS_ENABLED(CONFIG_BT_CENTRAL) && role == BT_HCI_ROLE_MASTER) { - conn = bt_conn_lookup_state_le(peer_addr, BT_CONN_CONNECT); + conn = bt_conn_lookup_state_le(BT_ID_DEFAULT, peer_addr, + BT_CONN_CONNECT); if (IS_ENABLED(CONFIG_BT_WHITELIST) && !conn) { - conn = bt_conn_lookup_state_le(BT_ADDR_LE_NONE, + conn = bt_conn_lookup_state_le(BT_ID_DEFAULT, + BT_ADDR_LE_NONE, BT_CONN_CONNECT_AUTO); } @@ -1225,10 +1228,11 @@ static struct bt_conn *find_pending_connect(u8_t role, bt_addr_le_t *peer_addr) } if (IS_ENABLED(CONFIG_BT_PERIPHERAL) && role == BT_HCI_ROLE_SLAVE) { - conn = bt_conn_lookup_state_le(peer_addr, + conn = bt_conn_lookup_state_le(bt_dev.adv_id, peer_addr, BT_CONN_CONNECT_DIR_ADV); if (!conn) { - conn = bt_conn_lookup_state_le(BT_ADDR_LE_NONE, + conn = bt_conn_lookup_state_le(bt_dev.adv_id, + BT_ADDR_LE_NONE, BT_CONN_CONNECT_ADV); } @@ -1761,7 +1765,8 @@ static void check_pending_conn(const bt_addr_le_t *id_addr, return; } - conn = bt_conn_lookup_state_le(id_addr, BT_CONN_CONNECT_SCAN); + conn = bt_conn_lookup_state_le(BT_ID_DEFAULT, id_addr, + BT_CONN_CONNECT_SCAN); if (!conn) { return; } @@ -3083,7 +3088,7 @@ void bt_id_add(struct bt_keys *keys) return; } - conn = bt_conn_lookup_state_le(NULL, BT_CONN_CONNECT); + conn = bt_conn_lookup_state_le(BT_ID_DEFAULT, NULL, BT_CONN_CONNECT); if (conn) { atomic_set_bit(bt_dev.flags, BT_DEV_ID_PENDING); keys->flags |= BT_KEYS_ID_PENDING_ADD; @@ -3190,7 +3195,7 @@ void bt_id_del(struct bt_keys *keys) return; } - conn = bt_conn_lookup_state_le(NULL, BT_CONN_CONNECT); + conn = bt_conn_lookup_state_le(BT_ID_DEFAULT, NULL, BT_CONN_CONNECT); if (conn) { atomic_set_bit(bt_dev.flags, BT_DEV_ID_PENDING); keys->flags |= BT_KEYS_ID_PENDING_DEL; @@ -3736,13 +3741,15 @@ int bt_le_scan_update(bool fast_scan) struct bt_conn *conn; /* don't restart scan if we have pending connection */ - conn = bt_conn_lookup_state_le(NULL, BT_CONN_CONNECT); + conn = bt_conn_lookup_state_le(BT_ID_DEFAULT, NULL, + BT_CONN_CONNECT); if (conn) { bt_conn_unref(conn); return 0; } - conn = bt_conn_lookup_state_le(NULL, BT_CONN_CONNECT_SCAN); + conn = bt_conn_lookup_state_le(BT_ID_DEFAULT, NULL, + BT_CONN_CONNECT_SCAN); if (!conn) { return 0; } @@ -6192,14 +6199,15 @@ int bt_le_adv_stop(void) if (IS_ENABLED(CONFIG_BT_PERIPHERAL)) { struct bt_conn *conn; - conn = bt_conn_lookup_state_le(BT_ADDR_LE_NONE, + conn = bt_conn_lookup_state_le(bt_dev.adv_id, BT_ADDR_LE_NONE, BT_CONN_CONNECT_ADV); if (conn) { bt_conn_set_state(conn, BT_CONN_DISCONNECTED); bt_conn_unref(conn); } - conn = bt_conn_lookup_state_le(NULL, BT_CONN_CONNECT_DIR_ADV); + conn = bt_conn_lookup_state_le(bt_dev.adv_id, NULL, + BT_CONN_CONNECT_DIR_ADV); if (conn) { bt_conn_set_state(conn, BT_CONN_DISCONNECTED); bt_conn_unref(conn);