public inbox for linux-bluetooth@vger.kernel.org
 help / color / mirror / Atom feed
* [bug report] GATT server crash after client disconnect (UAF)
@ 2024-06-26 10:53 Kévin Courdesses
  2024-06-26 11:12 ` [bug,report] " bluez.test.bot
  2024-06-26 14:03 ` [bug report] " Luiz Augusto von Dentz
  0 siblings, 2 replies; 3+ messages in thread
From: Kévin Courdesses @ 2024-06-26 10:53 UTC (permalink / raw)
  To: linux-bluetooth

Hello,

We detected a spurious crash of `bluetoothd` in our embedded system,
acting as a GATT server. The system is running the latest BlueZ
release (5.76).

I traced the problem to a use-after-free (UAF) error, sometimes
triggered when a device disconnects while a characteristic read or
write is still being processed. Below are relevant debug and Valgrind
traces.

----
bluetoothd[167]: src/shared/gatt-server.c:handle_read_req() Read Req -
handle: 0x0014
bluetoothd[167]: src/shared/gatt-server.c:read_complete_cb() Read
Complete: err 0
bluetoothd[167]: src/shared/att.c:can_read_data() (chan 0x53bd198) ATT
PDU received: 0x0a
bluetoothd[167]: src/shared/gatt-server.c:handle_read_req() Read Req -
handle: 0x002e
bluetoothd[167]: src/shared/mgmt.c:can_read_data() [0x0000] event 0x000c
bluetoothd[167]: src/adapter.c:dev_disconnected() Device
48:A4:72:79:82:D5 disconnected, reason 3
bluetoothd[167]: src/adapter.c:adapter_remove_connection()
bluetoothd[167]: plugins/policy.c:disconnect_cb() reason 3
bluetoothd[167]: src/adapter.c:bonding_attempt_complete() hci0 bdaddr
48:A4:72:79:82:D5 type 1 status 0xe
bluetoothd[167]: src/device.c:device_bonding_complete() bonding (nil)
status 0x0e
bluetoothd[167]: src/device.c:btd_device_set_temporary() temporary 1
bluetoothd[167]: src/device.c:device_bonding_failed() status 14
bluetoothd[167]: src/adapter.c:resume_discovery()
bluetoothd[167]: src/shared/att.c:disconnect_cb() Channel 0x53bd198
disconnected: Connection reset by peer
bluetoothd[167]: src/device.c:att_disconnected_cb()
bluetoothd[167]: src/device.c:att_disconnected_cb() Connection reset
by peer (104)
bluetoothd[167]: src/service.c:change_state() 0x53faa80: device
48:A4:72:79:82:D5 profile gap-profile state changed: connected ->
disconnecting (0)
bluetoothd[167]: src/service.c:change_state() 0x53faa80: device
48:A4:72:79:82:D5 profile gap-profile state changed: disconnecting ->
disconnected (0)
bluetoothd[167]: src/service.c:change_state() 0x54017a8: device
48:A4:72:79:82:D5 profile deviceinfo state changed: connected ->
disconnecting (0)
bluetoothd[167]: src/service.c:change_state() 0x54017a8: device
48:A4:72:79:82:D5 profile deviceinfo state changed: disconnecting ->
disconnected (0)
bluetoothd[167]: src/gatt-client.c:btd_gatt_client_disconnected()
Device disconnected. Cleaning up.
bluetoothd[167]: src/device.c:att_disconnected_cb() Automatic
connection disabled
bluetoothd[167]: src/gatt-database.c:btd_gatt_database_att_disconnected()
bluetoothd[167]: src/gatt-database.c:att_disconnected()
bluetoothd[167]: attrib/gattrib.c:g_attrib_unref() 0x53bd140: g_attrib_unref=0
bluetoothd[167]: src/gatt-database.c:read_reply_cb() Pending read was
canceled when object got removed
bluetoothd[167]: src/shared/gatt-server.c:read_complete_cb() Read
Complete: err -110
==167== Invalid read of size 4
==167==    at 0x8277C: bt_att_chan_send_error_rsp (in
/usr/libexec/bluetooth/bluetoothd)
==167==  Address 0x53bd198 is 0 bytes inside a block of size 44 free'd
==167==    at 0x482F350: free (vg_replace_malloc.c:538)
==167==    by 0x8320F: disconnect_cb (in /usr/libexec/bluetooth/bluetoothd)
==167==  Block was alloc'd at
==167==    at 0x482DE08: malloc (vg_replace_malloc.c:307)
==167==    by 0x7BDD3: util_malloc (in /usr/libexec/bluetooth/bluetoothd)
==167==
[cropped, valgrind complains a lot after this]
----

