diff mbox series

[BlueZ,v2,1/2] shared/uhid: Fix crash after bt_uhid_unregister_all

Message ID 20240916202341.238735-1-luiz.dentz@gmail.com
State Superseded
Headers show
Series [BlueZ,v2,1/2] shared/uhid: Fix crash after bt_uhid_unregister_all | expand

Commit Message

Luiz Augusto von Dentz Sept. 16, 2024, 8:23 p.m. UTC
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This fixes the following crash which happens when
bt_uhid_unregister_all is called from a notification callback:

Invalid read of size 8
   at 0x1D9EFF: queue_foreach (queue.c:206)
   by 0x1DEE58: uhid_read_handler (uhid.c:164)
 Address 0x51286d8 is 8 bytes inside a block of size 16 free'd
   at 0x48478EF: free (vg_replace_malloc.c:989)
   by 0x1DA08D: queue_remove_if (queue.c:292)
   by 0x1DA12F: queue_remove_all (queue.c:321)
   by 0x1DE592: bt_uhid_unregister_all (uhid.c:300)

Fixes: https://github.com/bluez/bluez/issues/952
---
 src/shared/uhid.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 44 insertions(+), 3 deletions(-)

Comments

patchwork-bot+bluetooth@kernel.org Sept. 17, 2024, 2:20 p.m. UTC | #1
Hello:

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

On Mon, 16 Sep 2024 16:23:40 -0400 you wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> This fixes the following crash which happens when
> bt_uhid_unregister_all is called from a notification callback:
> 
> Invalid read of size 8
>    at 0x1D9EFF: queue_foreach (queue.c:206)
>    by 0x1DEE58: uhid_read_handler (uhid.c:164)
>  Address 0x51286d8 is 8 bytes inside a block of size 16 free'd
>    at 0x48478EF: free (vg_replace_malloc.c:989)
>    by 0x1DA08D: queue_remove_if (queue.c:292)
>    by 0x1DA12F: queue_remove_all (queue.c:321)
>    by 0x1DE592: bt_uhid_unregister_all (uhid.c:300)
> 
> [...]

Here is the summary with links:
  - [BlueZ,v2,1/2] shared/uhid: Fix crash after bt_uhid_unregister_all
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=9a6a84a8a2b9
  - [BlueZ,v2,2/2] test-uhid: Add call to bt_uhid_unregister_all
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=f9f98c0b2aa4

You are awesome, thank you!
diff mbox series

Patch

diff --git a/src/shared/uhid.c b/src/shared/uhid.c
index ed21e1399ba7..20bd26781799 100644
--- a/src/shared/uhid.c
+++ b/src/shared/uhid.c
@@ -42,6 +42,7 @@  struct bt_uhid {
 	int ref_count;
 	struct io *io;
 	unsigned int notify_id;
+	bool notifying;
 	struct queue *notify_list;
 	struct queue *input;
 	uint8_t type;
@@ -56,6 +57,7 @@  struct uhid_notify {
 	uint32_t event;
 	bt_uhid_callback_t func;
 	void *user_data;
+	bool removed;
 };
 
 static void uhid_replay_free(struct uhid_replay *replay)
@@ -134,6 +136,28 @@  static int bt_uhid_record(struct bt_uhid *uhid, bool input,
 	return 0;
 }
 
+static bool match_removed(const void *a, const void *b)
+{
+	const struct uhid_notify *notify = a;
+
+	return notify->removed;
+}
+
+static void uhid_notify(struct bt_uhid *uhid, struct uhid_event *ev)
+{
+	/* Add a reference to the uhid to ensure it doesn't get freed while at
+	 * notify_handler.
+	 */
+	bt_uhid_ref(uhid);
+
+	uhid->notifying = true;
+	queue_foreach(uhid->notify_list, notify_handler, ev);
+	uhid->notifying = false;
+	queue_remove_all(uhid->notify_list, match_removed, NULL, free);
+
+	bt_uhid_unref(uhid);
+}
+
 static bool uhid_read_handler(struct io *io, void *user_data)
 {
 	struct bt_uhid *uhid = user_data;
@@ -161,7 +185,7 @@  static bool uhid_read_handler(struct io *io, void *user_data)
 		break;
 	}
 
-	queue_foreach(uhid->notify_list, notify_handler, &ev);
+	uhid_notify(uhid, &ev);
 
 	return true;
 }
@@ -292,13 +316,30 @@  static bool match_not_id(const void *a, const void *b)
 	return notify->id != id;
 }
 
+static void uhid_notify_removed(void *data, void *user_data)
+{
+	struct uhid_notify *notify = data;
+	struct bt_uhid *uhid = user_data;
+
+	/* Skip marking start_id as removed since that is not removed with
+	 * unregister all.
+	 */
+	if (notify->id == uhid->start_id)
+		return;
+
+	notify->removed = true;
+}
+
 bool bt_uhid_unregister_all(struct bt_uhid *uhid)
 {
 	if (!uhid)
 		return false;
 
-	queue_remove_all(uhid->notify_list, match_not_id,
+	if (!uhid->notifying)
+		queue_remove_all(uhid->notify_list, match_not_id,
 				UINT_TO_PTR(uhid->start_id), free);
+	else
+		queue_foreach(uhid->notify_list, uhid_notify_removed, uhid);
 
 	return true;
 }
@@ -588,7 +629,7 @@  resend:
 		return 0;
 	}
 
-	queue_foreach(uhid->notify_list, notify_handler, ev);
+	uhid_notify(uhid, ev);
 
 	return 0;
 }