[PATCH BlueZ 0/2] Fix use-after-free in GATT client discovery

[PATCH BlueZ 0/2] Fix use-after-free in GATT client discovery

[Gianluca Altomani]

bluetoothd crashes in discovery_op_complete() when a discovery of a
connected peer's GATT database is cut short, e.g. because the link
drops mid-discovery. Two separate bugs leave a freed service in the
discovery's pending_svcs queue, and each patch fixes one of them.

It reproduces by connecting over LE, reading one characteristic and
disconnecting about once a second, with a peer that answers the
remote's GATT discovery. Each connection is short enough that the
remote's discovery of the peer is still running when the link drops.
On 5.72 bluetoothd crashed on the 15th connection in 4 runs out of 4.
With both patches it survived 381 connections in a row. On 5.83, 50
such connections crashed the unpatched daemon within the first minute
and pass with both patches.

Gianluca Altomani (2):
  shared/gatt-db: Always notify service removal
  shared/gatt-client: Fix double-queued service

 src/shared/gatt-client.c | 10 ++++++++--
 src/shared/gatt-db.c     |  9 +++++++--
 2 files changed, 15 insertions(+), 4 deletions(-)

-- 
2.55.0

[PATCH BlueZ 1/2] shared/gatt-db: Always notify service removal

[Gianluca Altomani]

gatt_db_service_destroy() only notified removal listeners about active
services. A gatt-client discovery keeps services that are not active
yet in its pending_svcs queue, and it only drops them from that queue
through this notification. When such a service is destroyed while the
discovery is in flight, for example through gatt_db_clear_range() after
the link drops mid-discovery, the queue is left pointing at freed
memory, and discovery_op_complete() then dereferences it:

  gatt_db_service_get_active (src/shared/gatt-db.c)
  discovery_op_complete (src/shared/gatt-client.c)

Notify removal whether or not the service was active. The other
removal listeners already ignore services they never saw added, since
services are only announced when they become active.

It is reproduced by connecting over LE, reading one characteristic and
disconnecting about once a second, so that the link repeatedly drops
while bluetoothd is still discovering the peer: on 5.72 bluetoothd
crashed on the 15th connection in 4 runs out of 4.

The same change was found independently in
https://github.com/ownback/airpods-bluez-fix.

Assisted-by: Claude:claude-opus-5-5
---
 src/shared/gatt-db.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 751d4c3da..88c8e9e4e 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -507,8 +507,13 @@ static void gatt_db_service_destroy(void *data)
 	struct gatt_db_service *service = data;
 	int i;
 
-	if (service->active)
-		notify_service_changed(service->db, service, false);
+	/*
+	 * Notify even if the service never became active: gatt-client
+	 * discovery keeps inactive services in its pending list and only
+	 * learns of their removal from this notification, so skipping it
+	 * leaves that list pointing at freed memory.
+	 */
+	notify_service_changed(service->db, service, false);
 
 	for (i = 0; i < service->num_handles; i++)
 		attribute_destroy(service->attributes[i]);
-- 
2.55.0

RE: Fix use-after-free in GATT client discovery

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1021 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/1176731/

---Test result---

Test Summary:
CheckPatch                    PASS      1.06 seconds
GitLint                       PASS      0.69 seconds
BuildEll                      PASS      19.53 seconds
BluezMake                     PASS      392.19 seconds
MakeCheck                     PASS      1.11 seconds
MakeDistcheck                 PASS      139.36 seconds
CheckValgrind                 PASS      149.86 seconds
CheckSmatch                   PASS      289.11 seconds
bluezmakeextell               PASS      93.56 seconds
TestFunctional                PASS      1118.26 seconds
IncrementalBuild              PASS      394.02 seconds
ScanBuild                     PASS      1115.32 seconds



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

---
Regards,
Linux Bluetooth

[PATCH BlueZ 2/2] shared/gatt-client: Fix double-queued service

[Gianluca Altomani]

discovery_op_create() loads every service already in the client
database into pending_svcs, active or not, and discovery_found_service()
then queues an inactive service again when it finds it. A service left
inactive by an earlier discovery that was cut short therefore ends up
in the queue twice.

If that discovery fails too, discovery_op_complete() walks pending_svcs
with its removal callback already unregistered: the first entry removes
and frees the service, and the second one dereferences it:

  gatt_db_service_get_active (src/shared/gatt-db.c)
  discovery_op_complete (src/shared/gatt-client.c)

Only queue an inactive service if it is not pending already.

Assisted-by: Claude:claude-opus-5-5
---
 src/shared/gatt-client.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 95f83fa3e..3520b5cad 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -1170,10 +1170,16 @@ static void discovery_found_service(struct discovery_op *op,
 {
 	/* Skip if service already active */
 	if (!gatt_db_service_get_active(attr)) {
-		/* Skip if there are no attributes */
+		/*
+		 * Skip if there are no attributes. An inactive service left by
+		 * an interrupted discovery is already pending from
+		 * discovery_op_create(); queueing it twice makes
+		 * discovery_op_complete() free it and then dereference the
+		 * second entry.
+		 */
 		if (end == start)
 			gatt_db_service_set_active(attr, true);
-		else
+		else if (!queue_find(op->pending_svcs, NULL, attr))
 			queue_push_tail(op->pending_svcs, attr);
 
 		if (start < op->svc_first)
-- 
2.55.0