[PATCH BlueZ] gatt-database: Fix freeing wrong client notify IO

[PATCH BlueZ] gatt-database: Fix freeing wrong client notify IO

[Jerry Wu]

When a non-bonded device disconnects, att_disconnected() drops its
device state and clear_ccc_state() invokes the CCC callback with a
NULL pending operation. ccc_write_cb() then takes the notifications
disabled path and calls queue_remove_if() on chrc->notify_ios with a
NULL ATT instance.

match_client_att() matches every entry when the ATT instance is NULL
and queue_remove_if() only removes the first match, so what gets freed
is the head of the queue, that is the client which subscribed first,
and not the one that went away. With two or more subscribers this is
deterministic, because the disconnecting client's IO is still queued
at that point: sock_hup() only runs on a later mainloop iteration.

The victim does not recover. Its link and its CCC value are left
untouched, so gatt_ccc_write_cb() takes the "value is identical"
shortcut on any subsequent write and AcquireNotify is never issued for
it again. To the application the notifications simply stop.

The disconnecting client does not need to be handled here at all, as
its IO is reclaimed through att_disconnect_cb() -> io_shutdown() ->
sock_hup(). Only remove an IO when there is an actual operation, and
let the NULL case fall through to the notify count accounting. That
also restores the StopNotify call when the last subscriber goes away,
which the early exit used to skip.

Fixes: 8eb1dee87e01 ("gatt: Fix not establishing a socket for each device")
Assisted-by: Claude:claude-opus-5
---

Notes for reviewers (below the --- line, not part of the commit):

Verified still present on current master (e814134, "doc: Remove obsolete
security-bugs.rst"). match_client_att(), the ccc_write_cb() disabled path
and the gatt_ccc_write_cb() "value is identical" shortcut are unchanged
since 8eb1dee87e01. Originally hit in production on 5.83.

Reproducing with two centrals and an external GATT application (one
implementing org.bluez.GattCharacteristic1 with AcquireNotify):

 1. Central A connects without bonding and enables notifications on a
    characteristic of the application. AcquireNotify is called and A
    becomes the head of chrc->notify_ios.
 2. Central B connects, also without bonding, and enables notifications
    on the same characteristic. notify_ios is now [A, B], ntfy_cnt is 2.
 3. Central B disconnects.

Observed: A stops receiving notifications at once, even though its LE
link is still up and btmon shows no disconnection for A. A's notify
socket was closed because A happened to be the queue head. A never
recovers, since its CCC value is still 0x0001, so rewriting the CCC is a
no-op and AcquireNotify is not called for it again. Only a full
reconnect of A brings notifications back.

Expected, and what this patch gives: A keeps its notify socket, and B's
socket is closed by sock_hup() as usual.

Steps 1 and 2 must not be swapped. If the client that disconnects
happens to be the queue head, the bug is invisible, which is why a test
with a single central always passes.

I also have a small standalone harness that links the real
src/shared/queue.c and exercises match_client_att() together with the
ccc_write_cb() disabled path, showing the queue head being freed before
the patch and kept after it. Happy to send it if that is useful.

 src/gatt-database.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/gatt-database.c b/src/gatt-database.c
index 18b7aa667..87b166ade 100644
--- a/src/gatt-database.c
+++ b/src/gatt-database.c
@@ -2971,8 +2971,15 @@ static uint8_t ccc_write_cb(struct pending_op *op, void *user_data)
 		if (!chrc->ntfy_cnt)
 			goto done;
 
-		client = queue_remove_if(chrc->notify_ios, match_client_att,
-							op ? op->att : NULL);
+		/*
+		 * A NULL op means clear_ccc_state() is clearing the CCC of a
+		 * device that has just disconnected. Its own IO is reclaimed
+		 * by att_disconnect_cb(), and match_client_att() matches any
+		 * entry when the ATT instance is NULL, which would free the
+		 * IO of an unrelated client, so only account for it here.
+		 */
+		client = op ? queue_remove_if(chrc->notify_ios,
+					match_client_att, op->att) : NULL;
 		if (client) {
 			client_io_free(client);
 			__sync_sub_and_fetch(&chrc->ntfy_cnt, 1);
-- 
2.50.1 (Apple Git-155)

RE: [BlueZ] gatt-database: Fix freeing wrong client notify IO

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    FAIL      0.37 seconds
GitLint                       PASS      0.26 seconds
BuildEll                      PASS      15.01 seconds
BluezMake                     PASS      575.97 seconds
CheckSmatch                   PASS      190.71 seconds
bluezmakeextell               PASS      68.34 seconds
IncrementalBuild              PASS      565.92 seconds
ScanBuild                     PASS      602.89 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ] gatt-database: Fix freeing wrong client notify IO
WARNING:BAD_SIGN_OFF: Non-standard signature: Assisted-by:
#122: 
Assisted-by: Claude:claude-opus-5

ERROR:BAD_SIGN_OFF: Unrecognized email address: 'Claude:claude-opus-5'
#122: 
Assisted-by: Claude:claude-opus-5

/github/workspace/src/patch/14775471.patch total: 1 errors, 1 warnings, 17 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/14775471.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.




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

---
Regards,
Linux Bluetooth