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

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

[Jared Crapo]

From: Jerry Wu <jhs730127@gmail.com>

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
Tested-by: Jared Crapo <jared@kotfu.net>
---
Resending: the original posting (30 Aug) was picked up by CI but dropped
after two weeks without review. The patch is unchanged apart from my
Tested-by, and still applies to current master (a0580da).

Jerry's notes from the original posting:

  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.

Tested on hardware:

- Raspberry Pi 4, Raspberry Pi OS (Debian trixie), Linux 6.18.50,
  BlueZ 5.82-1.1+rpt2 with and without this patch; controller a Realtek
  RTL8761B USB dongle.
- Peripheral: an external GATT application with one primary service, a
  write characteristic (AcquireWrite) and a notify characteristic
  (AcquireNotify), advertising through LEAdvertisingManager1.
- Centrals: an iPhone and an iPad (iOS, aprs.fi), both subscribed and not
  bonded (ReverseServiceDiscovery = false, so bluetoothd doesn't start a
  pairing by reading their Battery Level).

Without the patch, whenever one central left while subscribed (the
application called Device1.Disconnect on it, or Bluetooth was turned off
on the device), the other central's notify socket was closed at the same
moment and it stopped receiving with its link still up. With the patch,
in the same three tests (disconnect the iPad, disconnect the iPhone,
Bluetooth off on the iPad), only the departing central's socket closed
and the other kept receiving. A central that wrote CCC 0x0000 before
disconnecting never triggered it, consistent with clear_ccc_state()
skipping CCCs that are already 0.

 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 b511e13e3..d8e45581a 100644
--- a/src/gatt-database.c
+++ b/src/gatt-database.c
@@ -2977,8 +2977,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.56.0

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

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 858 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/series/1193317/

---Test result---

Test Summary:
CheckPatch                    PASS      0.51 seconds
GitLint                       PASS      0.35 seconds
BuildEll                      PASS      20.33 seconds
BluezMake                     PASS      309.98 seconds
CheckSmatch                   PASS      301.98 seconds
bluezmakeextell               PASS      96.63 seconds
TestFunctional                PASS      1134.88 seconds
IncrementalBuild              PASS      306.94 seconds
ScanBuild                     PASS      1175.94 seconds



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

---
Regards,
Linux Bluetooth

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

[patchwork-bot+bluetooth]

Hello:

This patch was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Sun, 25 Oct 2026 21:25:44 -0600 you wrote:
> From: Jerry Wu <jhs730127@gmail.com>
> 
> 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.
> 
> [...]

Here is the summary with links:
  - [BlueZ,RESEND] gatt-database: Fix freeing wrong client notify IO
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=f7e2cd91c266

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html