These logs illustrate that the problem occurs soon after
`read_complete_cb` is executed, in `bt_att_chan_send_error_rsp`.

src/shared/gatt-server.c
     904 static void read_complete_cb(struct gatt_db_attribute *attr, int err,
     905                     const uint8_t *value, size_t len,
     906                     void *user_data)
     907 {
     908     struct async_read_op *op = user_data;
     909     struct bt_gatt_server *server = op->server;
     910     uint8_t rsp_opcode;
     911     uint16_t mtu;
     912     uint16_t handle;
     913
     914     DBG(server, "Read Complete: err %d", err);
     915
     916     mtu = bt_att_get_mtu(server->att);
     917     handle = gatt_db_attribute_get_handle(attr);
     918
     919     if (err) {
     920         bt_att_chan_send_error_rsp(op->chan, op->opcode, handle, err);
     921         async_read_op_destroy(op);
     922         return;
     923     }

However, at this point, `op->chan` has already been freed by a `disconnect_cb`.

I'm not familiar with the BlueZ code base and I don't know the
idiomatic way to fix this issue. I'd say `bt_att_chan_send_error_rsp`
should only be called if `op->chan` is still allocated. The following
patch attempts to implement a solution. It's probably not the right
way to do it, but this seemingly fixes the issue on our side.

-----
diff --git a/src/shared/att.c b/src/shared/att.c
index 485ef07..baef0cc 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -2068,3 +2068,11 @@ done:

        return true;
 }
+
+struct queue *bt_att_get_chans(struct bt_att *att)
+{
+       if (!att)
+               return NULL;
+
+       return att->chans;
+}
\ No newline at end of file
diff --git a/src/shared/att.h b/src/shared/att.h
index 6fd7863..98f91e1 100644
--- a/src/shared/att.h
+++ b/src/shared/att.h
@@ -111,3 +111,5 @@ bool bt_att_set_remote_key(struct bt_att *att,
uint8_t sign_key[16],
                        bt_att_counter_func_t func, void *user_data);
 bool bt_att_has_crypto(struct bt_att *att);
 bool bt_att_set_retry(struct bt_att *att, unsigned int id, bool retry);
+
+struct queue *bt_att_get_chans(struct bt_att *att);
\ No newline at end of file
diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
index 0e399ce..ca9af59 100644
--- a/src/shared/gatt-server.c
+++ b/src/shared/gatt-server.c
@@ -790,7 +790,21 @@ static void write_complete_cb(struct
gatt_db_attribute *attr, int err,
        handle = gatt_db_attribute_get_handle(attr);

        if (err)
-               bt_att_chan_send_error_rsp(op->chan, op->opcode, handle, err);
+       {
+               // Only call bt_att_chan_send_error_rsp if the channel
+               // is still valid
+               struct bt_att *att = bt_gatt_server_get_att(server);
+               struct queue *chans = bt_att_get_chans(att);
+               const struct queue_entry *entry;
+               for (entry = queue_get_entries(chans); entry; entry =
entry->next)
+               {
+                       if (entry->data == op->chan)
+                       {
+                               bt_att_chan_send_error_rsp(op->chan,
op->opcode, handle, err);
+                               break;
+                       }
+               }
+       }
        else
                bt_att_chan_send_rsp(op->chan, BT_ATT_OP_WRITE_RSP, NULL, 0);

@@ -917,7 +931,19 @@ static void read_complete_cb(struct
gatt_db_attribute *attr, int err,
        handle = gatt_db_attribute_get_handle(attr);

        if (err) {
-               bt_att_chan_send_error_rsp(op->chan, op->opcode, handle, err);
+               // Only call bt_att_chan_send_error_rsp if the channel
+               // is still valid
+               struct bt_att *att = bt_gatt_server_get_att(server);
+               struct queue *chans = bt_att_get_chans(att);
+               const struct queue_entry *entry;
+               for (entry = queue_get_entries(chans); entry; entry =
entry->next)
+               {
+                       if (entry->data == op->chan)
+                       {
+                               bt_att_chan_send_error_rsp(op->chan,
op->opcode, handle, err);
+                               break;
+                       }
+               }
                async_read_op_destroy(op);
                return;
        }
-----

Regards,

Kévin

^ permalink raw reply related	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-06-26 14:03 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-26 10:53 [bug report] GATT server crash after client disconnect (UAF) Kévin Courdesses
2024-06-26 11:12 ` [bug,report] " bluez.test.bot
2024-06-26 14:03 ` [bug report] " Luiz Augusto von Dentz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox