[PATCH BlueZ v2] shared/gatt-client: confirm a synthesized CCC handle before writing to it

[PATCH BlueZ v2] shared/gatt-client: confirm a synthesized CCC handle before writing to it

[Proxy alt]

From: Proxy <proxy-alt@proxy-alt.dev>

discover_descs() still synthesizes a 0x2902 for a notify/indicate
characteristic's lone descriptor without ever asking the peer - that
part is unchanged, since always discovering costs a round trip on
every characteristic for the sake of devices that violate Vol 3, Part
G 3.3.1.1. What changes is register_notify(): before it writes to a
handle discover_descs() only guessed at, it now issues one
single-handle FIND_INFORMATION to let the peer answer for itself, and
only after that answer confirms a real 0x2902 does the CCC write
happen at all.

If the peer's answer is anything else - a different UUID, or no
answer - chrc->ccc_handle is cleared instead of written to.
register_notify() already handles a characteristic with no CCC
correctly (gatt_db_attribute_get_ccc() returning NULL takes the same
path), so this reaches that existing, correct behaviour instead of
writing 0x0100 into an attribute the peer never claimed was a CCC.

Cost: one extra FIND_INFORMATION per notify/indicate characteristic
whose sole descriptor was synthesized, the first time register_notify()
is called for it.

v2 of the patch attached to this issue fixes a real bug the first
version had: unverified_ccc lived on struct bt_gatt_client, but
discover_descs() only ever runs on the root client, while
register_notify() is commonly called through a clone
(bt_gatt_client_clone(), used by src/gatt-client.c per D-Bus consumer)
- whose own copy of that queue is always empty. The result was that
the verify step silently never triggered and the original blind write
still happened. Fixed by adding root_client(), a two-line walk up
->parent, and using root_client(client)->unverified_ccc at both call
sites instead of client->unverified_ccc directly.

Tested against real hardware this time, and traced end to end. Built
and ran as bluetoothd itself (not a test harness) on plain Debian, no
containers, connected to a real Cync device (F4:BC:DA:39:03:D4) whose
notify characteristic's descriptor discovery skips 0x0013 exactly as
this issue describes - discover_descs_cb() finds 0x0004/0x0016/0x0019/
0x001c as 0x2901 and never queries 0x0013 at all. Calling StartNotify
on that characteristic with v1 of the patch reproduced the original
bug unchanged: a WRITE_REQ to 0x0013 that timed out after 30s
(src/shared/att.c:timeout_cb() ... 0x12) and tore down a connection
that was otherwise healthy - which is what led to finding the clone
bug above. With that fixed and the identical scenario repeated:

  verify_ccc_cb() handle 0x0013 confirmed is not a CCC descriptor

StartNotify's D-Bus method call returns success immediately, no write
is sent to 0x0013, and the connection stays up (confirmed via
Device1.Connected afterward). This is the same device, same
characteristic, same daemon build, same session - only the
root_client() fix differs between the failing and passing runs.

