[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 a6abe8a..8c50131 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -86,6 +86,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
@@ -112,6 +120,23 @@ struct bt_gatt_client {
 	uint16_t pending_error_handle;
 };
 
+/*
+ * 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;
@@ -219,10 +244,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 {
@@ -283,6 +318,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);
 }
@@ -334,9 +374,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;
@@ -789,6 +839,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;
 			}
@@ -1747,6 +1807,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,
@@ -1800,10 +1956,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;
 	}
@@ -1817,6 +1974,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);
@@ -2291,6 +2463,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);
@@ -2507,6 +2680,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: 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

[PATCH BlueZ v3 1/2] shared/gatt-client: verify a synthesized CCC before writing

[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.

v3 fixes two real bugs v2 had, both found by actually running it
against real hardware and then a real unit test rather than trusting
that it read correctly:

1. 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 / by profile implementations
   like bt_micp) - whose own copy of that queue is always empty. The
   verify step silently never triggered. Fixed with root_client(), a
   two-line walk up ->parent, used at both call sites instead of
   client->unverified_ccc directly.

2. Once (1) was fixed and verify genuinely ran, two more bugs showed
   up together under unit/test-micp: the CCC write could get skipped
   entirely, and requests the client issued after it could go out on
   the wire ahead of the (still in-flight) verify - reordering ATT
   traffic relative to what every existing caller of register_notify()
   was written to expect from a synchronous write.

   Root cause: bt_gatt_discover_descriptors(), which the verify step
   uses, is not tracked in client->pending_requests the way
   bt_gatt_client_write_value()/read are. notify_client_idle() only
   watches pending_requests, so it considered the client idle - and
   fired every registered idle callback, letting application code run
   - while the verify FIND_INFORMATION was still genuinely outstanding
   on the wire. That is what let a later application write jump ahead
   of the CCC write register_notify() had not had a chance to send
   yet. Separately, resume_after_ccc_verify() used the same
   "notify_count > 1 means someone already wrote it" check
   register_notify() itself uses - correct there, but not after an
   async gap, since other callers for the same characteristic can
   (correctly) queue up behind chrc->ccc_verify_req and bump
   notify_count before verification even resolves, with nobody having
   written anything yet.

   Fixed both: notify_client_idle() now also checks whether any
   notify_chrc on the client still has a CCC verify outstanding before
   firing idle callbacks, chrc_has_pending_ccc_verify() added for that;
   resume_after_ccc_verify() no longer rechecks notify_count, since by
   construction this is the first and only place that can write once a
   characteristic was ccc_unverified; and verify_ccc_cb() calls
   notify_client_idle() itself once verification concludes without a
   write, since nothing else naturally rechecks idle for a request
   this codebase's idle-tracking never had to account for before now.

Also fixes a real leak (3 struct bt_gatt_result plus their backing
allocations, confirmed via LeakSanitizer against the same test):
verify_ccc_cb() never released the reference
bt_gatt_discover_descriptors() returns, unlike every other discovery
completion in this file (see discovery_req_clear()). Fixed with the
same bt_gatt_request_unref() pattern.

unit/test-micp.c is updated to match: three subtests
(MICP/CL/CGGIT/SER/BV-01-C, MICP/CL/CGGIT/CHA/BV-01-C, MICP/CL/SPE/BI-01-C)
drive a real notify-enable through a synthesized CCC and now expect the
FIND_INFORMATION exchange this patch adds before the CCC write. With
v2 (before the fixes above), this test reproduced the original bug
exactly as reported: a WRITE_REQ to the synthesized handle that got no
response and would have hung until the 30s ATT timeout in a real
session. With v3, ./unit/test-micp passes 7/7 with zero leaks under
LeakSanitizer.

unit/test-mcp.c and unit/test-bap.c hit the same class of pre-existing
mock-script gap (their own CCC-enable sequences need the equivalent
FIND_INFORMATION step added) but I have not finished updating those
yet - flagging rather than shipping a partial fix for them silently.

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

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

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index a6abe8a..2154a20 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -86,6 +86,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
@@ -112,6 +120,23 @@ struct bt_gatt_client {
 	uint16_t pending_error_handle;
 };
 
+/*
+ * 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;
@@ -178,12 +203,30 @@ bt_gatt_client_ref_safe(struct bt_gatt_client *client)
 	return bt_gatt_client_ref(client);
 }
 
+/*
+ * Defined after struct notify_chrc (below); checks whether any
+ * notify_chrc on this client still has a CCC verify FIND_INFORMATION
+ * outstanding. That request goes through bt_gatt_discover_descriptors(),
+ * which - unlike bt_gatt_client_write_value()/read - is not tracked in
+ * client->pending_requests, so notify_client_idle() cannot see it there.
+ * Without this, the client looks idle (and idle_cbs fire, including
+ * whatever the application does next) while a request is still genuinely
+ * outstanding on the wire, reordering it ahead of the CCC write
+ * register_notify() has not been able to send yet.
+ */
+static bool chrc_has_pending_ccc_verify(struct bt_gatt_client *client);
+
 static void notify_client_idle(struct bt_gatt_client *client)
 {
 	client = bt_gatt_client_ref_safe(client);
 	if (!client)
 		return;
 
+	if (chrc_has_pending_ccc_verify(client)) {
+		bt_gatt_client_unref(client);
+		return;
+	}
+
 	queue_remove_all(client->idle_cbs, idle_notify, NULL, idle_destroy);
 
 	bt_gatt_client_unref(client);
@@ -219,10 +262,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 {
@@ -283,6 +336,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);
 }
@@ -334,9 +392,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;
@@ -789,6 +857,20 @@ static bool discover_descs(struct discovery_op *op, bool *discovering)
 							&ccc_uuid, 0, NULL,
 							NULL, NULL);
 			if (attr) {
+				struct bt_gatt_client *root;
+
+				root = root_client(client);
+
+				/*
+				 * 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->unverified_ccc,
+						UINT_TO_PTR(desc_start));
 				free(chrc_data);
 				continue;
 			}
@@ -1747,6 +1829,130 @@ static bool match_notify_chrc_value_handle(const void *a, const void *b)
 	return chrc->value_handle == value_handle;
 }
 
+static bool match_chrc_ccc_verify_pending(const void *data,
+						const void *user_data)
+{
+	const struct notify_chrc *chrc = data;
+
+	return chrc->ccc_verify_req != NULL;
+}
+
+static bool chrc_has_pending_ccc_verify(struct bt_gatt_client *client)
+{
+	return queue_find(client->notify_chrcs, match_chrc_ccc_verify_pending,
+							NULL) != NULL;
+}
+
+/*
+ * Resumes register_notify() for notify_data once ccc_unverified has been
+ * settled. This is deliberately not the same "notify_count > 1 means
+ * someone already wrote it" check register_notify() itself uses: with the
+ * verify request outstanding, every other caller for this characteristic
+ * queued behind chrc->ccc_verify_req instead of writing (register_notify()
+ * checks that before it checks notify_count), so notify_count having grown
+ * past 1 by the time verify resolves just means callers piled up while we
+ * waited - not that anyone already wrote the CCC. This resume is the only
+ * place that can, so it must go by whether the peer confirmed a CCC exists,
+ * not by how many callers are now waiting on the answer.
+ */
+static void resume_after_ccc_verify(struct notify_data *notify_data)
+{
+	struct notify_chrc *chrc = notify_data->chrc;
+
+	if (!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;
+
+	bt_gatt_request_unref(chrc->ccc_verify_req);
+	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);
+
+	/*
+	 * Nothing else naturally rechecks idle now that ccc_verify_req is
+	 * clear and no write followed it - do it explicitly, the same way
+	 * request_unref() would if this had gone through pending_requests.
+	 */
+	notify_client_idle(client);
+}
+
+/*
+ * 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,
@@ -1800,10 +2006,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;
 	}
@@ -1817,6 +2024,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);
@@ -2291,6 +2513,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);
@@ -2507,6 +2730,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)

[PATCH BlueZ v3 2/2] unit/test-micp: expect the FIND_INFORMATION the CCC fix adds

[Proxy alt]

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

Companion to the shared/gatt-client.c fix (bluez/bluez#2383): the
lazy-verify patch adds one FIND_INFORMATION exchange before the first
CCC write to a characteristic whose descriptor discover_descs()
synthesized. MICS_MUTE's CCC (handle 0x0004) is exactly that case, so
the three client subtests that drive it now need that exchange in
their scripted ATT sequence, right where register_notify() actually
issues it (after the value read, before the CCC-enable write).

Verified: ./unit/test-micp passes 7/7 with zero leaks under
LeakSanitizer, against the corresponding shared/gatt-client.c fix.

unit/test-mcp.c and unit/test-bap.c need the equivalent update for
their own synthesized-CCC subtests; not included here.

Signed-off-by: Proxy <proxy-alt@proxy-alt.dev>
---
 unit/test-micp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/unit/test-micp.c b/unit/test-micp.c
index ff17300..2229642 100644
--- a/unit/test-micp.c
+++ b/unit/test-micp.c
@@ -455,18 +455,21 @@ static void test_server(const void *user_data)
 
 #define MICP_CL_CGGIT_SER_BV_01_C \
 	MICS_MUTE_READ, \
+	MICP_FIND_INFO_REQ, \
 	MICS_EN_MUTE_DISCPTR, \
 	IOV_DATA(0x12, 0x03, 0x00, 0x01, 0x00), \
 	IOV_DATA(0x01, 0x12, 0x03, 0x00, 0x013)
 
 #define	MICP_CL_CGGIT_CHA_BV_01_C	\
 	MICS_MUTE_READ, \
+	MICP_FIND_INFO_REQ, \
 	MICS_EN_MUTE_DISCPTR, \
 	IOV_DATA(0x12, 0x03, 0x00, 0x01, 0x00), \
 	IOV_DATA(0x013)
 
 #define MICP_CL_SPE_BI_01_C	\
 	MICS_MUTE_READ, \
+	MICP_FIND_INFO_REQ, \
 	MICS_EN_MUTE_DISCPTR, \
 	IOV_DATA(0x12, 0x03, 0x00, 0x01, 0x00), \
 	IOV_DATA(0x01, 0x12, 0x03, 0x00, 0x80)
-- 
2.54.0 (Apple Git-157)

RE: [BlueZ,v3,1/2] shared/gatt-client: verify a synthesized CCC before writing

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2710 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=1158378

---Test result---

Test Summary:
CheckPatch                    PASS      0.80 seconds
GitLint                       PASS      0.50 seconds
BuildEll                      PASS      13.27 seconds
BluezMake                     PASS      428.90 seconds
MakeCheck                     FAIL      6.92 seconds
MakeDistcheck                 FAIL      85.52 seconds
CheckValgrind                 FAIL      117.31 seconds
CheckSmatch                   PASS      177.18 seconds
bluezmakeextell               PASS      63.36 seconds
IncrementalBuild              PASS      439.20 seconds
ScanBuild                     PASS      559.05 seconds

Details
##############################
Test: MakeCheck - FAIL
Desc: Run Bluez Make Check
Output:

./test-driver: line 107: 31720 Aborted                 (core dumped) "$@" > $log_file 2>&1
./test-driver: line 107: 31779 Aborted                 (core dumped) "$@" > $log_file 2>&1
make[3]: *** [Makefile:10413: test-suite.log] Error 1
make[2]: *** [Makefile:10521: check-TESTS] Error 2
make[1]: *** [Makefile:11006: check-am] Error 2
make: *** [Makefile:11008: check] Error 2
##############################
Test: MakeDistcheck - FAIL
Desc: Run Bluez Make Distcheck
Output:

../../test-driver: line 107: 53628 Aborted                 (core dumped) "$@" > $log_file 2>&1
../../test-driver: line 107: 53908 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
##############################
Test: CheckValgrind - FAIL
Desc: Run Bluez Make Check with Valgrind
Output:

tools/mgmt-tester.c: In function ‘main’:
tools/mgmt-tester.c:13131:5: note: variable tracking size limit exceeded with ‘-fvar-tracking-assignments’, retrying without
13131 | int main(int argc, char *argv[])
      |     ^~~~
./test-driver: line 107: 73177 Aborted                 (core dumped) "$@" > $log_file 2>&1
./test-driver: line 107: 73231 Aborted                 (core dumped) "$@" > $log_file 2>&1
make[3]: *** [Makefile:10413: test-suite.log] Error 1
make[2]: *** [Makefile:10521: check-TESTS] Error 2
make[1]: *** [Makefile:11006: check-am] Error 2
make: *** [Makefile:11008: check] Error 2


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

---
Regards,
Linux Bluetooth