[PATCH] Bluetooth: RFCOMM: hold listener socket in rfcomm_connect_ind()

[PATCH] Bluetooth: RFCOMM: hold listener socket in rfcomm_connect_ind()

[Zhang Cen]

rfcomm_get_sock_by_channel() returns a listener from rfcomm_sk_list
after dropping rfcomm_sk_list.lock, but it does not hold a reference on
that socket. rfcomm_connect_ind() then locks the listener, queues a
child on it, and may still notify it after unlocking it.

rfcomm_connect_ind()              listener close
1. Look up the parent on          1. close() enters
   rfcomm_sk_list.                   rfcomm_sock_release().
2. Drop rfcomm_sk_list.lock       2. rfcomm_sock_shutdown()
   without pinning the listener.     starts tearing the listener down.
3. Later call lock_sock(parent),  3. rfcomm_sock_kill() unlinks the
   bt_accept_enqueue(parent, ...),   listener and drops its last
   or the deferred-setup callback.   reference.

Take a socket reference on the selected listener before leaving
rfcomm_sk_list.lock, re-check that the parent is still in BT_LISTEN
after locking it, cache the deferred-setup bit while the parent is
locked, and drop the extra reference after the last parent use in
rfcomm_connect_ind().

The buggy scenario involves two paths, with each column showing the order within that path:

  rfcomm_connect_ind():                  listener close:
  1. Call                                1. close() enters
     rfcomm_get_sock_by_channel(BT_LISTEN,       rfcomm_sock_release()
     channel, &src)
  2. Return the matched listener         2. rfcomm_sock_shutdown() runs
     after                                  the BT_LISTEN close path and
     read_unlock(&rfcomm_sk_list.lock)       rfcomm_sock_cleanup_listen(parent)
     without sock_hold()                    sets parent->sk_state =
                                            BT_CLOSED and SOCK_ZAPPED
  3. Later call lock_sock(parent)        3. rfcomm_sock_release() calls
     and bt_accept_enqueue(parent,          sock_orphan(parent) and
     sk, true)                              rfcomm_sock_kill(parent)
  4. After release_sock(parent),         4. rfcomm_sock_kill() unlinks
     read bt_sk(parent)->flags and          the listener from
     may call                               rfcomm_sk_list and sock_put()
     parent->sk_state_change(parent)        can free it

The lookup path drops rfcomm_sk_list.lock without a private reference,
so listener close can mark the socket closed, unlink it, and free it
before rfcomm_connect_ind() reaches lock_sock(), bt_accept_enqueue(), or
its deferred-setup callback.

rfcomm_lock serializes RFCOMM core work but does not serialize userspace
close against rfcomm_sock_release() or rfcomm_sock_kill().

Sanitizer validation reported:
BUG: KASAN: slab-use-after-free in lock_sock_nested()
Read of size 1 at addr ffff88810da92250
Call trace:
  dump_stack_lvl() (?:?)
  print_address_description() (mm/kasan/report.c:373)
  lock_sock_nested() (net/core/sock.c:3780)
  print_report() (?:?)
  __virt_addr_valid() (?:?)
  srso_alias_return_thunk() (arch/x86/include/asm/nospec-branch.h:375)
  kasan_addr_to_slab() (mm/kasan/common.c:45)
  kasan_report() (?:?)
  __kasan_check_byte() (mm/kasan/common.c:571)
  lock_acquire() (?:?)
  rcu_is_watching() (?:?)
  rfcomm_connect_ind() (net/bluetooth/rfcomm/sock.c:952)
  rfcomm_recv_pn() (net/bluetooth/rfcomm/core.c:1432)
  trace_clock_x86_tsc() (arch/x86/kernel/trace_clock.c:14)
  __lock_acquire() (kernel/locking/lockdep.c:5077)
  rfcomm_recv_mcc() (net/bluetooth/rfcomm/core.c:1645)
  do_raw_spin_lock() (?:?)
  mark_held_locks() (kernel/locking/lockdep.c:4308)
  rfcomm_recv_frame() (net/bluetooth/rfcomm/core.c:1738)
  rfcomm_process_rx() (net/bluetooth/rfcomm/core.c:1933)
  rfcomm_run() (net/bluetooth/rfcomm/core.c:2114)
  find_held_lock() (kernel/locking/lockdep.c:5340)
  _raw_spin_unlock_irqrestore() (kernel/locking/spinlock.c:196)
  lockdep_hardirqs_on() (?:?)
  __kthread_parkme() (kernel/kthread.c:259)
  kthread() (?:?)
  _raw_spin_unlock_irq() (kernel/locking/spinlock.c:204)
  ret_from_fork() (?:?)
  __switch_to() (?:?)
  ret_from_fork_asm() (?:?)
  kasan_save_stack() (mm/kasan/common.c:52)
  kasan_save_track() (mm/kasan/common.c:74)
  __kasan_kmalloc() (?:?)
  __kmalloc_noprof() (?:?)
  sk_prot_alloc() (net/core/sock.c:2233)
  sk_alloc() (?:?)
  bt_sock_alloc() (?:?)
  rfcomm_sock_alloc() (net/bluetooth/rfcomm/sock.c:271)
  rfcomm_sock_create() (net/bluetooth/rfcomm/sock.c:305)
  bt_sock_create() (net/bluetooth/af_bluetooth.c:116)
  __sock_create() (?:?)
  __sys_socket() (net/socket.c:1801)
  __x64_sys_socket() (?:?)
  do_syscall_64() (arch/x86/entry/syscall_64.c:87)
  entry_SYSCALL_64_after_hwframe() (?:?)
  kasan_save_free_info() (?:?)
  __kasan_slab_free() (?:?)
  kfree() (?:?)
  __sk_destruct() (net/core/sock.c:2345)
  sk_destruct() (net/core/sock.c:2402)
  __sk_free() (net/core/sock.c:2417)
  sk_free() (net/core/sock.c:2428)
  rfcomm_sock_kill() (net/bluetooth/rfcomm/sock.c:192)
  rfcomm_sock_release() (net/bluetooth/rfcomm/sock.c:912)
  __sock_release() (net/socket.c:713)

