[PATCH v1] Bluetooth: mgmt: fix 'hdev->discovery.uuids' NULL dereference

[PATCH v1] Bluetooth: mgmt: fix 'hdev->discovery.uuids' NULL dereference

[Pavel Shpakovskiy]

'uuid_count' member of struct 'discovery_state' is assigned and read
without any locks, so there is a chance of situation when
uuid_count != 0, but uuids is NULL and there will be NULL pointer
dereference.

Possible race:
'hci_update_passive_scan_sync'
  'hci_discovery_filter_clear'
    hdev->discovery.uuid_count = 0;
      <----------------------preempted----------------------------->
                        'start_service_discovery'
                          // Set uuid_count to value != 0
                          hdev->discovery.uuid_count = uuid_count;
                          hdev->discovery.uuids = kmemdup(...);
      <----------------------preempted----------------------------->
    spin_lock(&hdev->discovery.lock);
    kfree(hdev->discovery.uuids);
    hdev->discovery.uuids = NULL;
    spin_unlock(&hdev->discovery.lock);

Now uuids == NULL and uuid_count != 0.
So 'mgmt_device_found' -> 'is_filter_match' -> 'eir_has_uuids' receives
non consistent discovery state, where NULL dereference of uuids happens.

To fix it let's add discovery.lock around every read/write of uuid_count,
uuids pair of struct members. It is also important to assign uuid_count
value only after success kmemdup() allocation in
start_service_discovery(), otherwise uuids is NULL, because kmemdup failed,
but uuid_count is already assigned to non zero value.

The following panic happens:

