From ed2a2ef16a6b9197a0e452308bf6acee6e01f709 Mon Sep 17 00:00:00 2001 From: Sven Peter Date: Wed, 15 May 2024 18:02:58 +0000 Subject: Bluetooth: Add quirk to ignore reserved PHY bits in LE Extended Adv Report Some Broadcom controllers found on Apple Silicon machines abuse the reserved bits inside the PHY fields of LE Extended Advertising Report events for additional flags. Add a quirk to drop these and correctly extract the Primary/Secondary_PHY field. The following excerpt from a btmon trace shows a report received with "Reserved" for "Primary PHY" on a 4388 controller: > HCI Event: LE Meta Event (0x3e) plen 26 LE Extended Advertising Report (0x0d) Num reports: 1 Entry 0 Event type: 0x2515 Props: 0x0015 Connectable Directed Use legacy advertising PDUs Data status: Complete Reserved (0x2500) Legacy PDU Type: Reserved (0x2515) Address type: Random (0x01) Address: 00:00:00:00:00:00 (Static) Primary PHY: Reserved Secondary PHY: No packets SID: no ADI field (0xff) TX power: 127 dBm RSSI: -60 dBm (0xc4) Periodic advertising interval: 0.00 msec (0x0000) Direct address type: Public (0x00) Direct address: 00:00:00:00:00:00 (Apple, Inc.) Data length: 0x00 Cc: stable@vger.kernel.org Fixes: 2e7ed5f5e69b ("Bluetooth: hci_sync: Use advertised PHYs on hci_le_ext_create_conn_sync") Reported-by: Janne Grunau Closes: https://lore.kernel.org/all/Zjz0atzRhFykROM9@robin Tested-by: Janne Grunau Signed-off-by: Sven Peter Signed-off-by: Luiz Augusto von Dentz --- drivers/bluetooth/hci_bcm4377.c | 8 ++++++++ include/net/bluetooth/hci.h | 11 +++++++++++ net/bluetooth/hci_event.c | 7 +++++++ 3 files changed, 26 insertions(+) diff --git a/drivers/bluetooth/hci_bcm4377.c b/drivers/bluetooth/hci_bcm4377.c index 0c2f15235b4c..94e736595443 100644 --- a/drivers/bluetooth/hci_bcm4377.c +++ b/drivers/bluetooth/hci_bcm4377.c @@ -495,6 +495,10 @@ struct bcm4377_data; * extended scanning * broken_mws_transport_config: Set to true if the chip erroneously claims to * support MWS Transport Configuration + * broken_le_ext_adv_report_phy: Set to true if this chip stuffs flags inside + * reserved bits of Primary/Secondary_PHY inside + * LE Extended Advertising Report events which + * have to be ignored * send_calibration: Optional callback to send calibration data * send_ptb: Callback to send "PTB" regulatory/calibration data */ @@ -513,6 +517,7 @@ struct bcm4377_hw { unsigned long broken_ext_scan : 1; unsigned long broken_mws_transport_config : 1; unsigned long broken_le_coded : 1; + unsigned long broken_le_ext_adv_report_phy : 1; int (*send_calibration)(struct bcm4377_data *bcm4377); int (*send_ptb)(struct bcm4377_data *bcm4377, @@ -2373,6 +2378,8 @@ static int bcm4377_probe(struct pci_dev *pdev, const struct pci_device_id *id) set_bit(HCI_QUIRK_BROKEN_EXT_SCAN, &hdev->quirks); if (bcm4377->hw->broken_le_coded) set_bit(HCI_QUIRK_BROKEN_LE_CODED, &hdev->quirks); + if (bcm4377->hw->broken_le_ext_adv_report_phy) + set_bit(HCI_QUIRK_FIXUP_LE_EXT_ADV_REPORT_PHY, &hdev->quirks); pci_set_drvdata(pdev, bcm4377); hci_set_drvdata(hdev, bcm4377); @@ -2477,6 +2484,7 @@ static const struct bcm4377_hw bcm4377_hw_variants[] = { .clear_pciecfg_subsystem_ctrl_bit19 = true, .broken_mws_transport_config = true, .broken_le_coded = true, + .broken_le_ext_adv_report_phy = true, .send_calibration = bcm4387_send_calibration, .send_ptb = bcm4378_send_ptb, }, diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h index fe932ca3bc8c..e372a88e8c3f 100644 --- a/include/net/bluetooth/hci.h +++ b/include/net/bluetooth/hci.h @@ -324,6 +324,17 @@ enum { * claim to support it. */ HCI_QUIRK_BROKEN_READ_ENC_KEY_SIZE, + + /* + * When this quirk is set, the reserved bits of Primary/Secondary_PHY + * inside the LE Extended Advertising Report events are discarded. + * This is required for some Apple/Broadcom controllers which + * abuse these reserved bits for unrelated flags. + * + * This quirk can be set before hci_register_dev is called or + * during the hdev->setup vendor callback. + */ + HCI_QUIRK_FIXUP_LE_EXT_ADV_REPORT_PHY, }; /* HCI device flags */ diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index a487f9df8145..da10738a052d 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -6311,6 +6311,13 @@ static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, void *data, evt_type = __le16_to_cpu(info->type) & LE_EXT_ADV_EVT_TYPE_MASK; legacy_evt_type = ext_evt_type_to_legacy(hdev, evt_type); + + if (test_bit(HCI_QUIRK_FIXUP_LE_EXT_ADV_REPORT_PHY, + &hdev->quirks)) { + info->primary_phy &= 0x1f; + info->secondary_phy &= 0x1f; + } + if (legacy_evt_type != LE_ADV_INVALID) { process_adv_report(hdev, legacy_evt_type, &info->bdaddr, info->bdaddr_type, NULL, 0, -- cgit From 897e6120566f1c108b85fefe78d1c1bddfbd5988 Mon Sep 17 00:00:00 2001 From: Hector Martin Date: Wed, 15 May 2024 18:15:04 +0000 Subject: Bluetooth: hci_bcm4377: Fix msgid release We are releasing a single msgid, so the order argument to bitmap_release_region must be zero. Fixes: 8a06127602de ("Bluetooth: hci_bcm4377: Add new driver for BCM4377 PCIe boards") Cc: stable@vger.kernel.org Signed-off-by: Hector Martin Reviewed-by: Sven Peter Reviewed-by: Neal Gompa Signed-off-by: Sven Peter Signed-off-by: Luiz Augusto von Dentz --- drivers/bluetooth/hci_bcm4377.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/bluetooth/hci_bcm4377.c b/drivers/bluetooth/hci_bcm4377.c index 94e736595443..d90858ea2fe5 100644 --- a/drivers/bluetooth/hci_bcm4377.c +++ b/drivers/bluetooth/hci_bcm4377.c @@ -721,7 +721,7 @@ static void bcm4377_handle_ack(struct bcm4377_data *bcm4377, ring->events[msgid] = NULL; } - bitmap_release_region(ring->msgids, msgid, ring->n_entries); + bitmap_release_region(ring->msgids, msgid, 0); unlock: spin_unlock_irqrestore(&ring->lock, flags); -- cgit From 3f35d9b3e9a0e3c4684252ab91e648b96a179482 Mon Sep 17 00:00:00 2001 From: Vijay Satija Date: Fri, 17 May 2024 15:24:47 +0530 Subject: Bluetooth: btintel_pcie: Fix REVERSE_INULL issue reported by coverity check pdata return of skb_pull_data, instead of data. Fixes: c2b636b3f788 ("Bluetooth: btintel_pcie: Add support for PCIe transport") Signed-off-by: Vijay Satija Signed-off-by: Kiran K Signed-off-by: Luiz Augusto von Dentz --- drivers/bluetooth/btintel_pcie.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c index 5b6805d87fcf..dd3c0626c72d 100644 --- a/drivers/bluetooth/btintel_pcie.c +++ b/drivers/bluetooth/btintel_pcie.c @@ -382,7 +382,7 @@ static int btintel_pcie_recv_frame(struct btintel_pcie_data *data, /* The first 4 bytes indicates the Intel PCIe specific packet type */ pdata = skb_pull_data(skb, BTINTEL_PCIE_HCI_TYPE_LEN); - if (!data) { + if (!pdata) { bt_dev_err(hdev, "Corrupted packet received"); ret = -EILSEQ; goto exit_error; -- cgit From ac65ecccae802417ce42e857defacad60e4b8329 Mon Sep 17 00:00:00 2001 From: Luiz Augusto von Dentz Date: Thu, 6 Jun 2024 12:04:30 -0400 Subject: Bluetooth: hci_event: Fix setting of unicast qos interval MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit qos->ucast interval reffers to the SDU interval, and should not be set to the interval value reported by the LE CIS Established event since the latter reffers to the ISO interval. These two interval are not the same thing: BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 6, Part G Isochronous interval: The time between two consecutive BIS or CIS events (designated ISO_Interval in the Link Layer) SDU interval: The nominal time between two consecutive SDUs that are sent or received by the upper layer. So this instead uses the following formula from the spec to calculate the resulting SDU interface: BLUETOOTH CORE SPECIFICATION Version 5.4 | Vol 6, Part G page 3075: Transport_Latency_C_To_P = CIG_Sync_Delay + (FT_C_To_P) × ISO_Interval + SDU_Interval_C_To_P Transport_Latency_P_To_C = CIG_Sync_Delay + (FT_P_To_C) × ISO_Interval + SDU_Interval_P_To_C Link: https://github.com/bluez/bluez/issues/823 Fixes: 2be22f1941d5 ("Bluetooth: hci_event: Fix parsing of CIS Established Event") Signed-off-by: Luiz Augusto von Dentz --- net/bluetooth/hci_event.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index da10738a052d..5d23bcf18716 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -6667,6 +6667,7 @@ static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data, struct bt_iso_qos *qos; bool pending = false; u16 handle = __le16_to_cpu(ev->handle); + u32 c_sdu_interval, p_sdu_interval; bt_dev_dbg(hdev, "status 0x%2.2x", ev->status); @@ -6691,12 +6692,25 @@ static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data, pending = test_and_clear_bit(HCI_CONN_CREATE_CIS, &conn->flags); - /* Convert ISO Interval (1.25 ms slots) to SDU Interval (us) */ - qos->ucast.in.interval = le16_to_cpu(ev->interval) * 1250; - qos->ucast.out.interval = qos->ucast.in.interval; + /* BLUETOOTH CORE SPECIFICATION Version 5.4 | Vol 6, Part G + * page 3075: + * Transport_Latency_C_To_P = CIG_Sync_Delay + (FT_C_To_P) × + * ISO_Interval + SDU_Interval_C_To_P + * ... + * SDU_Interval = (CIG_Sync_Delay + (FT) x ISO_Interval) - + * Transport_Latency + */ + c_sdu_interval = (get_unaligned_le24(ev->cig_sync_delay) + + (ev->c_ft * le16_to_cpu(ev->interval) * 1250)) - + get_unaligned_le24(ev->c_latency); + p_sdu_interval = (get_unaligned_le24(ev->cig_sync_delay) + + (ev->p_ft * le16_to_cpu(ev->interval) * 1250)) - + get_unaligned_le24(ev->p_latency); switch (conn->role) { case HCI_ROLE_SLAVE: + qos->ucast.in.interval = c_sdu_interval; + qos->ucast.out.interval = p_sdu_interval; /* Convert Transport Latency (us) to Latency (msec) */ qos->ucast.in.latency = DIV_ROUND_CLOSEST(get_unaligned_le24(ev->c_latency), @@ -6710,6 +6724,8 @@ static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data, qos->ucast.out.phy = ev->p_phy; break; case HCI_ROLE_MASTER: + qos->ucast.in.interval = p_sdu_interval; + qos->ucast.out.interval = c_sdu_interval; /* Convert Transport Latency (us) to Latency (msec) */ qos->ucast.out.latency = DIV_ROUND_CLOSEST(get_unaligned_le24(ev->c_latency), -- cgit From 88e72239ead9814b886db54fc4ee39ef3c2b8f26 Mon Sep 17 00:00:00 2001 From: Zijun Hu Date: Thu, 16 May 2024 21:31:34 +0800 Subject: Bluetooth: qca: Fix BT enable failure again for QCA6390 after warm reboot Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed serdev") will cause below regression issue: BT can't be enabled after below steps: cold boot -> enable BT -> disable BT -> warm reboot -> BT enable failure if property enable-gpios is not configured within DT|ACPI for QCA6390. The commit is to fix a use-after-free issue within qca_serdev_shutdown() by adding condition to avoid the serdev is flushed or wrote after closed but also introduces this regression issue regarding above steps since the VSC is not sent to reset controller during warm reboot. Fixed by sending the VSC to reset controller within qca_serdev_shutdown() once BT was ever enabled, and the use-after-free issue is also fixed by this change since the serdev is still opened before it is flushed or wrote. Verified by the reported machine Dell XPS 13 9310 laptop over below two kernel commits: commit e00fc2700a3f ("Bluetooth: btusb: Fix triggering coredump implementation for QCA") of bluetooth-next tree. commit b23d98d46d28 ("Bluetooth: btusb: Fix triggering coredump implementation for QCA") of linus mainline tree. Fixes: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed serdev") Cc: stable@vger.kernel.org Reported-by: Wren Turkal Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218726 Signed-off-by: Zijun Hu Tested-by: Wren Turkal Signed-off-by: Luiz Augusto von Dentz --- drivers/bluetooth/hci_qca.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c index 0c9c9ee56592..9a0bc86f9aac 100644 --- a/drivers/bluetooth/hci_qca.c +++ b/drivers/bluetooth/hci_qca.c @@ -2450,15 +2450,27 @@ static void qca_serdev_shutdown(struct device *dev) struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev); struct hci_uart *hu = &qcadev->serdev_hu; struct hci_dev *hdev = hu->hdev; - struct qca_data *qca = hu->priv; const u8 ibs_wake_cmd[] = { 0xFD }; const u8 edl_reset_soc_cmd[] = { 0x01, 0x00, 0xFC, 0x01, 0x05 }; if (qcadev->btsoc_type == QCA_QCA6390) { - if (test_bit(QCA_BT_OFF, &qca->flags) || - !test_bit(HCI_RUNNING, &hdev->flags)) + /* The purpose of sending the VSC is to reset SOC into a initial + * state and the state will ensure next hdev->setup() success. + * if HCI_QUIRK_NON_PERSISTENT_SETUP is set, it means that + * hdev->setup() can do its job regardless of SoC state, so + * don't need to send the VSC. + * if HCI_SETUP is set, it means that hdev->setup() was never + * invoked and the SOC is already in the initial state, so + * don't also need to send the VSC. + */ + if (test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks) || + hci_dev_test_flag(hdev, HCI_SETUP)) return; + /* The serdev must be in open state when conrol logic arrives + * here, so also fix the use-after-free issue caused by that + * the serdev is flushed or wrote after it is closed. + */ serdev_device_write_flush(serdev); ret = serdev_device_write_buf(serdev, ibs_wake_cmd, sizeof(ibs_wake_cmd)); -- cgit From 0d151a103775dd9645c78c97f77d6e2a5298d913 Mon Sep 17 00:00:00 2001 From: Tetsuo Handa Date: Mon, 10 Jun 2024 20:00:32 +0900 Subject: Bluetooth: hci_core: cancel all works upon hci_unregister_dev() syzbot is reporting that calling hci_release_dev() from hci_error_reset() due to hci_dev_put() from hci_error_reset() can cause deadlock at destroy_workqueue(), for hci_error_reset() is called from hdev->req_workqueue which destroy_workqueue() needs to flush. We need to make sure that hdev->{rx_work,cmd_work,tx_work} which are queued into hdev->workqueue and hdev->{power_on,error_reset} which are queued into hdev->req_workqueue are no longer running by the moment destroy_workqueue(hdev->workqueue); destroy_workqueue(hdev->req_workqueue); are called from hci_release_dev(). Call cancel_work_sync() on these work items from hci_unregister_dev() as soon as hdev->list is removed from hci_dev_list. Reported-by: syzbot Closes: https://syzkaller.appspot.com/bug?extid=da0a9c9721e36db712e8 Signed-off-by: Tetsuo Handa Signed-off-by: Luiz Augusto von Dentz --- net/bluetooth/hci_core.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index dd3b0f501018..dbbe5e2da210 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -2751,7 +2751,11 @@ void hci_unregister_dev(struct hci_dev *hdev) list_del(&hdev->list); write_unlock(&hci_dev_list_lock); + cancel_work_sync(&hdev->rx_work); + cancel_work_sync(&hdev->cmd_work); + cancel_work_sync(&hdev->tx_work); cancel_work_sync(&hdev->power_on); + cancel_work_sync(&hdev->error_reset); hci_cmd_sync_clear(hdev); -- cgit From 4183a7be77009fc31c5760429fe095f163bf96a9 Mon Sep 17 00:00:00 2001 From: Neeraj Sanjay Kale Date: Fri, 14 Jun 2024 13:50:39 +0530 Subject: Bluetooth: btnxpuart: Enable Power Save feature on startup This sets the default power save mode setting to enabled. The power save feature is now stable and stress test issues, such as the TX timeout error, have been resolved. commit c7ee0bc8db32 ("Bluetooth: btnxpuart: Resolve TX timeout error in power save stress test") With this setting, the driver will send the vendor command to FW at startup, to enable power save feature. User can disable this feature using the following vendor command: hcitool cmd 3f 23 03 00 00 (HCI_NXP_AUTO_SLEEP_MODE) Signed-off-by: Neeraj Sanjay Kale Reviewed-by: Paul Menzel Signed-off-by: Luiz Augusto von Dentz --- drivers/bluetooth/btnxpuart.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c index 9d0c7e278114..9bfa9a6ad56c 100644 --- a/drivers/bluetooth/btnxpuart.c +++ b/drivers/bluetooth/btnxpuart.c @@ -281,7 +281,7 @@ static u8 crc8_table[CRC8_TABLE_SIZE]; /* Default configurations */ #define DEFAULT_H2C_WAKEUP_MODE WAKEUP_METHOD_BREAK -#define DEFAULT_PS_MODE PS_MODE_DISABLE +#define DEFAULT_PS_MODE PS_MODE_ENABLE #define FW_INIT_BAUDRATE HCI_NXP_PRI_BAUDRATE static struct sk_buff *nxp_drv_send_cmd(struct hci_dev *hdev, u16 opcode, -- cgit From 015d79c96d62cd8a4a359fcf5be40d58088c936b Mon Sep 17 00:00:00 2001 From: Edward Adam Davis Date: Mon, 17 Jun 2024 19:09:37 +0800 Subject: Bluetooth: Ignore too large handle values in BIG hci_le_big_sync_established_evt is necessary to filter out cases where the handle value is belonging to ida id range, otherwise ida will be erroneously released in hci_conn_cleanup. Fixes: 181a42edddf5 ("Bluetooth: Make handle of hci_conn be unique") Reported-by: syzbot+b2545b087a01a7319474@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=b2545b087a01a7319474 Signed-off-by: Edward Adam Davis Signed-off-by: Luiz Augusto von Dentz --- net/bluetooth/hci_event.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index 5d23bcf18716..93f7ac905cec 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -6916,6 +6916,10 @@ static void hci_le_big_sync_established_evt(struct hci_dev *hdev, void *data, bis = hci_conn_hash_lookup_handle(hdev, handle); if (!bis) { + if (handle > HCI_CONN_HANDLE_MAX) { + bt_dev_dbg(hdev, "ignore too large handle %u", handle); + continue; + } bis = hci_conn_add(hdev, ISO_LINK, BDADDR_ANY, HCI_ROLE_SLAVE, handle); if (IS_ERR(bis)) -- cgit From 89e856e124f9ae548572c56b1b70c2255705f8fe Mon Sep 17 00:00:00 2001 From: Edward Adam Davis Date: Sat, 15 Jun 2024 09:45:54 +0800 Subject: bluetooth/l2cap: sync sock recv cb and release The problem occurs between the system call to close the sock and hci_rx_work, where the former releases the sock and the latter accesses it without lock protection. CPU0 CPU1 ---- ---- sock_close hci_rx_work l2cap_sock_release hci_acldata_packet l2cap_sock_kill l2cap_recv_frame sk_free l2cap_conless_channel l2cap_sock_recv_cb If hci_rx_work processes the data that needs to be received before the sock is closed, then everything is normal; Otherwise, the work thread may access the released sock when receiving data. Add a chan mutex in the rx callback of the sock to achieve synchronization between the sock release and recv cb. Sock is dead, so set chan data to NULL, avoid others use invalid sock pointer. Reported-and-tested-by: syzbot+b7f6f8c9303466e16c8a@syzkaller.appspotmail.com Signed-off-by: Edward Adam Davis Signed-off-by: Luiz Augusto von Dentz --- net/bluetooth/l2cap_sock.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c index 6db60946c627..962aa11ce3de 100644 --- a/net/bluetooth/l2cap_sock.c +++ b/net/bluetooth/l2cap_sock.c @@ -1239,6 +1239,10 @@ static void l2cap_sock_kill(struct sock *sk) BT_DBG("sk %p state %s", sk, state_to_string(sk->sk_state)); + /* Sock is dead, so set chan data to NULL, avoid other task use invalid + * sock pointer. + */ + l2cap_pi(sk)->chan->data = NULL; /* Kill poor orphan */ l2cap_chan_put(l2cap_pi(sk)->chan); @@ -1481,12 +1485,25 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(struct l2cap_chan *chan) static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb) { - struct sock *sk = chan->data; - struct l2cap_pinfo *pi = l2cap_pi(sk); + struct sock *sk; + struct l2cap_pinfo *pi; int err; - lock_sock(sk); + /* To avoid race with sock_release, a chan lock needs to be added here + * to synchronize the sock. + */ + l2cap_chan_hold(chan); + l2cap_chan_lock(chan); + sk = chan->data; + if (!sk) { + l2cap_chan_unlock(chan); + l2cap_chan_put(chan); + return -ENXIO; + } + + pi = l2cap_pi(sk); + lock_sock(sk); if (chan->mode == L2CAP_MODE_ERTM && !list_empty(&pi->rx_busy)) { err = -ENOMEM; goto done; @@ -1535,6 +1552,8 @@ static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb) done: release_sock(sk); + l2cap_chan_unlock(chan); + l2cap_chan_put(chan); return err; } -- cgit From 596b6f081336e77764ca35cfeab66d0fcdbe544e Mon Sep 17 00:00:00 2001 From: Iulia Tanasescu Date: Tue, 18 Jun 2024 13:33:24 +0300 Subject: Bluetooth: ISO: Check socket flag instead of hcon This fixes the following Smatch static checker warning: net/bluetooth/iso.c:1364 iso_sock_recvmsg() error: we previously assumed 'pi->conn->hcon' could be null (line 1359) net/bluetooth/iso.c 1347 static int iso_sock_recvmsg(struct socket *sock, struct msghdr *msg, 1348 size_t len, int flags) 1349 { 1350 struct sock *sk = sock->sk; 1351 struct iso_pinfo *pi = iso_pi(sk); 1352 1353 BT_DBG("sk %p", sk); 1354 1355 if (test_and_clear_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags)) { 1356 lock_sock(sk); 1357 switch (sk->sk_state) { 1358 case BT_CONNECT2: 1359 if (pi->conn->hcon && ^^^^^^^^^^^^^^ If ->hcon is NULL 1360 test_bit(HCI_CONN_PA_SYNC, &pi->conn->hcon->flags)) { 1361 iso_conn_big_sync(sk); 1362 sk->sk_state = BT_LISTEN; 1363 } else { --> 1364 iso_conn_defer_accept(pi->conn->hcon); ^^^^^^^^^^^^^^ then we're toast 1365 sk->sk_state = BT_CONFIG; 1366 } 1367 release_sock(sk); 1368 return 0; 1369 case BT_CONNECTED: 1370 if (test_bit(BT_SK_PA_SYNC, Fixes: fbdc4bc47268 ("Bluetooth: ISO: Use defer setup to separate PA sync and BIG sync") Signed-off-by: Iulia Tanasescu Signed-off-by: Luiz Augusto von Dentz --- net/bluetooth/iso.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c index cc055b952ce6..398fb81f7a13 100644 --- a/net/bluetooth/iso.c +++ b/net/bluetooth/iso.c @@ -1356,8 +1356,7 @@ static int iso_sock_recvmsg(struct socket *sock, struct msghdr *msg, lock_sock(sk); switch (sk->sk_state) { case BT_CONNECT2: - if (pi->conn->hcon && - test_bit(HCI_CONN_PA_SYNC, &pi->conn->hcon->flags)) { + if (test_bit(BT_SK_PA_SYNC, &pi->flags)) { iso_conn_big_sync(sk); sk->sk_state = BT_LISTEN; } else { -- cgit From 1cc18c2ab2e8c54c355ea7c0423a636e415a0c23 Mon Sep 17 00:00:00 2001 From: Pavel Skripkin Date: Thu, 20 Jun 2024 22:27:47 +0300 Subject: bluetooth/hci: disallow setting handle bigger than HCI_CONN_HANDLE_MAX Syzbot hit warning in hci_conn_del() caused by freeing handle that was not allocated using ida allocator. This is caused by handle bigger than HCI_CONN_HANDLE_MAX passed by hci_le_big_sync_established_evt(), which makes code think it's unset connection. Add same check for handle upper bound as in hci_conn_set_handle() to prevent warning. Link: https://syzkaller.appspot.com/bug?extid=b2545b087a01a7319474 Reported-by: syzbot+b2545b087a01a7319474@syzkaller.appspotmail.com Fixes: 181a42edddf5 ("Bluetooth: Make handle of hci_conn be unique") Signed-off-by: Pavel Skripkin Signed-off-by: Luiz Augusto von Dentz --- net/bluetooth/hci_conn.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c index 0c76dcde5361..080053a85b4d 100644 --- a/net/bluetooth/hci_conn.c +++ b/net/bluetooth/hci_conn.c @@ -899,8 +899,8 @@ static int hci_conn_hash_alloc_unset(struct hci_dev *hdev) U16_MAX, GFP_ATOMIC); } -struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst, - u8 role, u16 handle) +static struct hci_conn *__hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst, + u8 role, u16 handle) { struct hci_conn *conn; @@ -1041,7 +1041,16 @@ struct hci_conn *hci_conn_add_unset(struct hci_dev *hdev, int type, if (unlikely(handle < 0)) return ERR_PTR(-ECONNREFUSED); - return hci_conn_add(hdev, type, dst, role, handle); + return __hci_conn_add(hdev, type, dst, role, handle); +} + +struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst, + u8 role, u16 handle) +{ + if (handle > HCI_CONN_HANDLE_MAX) + return ERR_PTR(-EINVAL); + + return __hci_conn_add(hdev, type, dst, role, handle); } static void hci_conn_cleanup_child(struct hci_conn *conn, u8 reason) -- cgit From f1a8f402f13f94263cf349216c257b2985100927 Mon Sep 17 00:00:00 2001 From: Luiz Augusto von Dentz Date: Mon, 24 Jun 2024 09:42:09 -0400 Subject: Bluetooth: L2CAP: Fix deadlock This fixes the following deadlock introduced by 39a92a55be13 ("bluetooth/l2cap: sync sock recv cb and release") ============================================ WARNING: possible recursive locking detected 6.10.0-rc3-g4029dba6b6f1 #6823 Not tainted -------------------------------------------- kworker/u5:0/35 is trying to acquire lock: ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at: l2cap_sock_recv_cb+0x44/0x1e0 but task is already holding lock: ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at: l2cap_get_chan_by_scid+0xaf/0xd0 other info that might help us debug this: Possible unsafe locking scenario: CPU0 ---- lock(&chan->lock#2/1); lock(&chan->lock#2/1); *** DEADLOCK *** May be due to missing lock nesting notation 3 locks held by kworker/u5:0/35: #0: ffff888002b8a940 ((wq_completion)hci0#2){+.+.}-{0:0}, at: process_one_work+0x750/0x930 #1: ffff888002c67dd0 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0}, at: process_one_work+0x44e/0x930 #2: ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at: l2cap_get_chan_by_scid+0xaf/0xd0 To fix the original problem this introduces l2cap_chan_lock at l2cap_conless_channel to ensure that l2cap_sock_recv_cb is called with chan->lock held. Fixes: 89e856e124f9 ("bluetooth/l2cap: sync sock recv cb and release") Signed-off-by: Luiz Augusto von Dentz --- include/net/bluetooth/hci_sync.h | 2 ++ net/bluetooth/hci_core.c | 72 ++++++++++------------------------------ net/bluetooth/hci_sync.c | 13 ++++++++ net/bluetooth/l2cap_core.c | 3 ++ net/bluetooth/l2cap_sock.c | 13 +------- 5 files changed, 37 insertions(+), 66 deletions(-) diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h index 6a9d063e9f47..534c3386e714 100644 --- a/include/net/bluetooth/hci_sync.h +++ b/include/net/bluetooth/hci_sync.h @@ -38,6 +38,8 @@ int __hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen, int __hci_cmd_sync_status_sk(struct hci_dev *hdev, u16 opcode, u32 plen, const void *param, u8 event, u32 timeout, struct sock *sk); +int hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen, + const void *param, u32 timeout); void hci_cmd_sync_init(struct hci_dev *hdev); void hci_cmd_sync_clear(struct hci_dev *hdev); diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index dbbe5e2da210..c644b30977bd 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -63,50 +63,6 @@ DEFINE_MUTEX(hci_cb_list_lock); /* HCI ID Numbering */ static DEFINE_IDA(hci_index_ida); -static int hci_scan_req(struct hci_request *req, unsigned long opt) -{ - __u8 scan = opt; - - BT_DBG("%s %x", req->hdev->name, scan); - - /* Inquiry and Page scans */ - hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan); - return 0; -} - -static int hci_auth_req(struct hci_request *req, unsigned long opt) -{ - __u8 auth = opt; - - BT_DBG("%s %x", req->hdev->name, auth); - - /* Authentication */ - hci_req_add(req, HCI_OP_WRITE_AUTH_ENABLE, 1, &auth); - return 0; -} - -static int hci_encrypt_req(struct hci_request *req, unsigned long opt) -{ - __u8 encrypt = opt; - - BT_DBG("%s %x", req->hdev->name, encrypt); - - /* Encryption */ - hci_req_add(req, HCI_OP_WRITE_ENCRYPT_MODE, 1, &encrypt); - return 0; -} - -static int hci_linkpol_req(struct hci_request *req, unsigned long opt) -{ - __le16 policy = cpu_to_le16(opt); - - BT_DBG("%s %x", req->hdev->name, policy); - - /* Default link policy */ - hci_req_add(req, HCI_OP_WRITE_DEF_LINK_POLICY, 2, &policy); - return 0; -} - /* Get HCI device by index. * Device is held on return. */ struct hci_dev *hci_dev_get(int index) @@ -735,6 +691,7 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg) { struct hci_dev *hdev; struct hci_dev_req dr; + __le16 policy; int err = 0; if (copy_from_user(&dr, arg, sizeof(dr))) @@ -761,8 +718,8 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg) switch (cmd) { case HCISETAUTH: - err = hci_req_sync(hdev, hci_auth_req, dr.dev_opt, - HCI_INIT_TIMEOUT, NULL); + err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_AUTH_ENABLE, + 1, &dr.dev_opt, HCI_CMD_TIMEOUT); break; case HCISETENCRYPT: @@ -773,19 +730,23 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg) if (!test_bit(HCI_AUTH, &hdev->flags)) { /* Auth must be enabled first */ - err = hci_req_sync(hdev, hci_auth_req, dr.dev_opt, - HCI_INIT_TIMEOUT, NULL); + err = __hci_cmd_sync_status(hdev, + HCI_OP_WRITE_AUTH_ENABLE, + 1, &dr.dev_opt, + HCI_CMD_TIMEOUT); if (err) break; } - err = hci_req_sync(hdev, hci_encrypt_req, dr.dev_opt, - HCI_INIT_TIMEOUT, NULL); + err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_ENCRYPT_MODE, + 1, &dr.dev_opt, + HCI_CMD_TIMEOUT); break; case HCISETSCAN: - err = hci_req_sync(hdev, hci_scan_req, dr.dev_opt, - HCI_INIT_TIMEOUT, NULL); + err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_SCAN_ENABLE, + 1, &dr.dev_opt, + HCI_CMD_TIMEOUT); /* Ensure that the connectable and discoverable states * get correctly modified as this was a non-mgmt change. @@ -795,8 +756,11 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg) break; case HCISETLINKPOL: - err = hci_req_sync(hdev, hci_linkpol_req, dr.dev_opt, - HCI_INIT_TIMEOUT, NULL); + policy = cpu_to_le16(dr.dev_opt); + + err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_DEF_LINK_POLICY, + 2, &policy, + HCI_CMD_TIMEOUT); break; case HCISETLINKMODE: diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c index a8a7d2b36870..eea34e6a236f 100644 --- a/net/bluetooth/hci_sync.c +++ b/net/bluetooth/hci_sync.c @@ -280,6 +280,19 @@ int __hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen, } EXPORT_SYMBOL(__hci_cmd_sync_status); +int hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen, + const void *param, u32 timeout) +{ + int err; + + hci_req_sync_lock(hdev); + err = __hci_cmd_sync_status(hdev, opcode, plen, param, timeout); + hci_req_sync_unlock(hdev); + + return err; +} +EXPORT_SYMBOL(hci_cmd_sync_status); + static void hci_cmd_sync_work(struct work_struct *work) { struct hci_dev *hdev = container_of(work, struct hci_dev, cmd_sync_work); diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c index aed025734d04..c3c26bbb5dda 100644 --- a/net/bluetooth/l2cap_core.c +++ b/net/bluetooth/l2cap_core.c @@ -6761,6 +6761,8 @@ static void l2cap_conless_channel(struct l2cap_conn *conn, __le16 psm, BT_DBG("chan %p, len %d", chan, skb->len); + l2cap_chan_lock(chan); + if (chan->state != BT_BOUND && chan->state != BT_CONNECTED) goto drop; @@ -6777,6 +6779,7 @@ static void l2cap_conless_channel(struct l2cap_conn *conn, __le16 psm, } drop: + l2cap_chan_unlock(chan); l2cap_chan_put(chan); free_skb: kfree_skb(skb); diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c index 962aa11ce3de..ba437c6f6ee5 100644 --- a/net/bluetooth/l2cap_sock.c +++ b/net/bluetooth/l2cap_sock.c @@ -1489,18 +1489,9 @@ static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb) struct l2cap_pinfo *pi; int err; - /* To avoid race with sock_release, a chan lock needs to be added here - * to synchronize the sock. - */ - l2cap_chan_hold(chan); - l2cap_chan_lock(chan); sk = chan->data; - - if (!sk) { - l2cap_chan_unlock(chan); - l2cap_chan_put(chan); + if (!sk) return -ENXIO; - } pi = l2cap_pi(sk); lock_sock(sk); @@ -1552,8 +1543,6 @@ static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb) done: release_sock(sk); - l2cap_chan_unlock(chan); - l2cap_chan_put(chan); return err; } -- cgit