'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 --- Changelog v1->v2: * Sleeping function was called from invalid context, need to allocate uuids before acquiring discovery.lock.
include/net/bluetooth/hci_core.h | 2 +- net/bluetooth/mgmt.c | 18 +++++++++++++----- 2 files changed, 14 insertions(+), 6 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..9917710f2ff56 100644 --- a/net/bluetooth/mgmt.c +++ b/net/bluetooth/mgmt.c @@ -6088,6 +6088,7 @@ static int start_service_discovery(struct sock *sk, struct hci_dev *hdev, struct mgmt_pending_cmd *cmd; const u16 max_uuid_count = ((U16_MAX - sizeof(*cp)) / 16); u16 uuid_count, expected_len; + u8 (*uuids)[16] = NULL; u8 status; int err;
@@ -6164,12 +6165,10 @@ 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;
if (uuid_count > 0) { - hdev->discovery.uuids = kmemdup(cp->uuids, uuid_count * 16, - GFP_KERNEL); - if (!hdev->discovery.uuids) { + uuids = kmemdup(cp->uuids, uuid_count * sizeof(*uuids), GFP_KERNEL); + if (!uuids) { err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_START_SERVICE_DISCOVERY, MGMT_STATUS_FAILED, @@ -6179,6 +6178,11 @@ static int start_service_discovery(struct sock *sk, struct hci_dev *hdev, } }
+ spin_lock(&hdev->discovery.lock); + hdev->discovery.uuids = uuids; + 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 +10247,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 +10256,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.