public inbox for linux-kernel-mentees@lists.linux-foundation.org
 help / color / mirror / Atom feed
* [PATCH] Bluetooth: L2CAP: Fix use-after-free in l2cap_unregister_user
@ 2025-11-05 14:22 ssrane_b23
  2025-11-05 19:36 ` Pauli Virtanen
  2025-11-06 18:20 ` [PATCH v2 1/1] " ssrane_b23
  0 siblings, 2 replies; 3+ messages in thread
From: ssrane_b23 @ 2025-11-05 14:22 UTC (permalink / raw)
  To: marcel
  Cc: johan.hedberg, luiz.dentz, linux-bluetooth, linux-kernel,
	syzbot+14b6d57fb728e27ce23c, linux-kernel-mentees, skhan,
	david.hunter.linux, khalid, Shaurya Rane

From: Shaurya Rane <ssrane_b23@ee.vjti.ac.in>

Syzbot reported a use-after-free in l2cap_unregister_user(), caused by
missing reference counting on the associated hci_dev. If the device is
unregistered while L2CAP users are still active, l2cap_unregister_user()
may access a freed hci_dev when taking its lock.

Fix this by taking a device reference in l2cap_register_user() using
hci_dev_hold(), and releasing it in l2cap_unregister_user() via
hci_dev_put(). This ensures the hci_dev remains valid for the lifetime
of registered L2CAP users.

Reported-by: syzbot+14b6d57fb728e27ce23c@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=14b6d57fb728e27ce23c
Fixes: c8992cffbe74 ("Bluetooth: hci_event: Use of a function table to handle Command Complete")
Signed-off-by: Shaurya Rane <ssrane_b23@ee.vjti.ac.in>
---
 net/bluetooth/l2cap_core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 805c752ac0a9..6a880f8ab6c2 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1688,6 +1688,11 @@ int l2cap_register_user(struct l2cap_conn *conn, struct l2cap_user *user)
 	struct hci_dev *hdev = conn->hcon->hdev;
 	int ret;
 
+	/* Hold a reference to hdev to prevent it from being freed while
+	 * we have registered users.
+	 */
+	hci_dev_hold(hdev);
+
 	/* We need to check whether l2cap_conn is registered. If it is not, we
 	 * must not register the l2cap_user. l2cap_conn_del() is unregisters
 	 * l2cap_conn objects, but doesn't provide its own locking. Instead, it
@@ -1717,6 +1722,10 @@ int l2cap_register_user(struct l2cap_conn *conn, struct l2cap_user *user)
 
 out_unlock:
 	hci_dev_unlock(hdev);
+
+	if (ret)
+		hci_dev_put(hdev);
+
 	return ret;
 }
 EXPORT_SYMBOL(l2cap_register_user);
@@ -1735,6 +1744,9 @@ void l2cap_unregister_user(struct l2cap_conn *conn, struct l2cap_user *user)
 
 out_unlock:
 	hci_dev_unlock(hdev);
+
+	/* Release the reference we took in l2cap_register_user */
+	hci_dev_put(hdev);
 }
 EXPORT_SYMBOL(l2cap_unregister_user);
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] Bluetooth: L2CAP: Fix use-after-free in l2cap_unregister_user
  2025-11-05 14:22 [PATCH] Bluetooth: L2CAP: Fix use-after-free in l2cap_unregister_user ssrane_b23
@ 2025-11-05 19:36 ` Pauli Virtanen
  2025-11-06 18:20 ` [PATCH v2 1/1] " ssrane_b23
  1 sibling, 0 replies; 3+ messages in thread
From: Pauli Virtanen @ 2025-11-05 19:36 UTC (permalink / raw)
  To: ssrane_b23
  Cc: linux-bluetooth, linux-kernel, linux-kernel-mentees, skhan,
	david.hunter.linux, khalid

ke, 2025-11-05 kello 19:52 +0530, ssrane_b23@ee.vjti.ac.in kirjoitti:
> From: Shaurya Rane <ssrane_b23@ee.vjti.ac.in>
> 
> Syzbot reported a use-after-free in l2cap_unregister_user(), caused by
> missing reference counting on the associated hci_dev. If the device is
> unregistered while L2CAP users are still active, l2cap_unregister_user()
> may access a freed hci_dev when taking its lock.
> 
> Fix this by taking a device reference in l2cap_register_user() using
> hci_dev_hold(), and releasing it in l2cap_unregister_user() via
> hci_dev_put(). This ensures the hci_dev remains valid for the lifetime
> of registered L2CAP users.
> 
> Reported-by: syzbot+14b6d57fb728e27ce23c@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=14b6d57fb728e27ce23c
> Fixes: c8992cffbe74 ("Bluetooth: hci_event: Use of a function table to handle Command Complete")
> Signed-off-by: Shaurya Rane <ssrane_b23@ee.vjti.ac.in>
> ---
>  net/bluetooth/l2cap_core.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 805c752ac0a9..6a880f8ab6c2 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1688,6 +1688,11 @@ int l2cap_register_user(struct l2cap_conn *conn, struct l2cap_user *user)
>  	struct hci_dev *hdev = conn->hcon->hdev;
>  	int ret;
>  
> +	/* Hold a reference to hdev to prevent it from being freed while
> +	 * we have registered users.
> +	 */
> +	hci_dev_hold(hdev);
> +
>  	/* We need to check whether l2cap_conn is registered. If it is not, we
>  	 * must not register the l2cap_user. l2cap_conn_del() is unregisters
>  	 * l2cap_conn objects, but doesn't provide its own locking. Instead, it

The old comment here seems out of date since commit ab4eedb790cae,
currently l2cap_conn_del() appears to be using conn->lock to do

	mutex_lock(&conn->lock);
	...
	l2cap_unregister_all_users(conn);
	...
        hci_chan_del(conn->hchan);
        conn->hchan = NULL;
	...
	mutex_unlock(&conn->lock);

so it looks likely also taking conn->lock could avoid the races with
conn->users and conn->hchan.

> @@ -1717,6 +1722,10 @@ int l2cap_register_user(struct l2cap_conn *conn, struct l2cap_user *user)
>  
>  out_unlock:
>  	hci_dev_unlock(hdev);
> +
> +	if (ret)
> +		hci_dev_put(hdev);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(l2cap_register_user);
> @@ -1735,6 +1744,9 @@ void l2cap_unregister_user(struct l2cap_conn *conn, struct l2cap_user *user)
>  
>  out_unlock:
>  	hci_dev_unlock(hdev);
> +
> +	/* Release the reference we took in l2cap_register_user */
> +	hci_dev_put(hdev);
>  }
>  EXPORT_SYMBOL(l2cap_unregister_user);
>  

