diff mbox series

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

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

Commit Message

Luiz Augusto von Dentz Sept. 16, 2024, 8:22 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

bluez.test.bot@gmail.com Sept. 16, 2024, 10:28 p.m. UTC | #1 
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=890772

---Test result---

Test Summary:
CheckPatch                    PASS      0.81 seconds
GitLint                       PASS      0.57 seconds
BuildEll                      PASS      24.74 seconds
BluezMake                     PASS      1627.17 seconds
MakeCheck                     PASS      13.33 seconds
MakeDistcheck                 PASS      178.50 seconds
CheckValgrind                 PASS      253.18 seconds
CheckSmatch                   PASS      355.42 seconds
bluezmakeextell               PASS      120.23 seconds
IncrementalBuild              PASS      2955.98 seconds
ScanBuild                     PASS      1053.50 seconds



---
Regards,
Linux Bluetooth
patchwork-bot+bluetooth@kernel.org Sept. 17, 2024, 2:20 p.m. UTC | #2 
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:22:31 -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,v1,1/2] shared/uhid: Fix not crash after bt_uhid_unregister_all
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=9a6a84a8a2b9
  - [BlueZ,v1,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;
 }