Fixes: https://github.com/bluez/bluez/issues/2383
Signed-off-by: Proxy <proxy-alt@proxy-alt.dev>
---
 src/shared/gatt-client.c | 182 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 178 insertions(+), 4 deletions(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index f8ebab3..cce73e6 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -94,6 +94,14 @@ struct bt_gatt_client {
 	int next_reg_id;
 	unsigned int disc_id, nfy_id, nfy_mult_id, ind_id;
 
+	/*
+	 * Handles of CCC descriptors that were synthesized rather than
+	 * discovered (discover_descs() assumed a lone descriptor on a
+	 * notify/indicate characteristic must be the CCC). Consulted by
+	 * register_notify() before it writes to one of these handles.
+	 */
+	struct queue *unverified_ccc;
+
 	/*
 	 * Handles of the GATT Service and the Service Changed characteristic
 	 * value handle. These will have the value 0 if they are not present on
@@ -116,6 +124,23 @@ struct bt_gatt_client {
 	unsigned int mtu_req_id;
 };
 
+/*
+ * discover_descs() only ever runs on the root (non-cloned) client, since
+ * clones share the parent's gatt_db rather than discovering it themselves
+ * (see bt_gatt_client_clone()). unverified_ccc must therefore live on the
+ * root: a clone's own copy is always empty, and register_notify() is
+ * commonly called through a clone (src/gatt-client.c takes one per D-Bus
+ * consumer), so checking client->unverified_ccc directly there would never
+ * see anything discover_descs() recorded.
+ */
+static struct bt_gatt_client *root_client(struct bt_gatt_client *client)
+{
+	while (client->parent)
+		client = client->parent;
+
+	return client;
+}
+
 struct request {
 	struct bt_gatt_client *client;
 	bool long_write;
@@ -223,10 +248,20 @@ struct notify_chrc {
 	int notify_count;  /* Reference count of registered notify callbacks */
 
 	/* Pending calls to register_notify are queued here so that they can be
-	 * processed after a write that modifies the CCC descriptor.
+	 * processed after a write that modifies the CCC descriptor, or after
+	 * a pending ccc_verify_req below is resolved.
 	 */
 	struct queue *reg_notify_queue;
 	unsigned int ccc_write_id;
+
+	/*
+	 * Set if ccc_handle names a descriptor discover_descs() synthesized
+	 * rather than discovered. register_notify() must confirm it with the
+	 * peer before writing to it; ccc_verify_req is the outstanding
+	 * confirmation request, if any.
+	 */
+	bool ccc_unverified;
+	struct bt_gatt_request *ccc_verify_req;
 };
 
 struct notify_data {
@@ -287,6 +322,11 @@ static void notify_chrc_free(void *data)
 	if (chrc->notify_id)
 		gatt_db_attribute_unregister(chrc->attr, chrc->notify_id);
 
+	if (chrc->ccc_verify_req) {
+		bt_gatt_request_cancel(chrc->ccc_verify_req);
+		bt_gatt_request_unref(chrc->ccc_verify_req);
+	}
+
 	queue_destroy(chrc->reg_notify_queue, notify_data_unref);
 	free(chrc);
 }
@@ -338,9 +378,19 @@ static struct notify_chrc *notify_chrc_create(struct bt_gatt_client *client,
 	}
 
 	ccc = gatt_db_attribute_get_ccc(attr);
-	if (ccc)
+	if (ccc) {
 		chrc->ccc_handle = gatt_db_attribute_get_handle(ccc);
 
+		/*
+		 * If discover_descs() never actually asked the peer about
+		 * this handle, don't trust it until register_notify() has
+		 * confirmed it.
+		 */
+		if (queue_remove(root_client(client)->unverified_ccc,
+					UINT_TO_PTR(chrc->ccc_handle)))
+			chrc->ccc_unverified = true;
+	}
+
 	chrc->client = client;
 	chrc->attr = attr;
 	chrc->value_handle = value_handle;
@@ -793,6 +843,16 @@ static bool discover_descs(struct discovery_op *op, bool *discovering)
 							&ccc_uuid, 0, NULL,
 							NULL, NULL);
 			if (attr) {
+				/*
+				 * The peer was never asked about this handle.
+				 * register_notify() will issue a single-handle
+				 * FIND_INFORMATION before it writes here, in
+				 * case this device is one of the ones that
+				 * declares notify/indicate without actually
+				 * having a CCC descriptor.
+				 */
+				queue_push_tail(root_client(client)->unverified_ccc,
+						UINT_TO_PTR(desc_start));
 				free(chrc_data);
 				continue;
 			}
@@ -1751,6 +1811,102 @@ static bool match_notify_chrc_value_handle(const void *a, const void *b)
 	return chrc->value_handle == value_handle;
 }
 
+/*
+ * Resumes register_notify() for notify_data once ccc_unverified has been
+ * settled, taking the same branch register_notify() itself would have taken
+ * had the answer been known up front.
+ */
+static void resume_after_ccc_verify(struct notify_data *notify_data)
+{
+	struct notify_chrc *chrc = notify_data->chrc;
+
+	if (chrc->notify_count > 1 || !chrc->ccc_handle ||
+							!notify_data->callback) {
+		complete_notify_request(notify_data);
+		return;
+	}
+
+	if (!notify_data_write_ccc(notify_data, true, enable_ccc_callback))
+		complete_notify_request(notify_data);
+}
+
+static void verify_ccc_cb(bool success, uint8_t att_ecode,
+					struct bt_gatt_result *result,
+					void *user_data)
+{
+	struct notify_data *notify_data = user_data;
+	struct notify_chrc *chrc = notify_data->chrc;
+	struct bt_gatt_client *client = notify_data->client;
+	struct bt_gatt_iter iter;
+	uint16_t handle;
+	uint128_t u128;
+	bt_uuid_t uuid, ccc_uuid;
+	bool is_ccc = false;
+
+	chrc->ccc_verify_req = NULL;
+	chrc->ccc_unverified = false;
+
+	bt_uuid16_create(&ccc_uuid, GATT_CLIENT_CHARAC_CFG_UUID);
+
+	if (success && result && bt_gatt_iter_init(&iter, result) &&
+			bt_gatt_iter_next_descriptor(&iter, &handle,
+								u128.data)) {
+		bt_uuid128_create(&uuid, u128);
+
+		if (handle == chrc->ccc_handle && !bt_uuid_cmp(&uuid,
+								&ccc_uuid))
+			is_ccc = true;
+	}
+
+	DBG(client, "handle 0x%04x confirmed %s a CCC descriptor",
+				chrc->ccc_handle, is_ccc ? "is" : "is not");
+
+	/*
+	 * The peer just answered for itself: the earlier guess was wrong.
+	 * Undo it so nothing downstream (including a later notify_count > 1
+	 * fast path) treats this characteristic as having a CCC to write.
+	 */
+	if (!is_ccc)
+		chrc->ccc_handle = 0;
+
+	resume_after_ccc_verify(notify_data);
+
+	if (is_ccc)
+		return;
+
+	/*
+	 * No write is coming to drive enable_ccc_callback's usual flush of
+	 * reg_notify_queue, so do it here instead.
+	 */
+	queue_remove_all(chrc->reg_notify_queue, notify_set_ecode,
+				UINT_TO_PTR(0), complete_notify_request);
+}
+
+/*
+ * Issues a single-handle FIND_INFORMATION for chrc->ccc_handle to confirm
+ * it is really a CCC descriptor before register_notify() writes to it.
+ * Returns false only on the kind of immediate failure register_notify()
+ * already treats as a failed registration.
+ */
+static bool verify_ccc_handle(struct notify_data *notify_data)
+{
+	struct notify_chrc *chrc = notify_data->chrc;
+	struct bt_gatt_client *client = notify_data->client;
+
+	chrc->ccc_verify_req = bt_gatt_discover_descriptors(client->att,
+						chrc->ccc_handle,
+						chrc->ccc_handle,
+						verify_ccc_cb,
+						notify_data_ref(notify_data),
+						notify_data_unref);
+	if (!chrc->ccc_verify_req) {
+		notify_data_unref(notify_data);
+		return false;
+	}
+
+	return true;
+}
+
 static unsigned int register_notify(struct bt_gatt_client *client,
 				uint16_t handle,
 				bt_gatt_client_register_callback_t callback,
@@ -1804,10 +1960,11 @@ static unsigned int register_notify(struct bt_gatt_client *client,
 	__sync_fetch_and_add(&notify_data->chrc->notify_count, 1);
 
 	/*
-	 * If a write to the CCC descriptor is in progress, then queue this
+	 * If a write to the CCC descriptor is in progress, or a synthesized
+	 * CCC handle is still being confirmed with the peer, then queue this
 	 * request.
 	 */
-	if (chrc->ccc_write_id) {
+	if (chrc->ccc_write_id || chrc->ccc_verify_req) {
 		queue_push_tail(chrc->reg_notify_queue, notify_data);
 		return notify_data->id;
 	}
@@ -1821,6 +1978,21 @@ static unsigned int register_notify(struct bt_gatt_client *client,
 		return notify_data->id;
 	}
 
+	/*
+	 * ccc_handle was never actually discovered - confirm it with the
+	 * peer before writing to it. resume_after_ccc_verify() takes the
+	 * write-or-complete branch below once the answer is known.
+	 */
+	if (chrc->ccc_unverified) {
+		if (!verify_ccc_handle(notify_data)) {
+			queue_remove(client->notify_list, notify_data);
+			free(notify_data);
+			return 0;
+		}
+
+		return notify_data->id;
+	}
+
 	/* Write to the CCC descriptor */
 	if (!notify_data_write_ccc(notify_data, true, enable_ccc_callback)) {
 		queue_remove(client->notify_list, notify_data);
@@ -2295,6 +2467,7 @@ static void bt_gatt_client_free(struct bt_gatt_client *client)
 
 	queue_destroy(client->notify_chrcs, notify_chrc_free);
 	queue_destroy(client->notify_list, notify_data_cleanup);
+	queue_destroy(client->unverified_ccc, NULL);
 
 	queue_destroy(client->ready_cbs, ready_destroy);
 	queue_destroy(client->idle_cbs, idle_destroy);
@@ -2361,6 +2534,7 @@ static struct bt_gatt_client *gatt_client_new(struct gatt_db *db,
 	client->svc_chngd_queue = queue_new();
 	client->notify_list = queue_new();
 	client->notify_chrcs = queue_new();
+	client->unverified_ccc = queue_new();
 	client->pending_requests = queue_new();
 
 	client->nfy_id = bt_att_register(att, BT_ATT_OP_HANDLE_NFY,
-- 
2.54.0 (Apple Git-157)

RE: [BlueZ,v2] shared/gatt-client: confirm a synthesized CCC handle before writing to it

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 550 bytes --]

This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----

error: patch failed: src/shared/gatt-client.c:116
error: src/shared/gatt-client.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch

Please resolve the issue and submit the patches again.


---
Regards,
Linux Bluetooth

RE: [BlueZ,v2] shared/gatt-client: confirm a synthesized CCC handle before writing to it

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2991 bytes --]

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=1158295

---Test result---

Test Summary:
CheckPatch                    FAIL      0.48 seconds
GitLint                       FAIL      0.25 seconds
BuildEll                      PASS      20.07 seconds
BluezMake                     PASS      549.72 seconds
MakeCheck                     PASS      1.08 seconds
MakeDistcheck                 FAIL      127.88 seconds
CheckValgrind                 PASS      151.27 seconds
CheckSmatch                   PASS      297.92 seconds
bluezmakeextell               PASS      96.93 seconds
IncrementalBuild              PASS      562.30 seconds
ScanBuild                     PASS      878.13 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ,v2] shared/gatt-client: confirm a synthesized CCC handle before writing to it
WARNING:LONG_LINE: line length of 84 exceeds 80 columns
#251: FILE: src/shared/gatt-client.c:850:
+				queue_push_tail(root_client(client)->unverified_ccc,

WARNING:LONG_LINE: line length of 81 exceeds 80 columns
#270: FILE: src/shared/gatt-client.c:1820:
+							!notify_data->callback) {

/github/workspace/src/patch/14792115.patch total: 0 errors, 2 warnings, 255 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/patch/14792115.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[BlueZ,v2] shared/gatt-client: confirm a synthesized CCC handle before writing to it

1: T1 Title exceeds max length (84>80): "[BlueZ,v2] shared/gatt-client: confirm a synthesized CCC handle before writing to it"
##############################
Test: MakeDistcheck - FAIL
Desc: Run Bluez Make Distcheck
Output:

../../test-driver: line 107: 53187 Aborted                 (core dumped) "$@" > $log_file 2>&1
../../test-driver: line 107: 53223 Aborted                 (core dumped) "$@" > $log_file 2>&1
../../test-driver: line 107: 53503 Aborted                 (core dumped) "$@" > $log_file 2>&1
make[4]: *** [Makefile:10413: test-suite.log] Error 1
make[3]: *** [Makefile:10521: check-TESTS] Error 2
make[2]: *** [Makefile:11006: check-am] Error 2
make[1]: *** [Makefile:11008: check] Error 2
make: *** [Makefile:10929: distcheck] Error 1


https://github.com/bluez/bluez/pull/2493

---
Regards,
Linux Bluetooth