-- 
Pauli Virtanen

^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH v2 1/1] Bluetooth: L2CAP: Fix use-after-free in l2cap_unregister_user
  2025-11-05 14:22 [PATCH] Bluetooth: L2CAP: Fix use-after-free in l2cap_unregister_user ssrane_b23
  2025-11-05 19:36 ` Pauli Virtanen
@ 2025-11-06 18:20 ` ssrane_b23
  1 sibling, 0 replies; 3+ messages in thread
From: ssrane_b23 @ 2025-11-06 18:20 UTC (permalink / raw)
  To: linux-bluetooth
  Cc: luiz.dentz, marcel, johan.hedberg, pav, linux-kernel,
	linux-kernel-mentees, syzbot+14b6d57fb728e27ce23c, Shaurya Rane

From: Shaurya Rane <ssrane_b23@ee.vjti.ac.in>

After commit ab4eedb790ca ("Bluetooth: L2CAP: Fix corrupted list in
hci_chan_del"), l2cap_conn_del() uses conn->lock to protect access to
conn->users and conn->hchan. However, l2cap_register_user() and
l2cap_unregister_user() still use hci_dev_lock(), creating a race
condition where these functions can access conn->users and conn->hchan
concurrently with l2cap_conn_del().

This can lead to use-after-free and list corruption bugs, as reported
by syzbot.

Fix this by changing l2cap_register_user() and l2cap_unregister_user()
to use conn->lock instead of hci_dev_lock(), ensuring consistent locking
for the l2cap_conn structure.

Reported-by: syzbot+14b6d57fb728e27ce23c@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=14b6d57fb728e27ce23c
Fixes: ab4eedb790ca ("Bluetooth: L2CAP: Fix corrupted list in hci_chan_del")

Changes in v2:
 - Replaced hci_dev_lock()/unlock() with mutex_lock()/unlock(&conn->lock)
   in both l2cap_register_user() and l2cap_unregister_user().
 - Updated comments to match current locking rules.
 - Removed unnecessary hci_dev_hold()/hci_dev_put() usage.

Signed-off-by: Shaurya Rane <ssrane_b23@ee.vjti.ac.in>
---
 net/bluetooth/l2cap_core.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index d08320380ad6..29e78801c507 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1685,17 +1685,15 @@ static void l2cap_info_timeout(struct work_struct *work)
 
 int l2cap_register_user(struct l2cap_conn *conn, struct l2cap_user *user)
 {
-	struct hci_dev *hdev = conn->hcon->hdev;
 	int ret;
 
 	/* We need to check whether l2cap_conn is registered. If it is not, we
-	 * must not register the l2cap_user. l2cap_conn_del() is unregisters
-	 * l2cap_conn objects, but doesn't provide its own locking. Instead, it
-	 * relies on the parent hci_conn object to be locked. This itself relies
-	 * on the hci_dev object to be locked. So we must lock the hci device
-	 * here, too. */
+	 * must not register the l2cap_user. l2cap_conn_del() unregisters
+	 * l2cap_conn objects under conn->lock, and we use the same lock here
+	 * to protect access to conn->users and conn->hchan.
+	 */
 
-	hci_dev_lock(hdev);
+	mutex_lock(&conn->lock);
 
 	if (!list_empty(&user->list)) {
 		ret = -EINVAL;
@@ -1716,16 +1714,14 @@ int l2cap_register_user(struct l2cap_conn *conn, struct l2cap_user *user)
 	ret = 0;
 
 out_unlock:
-	hci_dev_unlock(hdev);
+	mutex_unlock(&conn->lock);
 	return ret;
 }
 EXPORT_SYMBOL(l2cap_register_user);
 
 void l2cap_unregister_user(struct l2cap_conn *conn, struct l2cap_user *user)
 {
-	struct hci_dev *hdev = conn->hcon->hdev;
-
-	hci_dev_lock(hdev);
+	mutex_lock(&conn->lock);
 
 	if (list_empty(&user->list))
 		goto out_unlock;
@@ -1734,7 +1730,7 @@ void l2cap_unregister_user(struct l2cap_conn *conn, struct l2cap_user *user)
 	user->remove(conn, user);
 
 out_unlock:
-	hci_dev_unlock(hdev);
+	mutex_unlock(&conn->lock);
 }
 EXPORT_SYMBOL(l2cap_unregister_user);
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2025-11-06 18:20 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-05 14:22 [PATCH] Bluetooth: L2CAP: Fix use-after-free in l2cap_unregister_user ssrane_b23
2025-11-05 19:36 ` Pauli Virtanen
2025-11-06 18:20 ` [PATCH v2 1/1] " ssrane_b23

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