Hi Szymon,
On Fri, Mar 23, 2018 at 2:06 PM, Szymon Janc <szymon.janc@xxxxxxxxxxx> wrote:
> This adds support for storing CCC value of Service Changed
> characteristic. Once bluetoothd is restart stored values are read
> and any device subscribed to indications will receive Service Changed
> indication with 0x00010-0xffff value. This is to invalidate any
> non-core services since there is no way to verify if applications
> will register their services in same order (or at all).
>
> This fix accessing invalid handles by stacks that rely only on Service
> Changed indication for rediscovery ie. Apple iOS.
> ---
> src/adapter.c | 3 ++
> src/device.c | 49 +++++++++++++++++++++++++
> src/device.h | 4 +++
> src/gatt-database.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++---
> src/gatt-database.h | 2 ++
> 5 files changed, 154 insertions(+), 4 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 6d7d61504..fba01df0c 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -7998,6 +7998,9 @@ load:
> clear_blocked(adapter);
> load_devices(adapter);
>
> + /* restore Service Changed CCC value for bonded devices */
> + btd_gatt_database_restore_svc_chng_ccc(adapter->database);
> +
> /* retrieve the active connections: address the scenario where
> * the are active connections before the daemon've started */
> if (adapter->current_settings & MGMT_SETTING_POWERED)
> diff --git a/src/device.c b/src/device.c
> index bf5441543..f624649ff 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -225,6 +225,9 @@ struct btd_device {
> uint16_t att_mtu; /* The ATT MTU */
> unsigned int att_disconn_id;
>
> + uint16_t le_svc_cng_ccc;
> + uint16_t br_svc_cng_ccc;
I wonder why can't we pass these directly to gatt-database.c? I mean
once you load it you could call restore_state directly instead of
iterating to each device
and calling device_get_svc_chng_ccc.
> /*
> * TODO: For now, device creates and owns the client-role gatt_db, but
> * this needs to be persisted in a more central place so that proper
> @@ -364,6 +367,17 @@ static void store_csrk(struct csrk_info *csrk, GKeyFile *key_file,
> g_key_file_set_integer(key_file, group, "Counter", csrk->counter);
> }
>
> +static void update_svc_chng_ccc(GKeyFile *file, struct btd_device *dev)
> +{
> + if (dev->bredr && dev->bredr_state.bonded)
> + g_key_file_set_integer(file, "ServiceChanged", "CCC_BR/EDR",
> + dev->br_svc_cng_ccc);
> +
> + if (dev->le && dev->le_state.bonded)
> + g_key_file_set_integer(file, "ServiceChanged", "CCC_LE",
> + dev->le_svc_cng_ccc);
> +}
> +
> static gboolean store_device_info_cb(gpointer user_data)
> {
> struct btd_device *device = user_data;
> @@ -408,6 +422,7 @@ static gboolean store_device_info_cb(gpointer user_data)
> }
>
> update_technologies(key_file, device);
> + update_svc_chng_ccc(key_file, device);
>
> g_key_file_set_boolean(key_file, "General", "Trusted",
> device->trusted);
> @@ -2991,6 +3006,16 @@ static void load_info(struct btd_device *device, const char *local,
> device->remote_csrk = load_csrk(key_file, "RemoteSignatureKey");
> }
>
> + if (device->bredr)
> + device->br_svc_cng_ccc = g_key_file_get_integer(key_file,
> + "ServiceChanged", "CCC_BR/EDR",
> + NULL);
> +
> + if (device->le)
> + device->le_svc_cng_ccc = g_key_file_get_integer(key_file,
> + "ServiceChanged", "CCC_LE",
> + NULL);
We should probably document these in the storage documentation. Also
Id like to store the core service handle range along with it so we can
compare and in case it changes a full reset shall be sent, it is not
fail proof but it should make it easier to detect if plugin register a
service which shall probably be considered a fixed service as well.
> g_strfreev(techno);
>
> next:
> @@ -5238,6 +5263,10 @@ const bdaddr_t *device_get_address(struct btd_device *device)
> {
> return &device->bdaddr;
> }
> +uint8_t device_get_le_address_type(struct btd_device *device)
> +{
> + return device->bdaddr_type;
> +}
>
> const char *device_get_path(const struct btd_device *device)
> {
> @@ -5329,6 +5358,26 @@ void device_set_legacy(struct btd_device *device, bool legacy)
> DEVICE_INTERFACE, "LegacyPairing");
> }
>
> +void device_set_svc_chng_ccc(struct btd_device *device, uint8_t addr_type,
> + uint16_t value)
> +{
> + if (addr_type == BDADDR_BREDR)
> + device->br_svc_cng_ccc = value;
> + else
> + device->le_svc_cng_ccc = value;
> +
> + if (device_is_bonded(device, addr_type))
> + store_device_info(device);
> +}
> +
> +uint16_t device_get_svc_chng_ccc(struct btd_device *device, uint8_t addr_type)
> +{
> + if (addr_type == BDADDR_BREDR)
> + return device->br_svc_cng_ccc;
> +
> + return device->le_svc_cng_ccc;
> +}
> +
> void device_set_rssi_with_delta(struct btd_device *device, int8_t rssi,
> int8_t delta_threshold)
> {
> diff --git a/src/device.h b/src/device.h
> index bc046f780..4afaa9cde 100644
> --- a/src/device.h
> +++ b/src/device.h
> @@ -87,6 +87,7 @@ void device_probe_profile(gpointer a, gpointer b);
> void device_remove_profile(gpointer a, gpointer b);
> struct btd_adapter *device_get_adapter(struct btd_device *device);
> const bdaddr_t *device_get_address(struct btd_device *device);
> +uint8_t device_get_le_address_type(struct btd_device *device);
> const char *device_get_path(const struct btd_device *device);
> gboolean device_is_temporary(struct btd_device *device);
> bool device_is_connectable(struct btd_device *device);
> @@ -131,6 +132,9 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type);
> void device_request_disconnect(struct btd_device *device, DBusMessage *msg);
> bool device_is_disconnecting(struct btd_device *device);
>
> +void device_set_svc_chng_ccc(struct btd_device *device, uint8_t badddr_type, uint16_t value);
> +uint16_t device_get_svc_chng_ccc(struct btd_device *device, uint8_t badddr_type);
> +
> typedef void (*disconnect_watch) (struct btd_device *device, gboolean removal,
> void *user_data);
>
> diff --git a/src/gatt-database.c b/src/gatt-database.c
> index 19f03c544..f97f3caf7 100644
> --- a/src/gatt-database.c
> +++ b/src/gatt-database.c
> @@ -265,7 +265,7 @@ static struct ccc_state *find_ccc_state(struct device_state *dev_state,
> }
>
> static struct device_state *device_state_create(struct btd_gatt_database *db,
> - bdaddr_t *bdaddr,
> + const bdaddr_t *bdaddr,
> uint8_t bdaddr_type)
> {
> struct device_state *dev_state;
> @@ -950,6 +950,25 @@ service_add_ccc(struct gatt_db_attribute *service,
> return ccc;
> }
>
> +static uint8_t svc_chngd_ccc_write_cb(struct bt_att *att, uint16_t value,
> + void *user_data)
> +{
> + struct btd_gatt_database *database = user_data;
> + struct btd_device * device;
> + bdaddr_t bdaddr;
> + uint8_t bdaddr_type;
> +
> + if (!get_dst_info(att, &bdaddr, &bdaddr_type))
> + return 0;
> +
> + device = btd_adapter_find_device(database->adapter, &bdaddr,
> + bdaddr_type);
> + if (device)
> + device_set_svc_chng_ccc(device, bdaddr_type, value);
> +
> + return 0;
> +}
> +
> static void populate_gatt_service(struct btd_gatt_database *database)
> {
> bt_uuid_t uuid;
> @@ -964,8 +983,9 @@ static void populate_gatt_service(struct btd_gatt_database *database)
> BT_ATT_PERM_READ, BT_GATT_CHRC_PROP_INDICATE,
> NULL, NULL, database);
>
> - database->svc_chngd_ccc = service_add_ccc(service, database, NULL, NULL,
> - NULL);
> + database->svc_chngd_ccc = service_add_ccc(service, database,
> + svc_chngd_ccc_write_cb,
> + database, NULL);
>
> gatt_db_service_set_active(service, true);
> }
> @@ -996,6 +1016,7 @@ static void state_set_pending(struct device_state *state, struct notify *notify)
> {
> uint16_t start, end, old_start, old_end;
>
> + /* Cache this only for Service Changed */
> if (notify->conf != service_changed_conf)
> return;
>
> @@ -3175,7 +3196,6 @@ struct btd_gatt_database *btd_gatt_database_new(struct btd_adapter *adapter)
> if (!database->db_id)
> goto fail;
>
> -
> return database;
>
> fail:
> @@ -3224,3 +3244,75 @@ void btd_gatt_database_att_connected(struct btd_gatt_database *database,
> free(state->pending);
> state->pending = NULL;
> }
> +
> +static void restore_ccc(struct btd_gatt_database *database,
> + const bdaddr_t *addr, uint8_t addr_type, uint16_t value)
> +{
> + struct device_state *dev_state;
> + struct ccc_state *ccc;
> +
> + dev_state = device_state_create(database, addr, addr_type);
> + queue_push_tail(database->device_states, dev_state);
> +
> + ccc = new0(struct ccc_state, 1);
> + ccc->handle = gatt_db_attribute_get_handle(database->svc_chngd_ccc);
> + memcpy(ccc->value, &value, sizeof(ccc->value));
> + queue_push_tail(dev_state->ccc_states, ccc);
> +}
> +
> +static void restore_state(struct btd_device *device, void *data)
> +{
> + struct btd_gatt_database *database = data;
> + uint16_t ccc_value;
> + uint8_t addr_type;
> +
> + addr_type = device_get_le_address_type(device);
> +
> + if (addr_type != BDADDR_BREDR) {
> + ccc_value = device_get_svc_chng_ccc(device, addr_type);
> + if (ccc_value) {
> + restore_ccc(database, device_get_address(device),
> + addr_type, ccc_value);
> + DBG("%s LE", device_get_path(device));
> + }
> + }
> +
> + ccc_value = device_get_svc_chng_ccc(device, BDADDR_BREDR);
> + if (ccc_value) {
> + restore_ccc(database, device_get_address(device), BDADDR_BREDR,
> + ccc_value);
> + DBG("%s BR/EDR", device_get_path(device));
> + }
> +}
> +
> +void btd_gatt_database_restore_svc_chng_ccc(struct btd_gatt_database *database)
> +{
> + uint8_t value[4];
> + uint16_t handle, ccc_handle;
> +
> + handle = gatt_db_attribute_get_handle(database->svc_chngd);
> + ccc_handle = gatt_db_attribute_get_handle(database->svc_chngd_ccc);
> +
> + if (!handle || !ccc_handle) {
> + error("Failed to obtain handles for \"Service Changed\""
> + " characteristic");
> + return;
> + }
> +
> + /* restore states for bonded device that registered for Service Changed
> + * indication
> + */
> + btd_adapter_for_each_device(database->adapter, restore_state, database);
> +
> + /* This needs to be updated (probably to 0x0001) if we ever change
> + * core services
> + *
> + * TODO we could also store this info (along with CCC value) and be able
> + * to send 0x0001-0xffff only once per device.
> + */
> + put_le16(0x000a, value);
> + put_le16(0xffff, value + 2);
> +
> + send_notification_to_devices(database, handle, value, sizeof(value),
> + ccc_handle, service_changed_conf, NULL);
> +}
> diff --git a/src/gatt-database.h b/src/gatt-database.h
> index 0da33f604..a6c3139c4 100644
> --- a/src/gatt-database.h
> +++ b/src/gatt-database.h
> @@ -25,3 +25,5 @@ void btd_gatt_database_destroy(struct btd_gatt_database *database);
> struct gatt_db *btd_gatt_database_get_db(struct btd_gatt_database *database);
> void btd_gatt_database_att_connected(struct btd_gatt_database *database,
> struct bt_att *att);
> +
> +void btd_gatt_database_restore_svc_chng_ccc(struct btd_gatt_database *database);
> --
> 2.14.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Luiz Augusto von Dentz
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