[ ] ------------[ cut here ]------------
[ ] Unable to handle kernel NULL pointer dereference at virtual
address 0000000000000000
[ ] Internal error: Oops: 0000000096000006 [#1] PREEMPT SMP
[ ] CPU: 0 PID: 15056 Comm: kworker/u9:2
[ ] Workqueue: hci0 hci_rx_work
[ ] pstate: 10400009 (nzcV daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ ] pc : eir_has_uuids+0x2d8/0x590
[ ] lr : is_filter_match+0x258/0x320
...
[ ] Call trace:
[ ]  eir_has_uuids+0x2d8/0x590
[ ]  is_filter_match+0x258/0x320
[ ]  mgmt_device_found+0x5b0/0xafc
[ ]  process_adv_report.part.0+0x8c8/0xf14
[ ]  hci_le_adv_report_evt+0x338/0x3f0
[ ]  hci_le_meta_evt+0x1f0/0x4c8
[ ]  hci_event_packet+0x440/0xc9c
[ ]  hci_rx_work+0x44c/0xaf8
[ ]  process_one_work+0x54c/0x103c
[ ]  worker_thread+0x6c4/0x10c4
[ ]  kthread+0x274/0x2ec
[ ]  ret_from_fork+0x10/0x20
[ ] Code: 14000004 91004021 eb14003f 54000180 (f9400024)
[ ] ---[ end trace 0000000000000000 ]---

Fixes: 2935e556850e ("Bluetooth: hci_sync: fix double free in 'hci_discovery_filter_clear()'")
Signed-off-by: Pavel Shpakovskiy <pashpakovskii@salutedevices.com>
---
 include/net/bluetooth/hci_core.h |  2 +-
 net/bluetooth/mgmt.c             | 13 +++++++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index a7bffb908c1ec..f14239d1a817e 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -920,9 +920,9 @@ static inline void hci_discovery_filter_clear(struct hci_dev *hdev)
 	hdev->discovery.result_filtering = false;
 	hdev->discovery.report_invalid_rssi = true;
 	hdev->discovery.rssi = HCI_RSSI_INVALID;
-	hdev->discovery.uuid_count = 0;
 
 	spin_lock(&hdev->discovery.lock);
+	hdev->discovery.uuid_count = 0;
 	kfree(hdev->discovery.uuids);
 	hdev->discovery.uuids = NULL;
 	spin_unlock(&hdev->discovery.lock);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index de5bd6b637b20..7368a4ac0c839 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -6164,12 +6164,14 @@ static int start_service_discovery(struct sock *sk, struct hci_dev *hdev,
 	hdev->discovery.result_filtering = true;
 	hdev->discovery.type = cp->type;
 	hdev->discovery.rssi = cp->rssi;
-	hdev->discovery.uuid_count = uuid_count;
+
+	spin_lock(&hdev->discovery.lock);
 
 	if (uuid_count > 0) {
 		hdev->discovery.uuids = kmemdup(cp->uuids, uuid_count * 16,
 						GFP_KERNEL);
 		if (!hdev->discovery.uuids) {
+			spin_unlock(&hdev->discovery.lock);
 			err = mgmt_cmd_complete(sk, hdev->id,
 						MGMT_OP_START_SERVICE_DISCOVERY,
 						MGMT_STATUS_FAILED,
@@ -6179,6 +6181,9 @@ static int start_service_discovery(struct sock *sk, struct hci_dev *hdev,
 		}
 	}
 
+	hdev->discovery.uuid_count = uuid_count;
+	spin_unlock(&hdev->discovery.lock);
+
 	err = hci_cmd_sync_queue(hdev, start_discovery_sync, cmd,
 				 start_discovery_complete);
 	if (err < 0) {
@@ -10243,6 +10248,7 @@ static bool is_filter_match(struct hci_dev *hdev, s8 rssi, u8 *eir,
 	     !hci_test_quirk(hdev, HCI_QUIRK_STRICT_DUPLICATE_FILTER))))
 		return  false;
 
+	spin_lock(&hdev->discovery.lock);
 	if (hdev->discovery.uuid_count != 0) {
 		/* If a list of UUIDs is provided in filter, results with no
 		 * matching UUID should be dropped.
@@ -10251,9 +10257,12 @@ static bool is_filter_match(struct hci_dev *hdev, s8 rssi, u8 *eir,
 				   hdev->discovery.uuids) &&
 		    !eir_has_uuids(scan_rsp, scan_rsp_len,
 				   hdev->discovery.uuid_count,
-				   hdev->discovery.uuids))
+				   hdev->discovery.uuids)) {
+			spin_unlock(&hdev->discovery.lock);
 			return false;
+		}
 	}
+	spin_unlock(&hdev->discovery.lock);
 
 	/* If duplicate filtering does not report RSSI changes, then restart
 	 * scanning to ensure updated result with updated RSSI values.
-- 
2.43.0

RE: [v1] Bluetooth: mgmt: fix 'hdev->discovery.uuids' NULL dereference

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 4122 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=1142386

---Test result---

Test Summary:
CheckPatch                    PASS      0.80 seconds
VerifyFixes                   PASS      0.11 seconds
VerifySignedoff               PASS      0.10 seconds
GitLint                       PASS      0.26 seconds
SubjectPrefix                 PASS      0.55 seconds
BuildKernel                   PASS      24.26 seconds
CheckAllWarning               PASS      26.68 seconds
CheckSparse                   PASS      25.40 seconds
BuildKernel32                 PASS      23.34 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      450.69 seconds
TestRunner_l2cap-tester       PASS      63.40 seconds
TestRunner_iso-tester         PASS      79.90 seconds
TestRunner_bnep-tester        PASS      18.53 seconds
TestRunner_mgmt-tester        FAIL      219.06 seconds
TestRunner_rfcomm-tester      PASS      25.06 seconds
TestRunner_sco-tester         PASS      30.81 seconds
TestRunner_ioctl-tester       PASS      26.04 seconds
TestRunner_mesh-tester        FAIL      25.87 seconds
TestRunner_smp-tester         PASS      23.13 seconds
TestRunner_userchan-tester    PASS      19.44 seconds
TestRunner_6lowpan-tester     PASS      22.71 seconds
IncrementalBuild              PASS      22.52 seconds

Details
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
BUG: sleeping function called from invalid context at ./include/linux/sched/mm.h:323
in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 37, name: memcheck-amd64-
preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
4 locks held by memcheck-amd64-/37:
 #0: ffff88800272c240 (sk_lock-AF_BLUETOOTH-BTPROTO_HCI){+.+.}-{0:0}, at: hci_sock_sendmsg+0x409/0x2140
 #1: ffffffff87ecdf58 (mgmt_chan_list_lock){+.+.}-{4:4}, at: hci_sock_sendmsg+0xb95/0x2140
 #2: ffff8880028540b0 (&hdev->lock){+.+.}-{4:4}, at: start_service_discovery+0xcc/0xa70
 #3: ffff888002855098 (&hdev->discovery.lock){+.+.}-{3:3}, at: start_service_discovery+0x77d/0xa70
CPU: 0 UID: 0 PID: 37 Comm: memcheck-amd64- Not tainted 7.1.0-rc6-gac83b5e5eef3 #1 PREEMPT(lazy) 
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-1ubuntu1.1 04/01/2014
Call Trace:
 <TASK>
 dump_stack_lvl+0x49/0x60
 __might_resched+0x2ea/0x500
 ? start_service_discovery+0x79f/0xa70
 __kmalloc_node_track_caller_noprof+0x2c3/0x560
 kmemdup_noprof+0x21/0x50
 start_service_discovery+0x79f/0xa70
 ? __pfx_start_service_discovery+0x10/0x10
 ? _raw_read_unlock+0x1e/0x40
 hci_sock_sendmsg+0x193e/0x2140
 ? check_irq_usage+0x5d4/0x9f0
 ? __pfx_hci_sock_sendmsg+0x10/0x10
 ? __might_fault+0x112/0x170
 ? lock_release+0xc6/0x260
 sock_write_iter+0x43e/0x4e0
 ? __pfx_sock_write_iter+0x10/0x10
 ? __wake_up_common_lock+0x41/0x60
 ? lock_release+0xc6/0x260
 do_iter_readv_writev+0x3cb/0x780
 ? __wake_up_common_lock+0x41/0x60
 ? __pfx_do_iter_readv_writev+0x10/0x10
 ? __import_iovec+0x382/0x5d0
 vfs_writev+0x20d/0xb70
 ? __pfx_vfs_writev+0x10/0x10
 ? __lock_acquire+0x50f/0x1df0
 ? posixtimer_queue_sigqueue+0x171/0x230
 ? __might_fault+0xbb/0x170
 ? find_held_lock+0x2b/0x80
...
Total: 501, Passed: 496 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
Read Exp Feature - Success                           Failed       0.248 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Timed out    2.647 seconds
Mesh - Send cancel - 2                               Timed out    1.982 seconds


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

---
Regards,
Linux Bluetooth