Signed-off-by: Zhang Cen <rollkingzzc@gmail.com>

---
diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index be6639cd6f59..677c9cd22fb2 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -122,7 +122,7 @@ static struct sock *__rfcomm_get_listen_sock_by_addr(u8 channel, bdaddr_t *src)
 }
 
 /* Find socket with channel and source bdaddr.
- * Returns closest match.
+ * Returns closest match with an extra reference held.
  */
 static struct sock *rfcomm_get_sock_by_channel(int state, u8 channel, bdaddr_t *src)
 {
@@ -136,15 +136,25 @@ static struct sock *rfcomm_get_sock_by_channel(int state, u8 channel, bdaddr_t *
 
 		if (rfcomm_pi(sk)->channel == channel) {
 			/* Exact match. */
-			if (!bacmp(&rfcomm_pi(sk)->src, src))
+			if (!bacmp(&rfcomm_pi(sk)->src, src)) {
+				sock_hold(sk);
 				break;
+			}
 
 			/* Closest match */
-			if (!bacmp(&rfcomm_pi(sk)->src, BDADDR_ANY))
+			if (!bacmp(&rfcomm_pi(sk)->src, BDADDR_ANY)) {
+				if (sk1)
+					sock_put(sk1);
+
 				sk1 = sk;
+				sock_hold(sk1);
+			}
 		}
 	}
 
+	if (sk && sk1)
+		sock_put(sk1);
+
 	read_unlock(&rfcomm_sk_list.lock);
 
 	return sk ? sk : sk1;
@@ -934,6 +944,7 @@ int rfcomm_connect_ind(struct rfcomm_session *s, u8 channel, struct rfcomm_dlc *
 {
 	struct sock *sk, *parent;
 	bdaddr_t src, dst;
+	bool defer_setup = false;
 	int result = 0;
 
 	BT_DBG("session %p channel %d", s, channel);
@@ -947,6 +958,11 @@ int rfcomm_connect_ind(struct rfcomm_session *s, u8 channel, struct rfcomm_dlc *
 
 	lock_sock(parent);
 
+	if (parent->sk_state != BT_LISTEN)
+		goto done;
+
+	defer_setup = test_bit(BT_SK_DEFER_SETUP, &bt_sk(parent)->flags);
+
 	/* Check for backlog size */
 	if (sk_acceptq_is_full(parent)) {
 		BT_DBG("backlog full %d", parent->sk_ack_backlog);
@@ -974,9 +990,11 @@ int rfcomm_connect_ind(struct rfcomm_session *s, u8 channel, struct rfcomm_dlc *
 done:
 	release_sock(parent);
 
-	if (test_bit(BT_SK_DEFER_SETUP, &bt_sk(parent)->flags))
+	if (defer_setup)
 		parent->sk_state_change(parent);
 
+	sock_put(parent);
+
 	return result;
 }

RE: Bluetooth: RFCOMM: hold listener socket in rfcomm_connect_ind()

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2465 bytes --]

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=1092078

---Test result---

Test Summary:
CheckPatch                    FAIL      0.77 seconds
GitLint                       FAIL      0.34 seconds
SubjectPrefix                 PASS      0.13 seconds
BuildKernel                   PASS      24.98 seconds
CheckAllWarning               PASS      27.45 seconds
CheckSparse                   PASS      26.44 seconds
BuildKernel32                 PASS      24.22 seconds
TestRunnerSetup               PASS      518.00 seconds
TestRunner_rfcomm-tester      PASS      63.49 seconds
IncrementalBuild              PASS      23.77 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
Bluetooth: RFCOMM: hold listener socket in rfcomm_connect_ind()
WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
#119: 
The buggy scenario involves two paths, with each column showing the order within that path:

WARNING: The commit message has 'BUG: KASAN: ', perhaps it also needs a 'Fixes:' tag?

total: 0 errors, 2 warnings, 0 checks, 65 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/patch/14563150.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
Bluetooth: RFCOMM: hold listener socket in rfcomm_connect_ind()

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
23: B1 Line exceeds max length (91>80): "The buggy scenario involves two paths, with each column showing the order within that path:"


https://github.com/bluez/bluetooth-next/pull/160

---
Regards,
Linux Bluetooth