[v3] Bluetooth: Fix possible deadlock in rfcomm_sk_state_change

Commit Message

Ying Hsu Jan. 11, 2023, 3:16 a.m. UTC

syzbot reports a possible deadlock in rfcomm_sk_state_change [1].
While rfcomm_sock_connect acquires the sk lock and waits for
the rfcomm lock, rfcomm_sock_release could have the rfcomm
lock and hit a deadlock for acquiring the sk lock.
Here's a simplified flow:

rfcomm_sock_connect:
  lock_sock(sk)
  rfcomm_dlc_open:
    rfcomm_lock()

rfcomm_sock_release:
  rfcomm_sock_shutdown:
    rfcomm_lock()
    __rfcomm_dlc_close:
        rfcomm_k_state_change:
	  lock_sock(sk)

This patch drops the sk lock before calling rfcomm_dlc_open to
avoid the possible deadlock and holds sk's reference count to
prevent use-after-free after rfcomm_dlc_open completes.

Reported-by: [e-mail]
Fixes: 1804fdf6e494 ("Bluetooth: btintel: Combine setting up MSFT extension")
Link: https://syzkaller.appspot.com/bug?extid=d7ce59b06b3eb14fd218 [1]

Signed-off-by: Ying Hsu <[e-mail]>
---
This commit has been tested with a C reproducer on qemu-x86_64
and a ChromeOS device.

Changes in v3:
- Revise the commit message.

Changes in v2:
- Fix potential use-after-free in rfc_comm_sock_connect.

 net/bluetooth/rfcomm/sock.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

[e-mail] Jan. 11, 2023, 3:57 a.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=710770

---Test result---

Test Summary:
CheckPatch                    PASS      0.77 seconds
GitLint                       FAIL      0.53 seconds
SubjectPrefix                 PASS      0.05 seconds
BuildKernel                   PASS      30.88 seconds
CheckAllWarning               PASS      33.71 seconds
CheckSparse                   PASS      38.46 seconds
CheckSmatch                   PASS      106.19 seconds
BuildKernel32                 PASS      29.67 seconds
TestRunnerSetup               PASS      427.02 seconds
TestRunner_l2cap-tester       PASS      15.97 seconds
TestRunner_iso-tester         PASS      16.16 seconds
TestRunner_bnep-tester        PASS      5.52 seconds
TestRunner_mgmt-tester        PASS      105.33 seconds
TestRunner_rfcomm-tester      PASS      8.60 seconds
TestRunner_sco-tester         PASS      7.89 seconds
TestRunner_ioctl-tester       PASS      9.18 seconds
TestRunner_mesh-tester        PASS      6.85 seconds
TestRunner_smp-tester         PASS      7.81 seconds
TestRunner_userchan-tester    PASS      5.69 seconds
IncrementalBuild              PASS      27.91 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v3] Bluetooth: Fix possible deadlock in rfcomm_sk_state_change

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
19: B3 Line contains hard tab characters (\t): "	  lock_sock(sk)"


---
Regards,
Linux Bluetooth

[e-mail] Jan. 11, 2023, 8:10 p.m. UTC | #2

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <[e-mail]>:

On Wed, 11 Jan 2023 03:16:14 +0000 you wrote:
> syzbot reports a possible deadlock in rfcomm_sk_state_change [1].
> While rfcomm_sock_connect acquires the sk lock and waits for
> the rfcomm lock, rfcomm_sock_release could have the rfcomm
> lock and hit a deadlock for acquiring the sk lock.
> Here's a simplified flow:
> 
> rfcomm_sock_connect:
>   lock_sock(sk)
>   rfcomm_dlc_open:
>     rfcomm_lock()
> 
> [...]

Here is the summary with links:
  - [v3] Bluetooth: Fix possible deadlock in rfcomm_sk_state_change
    https://git.kernel.org/bluetooth/bluetooth-next/c/7ed38304a633

You are awesome, thank you!

Patch

diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index 21e24da4847f..4397e14ff560 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -391,6 +391,7 @@  static int rfcomm_sock_connect(struct socket *sock, struct sockaddr *addr, int a
 	    addr->sa_family != AF_BLUETOOTH)
 		return -EINVAL;
 
+	sock_hold(sk);
 	lock_sock(sk);
 
 	if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND) {
@@ -410,14 +411,18 @@  static int rfcomm_sock_connect(struct socket *sock, struct sockaddr *addr, int a
 	d->sec_level = rfcomm_pi(sk)->sec_level;
 	d->role_switch = rfcomm_pi(sk)->role_switch;
 
+	/* Drop sock lock to avoid potential deadlock with the RFCOMM lock */
+	release_sock(sk);
 	err = rfcomm_dlc_open(d, &rfcomm_pi(sk)->src, &sa->rc_bdaddr,
 			      sa->rc_channel);
-	if (!err)
+	lock_sock(sk);
+	if (!err && !sock_flag(sk, SOCK_ZAPPED))
 		err = bt_sock_wait_state(sk, BT_CONNECTED,
 				sock_sndtimeo(sk, flags & O_NONBLOCK));
 
 done:
 	release_sock(sk);
+	sock_put(sk);
 	return err;
 }