| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <76e031118390810ddd1d33b92d8a30c5d49d0425.camel@iki.fi>
Date: Mon, 03 Nov 2025 02:04:15 +0200
From: Pauli Virtanen <pav@....fi>
To: Hillf Danton <hdanton@...a.com>
Cc: marcel@...tmann.org, johan.hedberg@...il.com, luiz.dentz@...il.com,
linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/8] Bluetooth: mgmt: take lock and hold reference
when handling hci_conn
ma, 2025-11-03 kello 07:21 +0800, Hillf Danton kirjoitti:
> On Sun, 2 Nov 2025 18:19:35 +0200 Pauli Virtanen wrote:
> > Take hdev/rcu lock to prevent concurrent deletion of the hci_conn we are
> > handling.
> >
> > When using hci_conn* pointers across critical sections, always take
> > refcount to keep the pointer valid.
> >
> > For hci_abort_conn() only hold refcount, as the function takes
> > hdev->lock itself.
> >
> > Fixes: 227a0cdf4a028 ("Bluetooth: MGMT: Fix not generating command complete for MGMT_OP_DISCONNECT")
> > Signed-off-by: Pauli Virtanen <pav@....fi>
> > ---
> >
> > Notes:
> > v2:
> > - no change
> >
> > net/bluetooth/mgmt.c | 42 ++++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 38 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> > index 78b7af8bf45f..535c475c2d25 100644
> > --- a/net/bluetooth/mgmt.c
> > +++ b/net/bluetooth/mgmt.c
> > @@ -3081,6 +3081,8 @@ static int unpair_device_sync(struct hci_dev *hdev, void *data)
> > struct mgmt_cp_unpair_device *cp = cmd->param;
> > struct hci_conn *conn;
> >
> > + rcu_read_lock();
> > +
> > if (cp->addr.type == BDADDR_BREDR)
> > conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> > &cp->addr.bdaddr);
> > @@ -3088,6 +3090,11 @@ static int unpair_device_sync(struct hci_dev *hdev, void *data)
> > conn = hci_conn_hash_lookup_le(hdev, &cp->addr.bdaddr,
> > le_addr_type(cp->addr.type));
> >
> > + if (conn)
> > + hci_conn_get(conn);
> > +
> > + rcu_read_unlock();
> > +
> Given RCU in the lookup helpers, nested RCU makes no sense.
> Feel free to add the lookup_and_get helper instead to bump the refcnt up
> for the conn found.
RCU must be held until the refcount is increased. I don't think there
is difference in whether it's done by nesting or by duplicating all the
helpers without the rcu_read_lock().
The rcu_read_lock() in the helpers is probaly something that should be
done away with --- the caller needs to hold RCU or suitable other lock
if it wants to use the returned value safely.
> Another simpler option is -- add conn to hash with refcnt increased and
> correspondingly put conn after deleting it from hash.
>
> > if (!conn)
> > return 0;
> >
> > @@ -3095,6 +3102,7 @@ static int unpair_device_sync(struct hci_dev *hdev, void *data)
> > * will clean up the connection no matter the error.
> > */
> > hci_abort_conn(conn, HCI_ERROR_REMOTE_USER_TERM);
> > + hci_conn_put(conn);
> >
> > return 0;
> > }
> > @@ -3242,6 +3250,8 @@ static int disconnect_sync(struct hci_dev *hdev, void *data)
> > struct mgmt_cp_disconnect *cp = cmd->param;
> > struct hci_conn *conn;
> >
> > + rcu_read_lock();
> > +
> > if (cp->addr.type == BDADDR_BREDR)
> > conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> > &cp->addr.bdaddr);
> > @@ -3249,6 +3259,11 @@ static int disconnect_sync(struct hci_dev *hdev, void *data)
> > conn = hci_conn_hash_lookup_le(hdev, &cp->addr.bdaddr,
> > le_addr_type(cp->addr.type));
> >
> > + if (conn)
> > + hci_conn_get(conn);
> > +
> > + rcu_read_unlock();
> > +
> > if (!conn)
> > return -ENOTCONN;
> >
> > @@ -3256,6 +3271,7 @@ static int disconnect_sync(struct hci_dev *hdev, void *data)
> > * will clean up the connection no matter the error.
> > */
> > hci_abort_conn(conn, HCI_ERROR_REMOTE_USER_TERM);
> > + hci_conn_put(conn);
> >
> > return 0;
> > }
> > @@ -7372,6 +7388,9 @@ static void get_conn_info_complete(struct hci_dev *hdev, void *data, int err)
> > rp.max_tx_power = HCI_TX_POWER_INVALID;
> > }
> >
> > + if (conn)
> > + hci_conn_put(conn);
> > +
> > mgmt_cmd_complete(cmd->sk, cmd->hdev->id, MGMT_OP_GET_CONN_INFO, status,
> > &rp, sizeof(rp));
> >
> > @@ -7386,6 +7405,8 @@ static int get_conn_info_sync(struct hci_dev *hdev, void *data)
> > int err;
> > __le16 handle;
> >
> > + hci_dev_lock(hdev);
> > +
> > /* Make sure we are still connected */
> > if (cp->addr.type == BDADDR_BREDR)
> > conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> > @@ -7393,12 +7414,16 @@ static int get_conn_info_sync(struct hci_dev *hdev, void *data)
> > else
> > conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &cp->addr.bdaddr);
> >
> > - if (!conn || conn->state != BT_CONNECTED)
> > + if (!conn || conn->state != BT_CONNECTED) {
> > + hci_dev_unlock(hdev);
> > return MGMT_STATUS_NOT_CONNECTED;
> > + }
> >
> > - cmd->user_data = conn;
> > + cmd->user_data = hci_conn_get(conn);
> > handle = cpu_to_le16(conn->handle);
> >
> > + hci_dev_unlock(hdev);
> > +
> > /* Refresh RSSI each time */
> > err = hci_read_rssi_sync(hdev, handle);
> >
> > @@ -7532,6 +7557,9 @@ static void get_clock_info_complete(struct hci_dev *hdev, void *data, int err)
> > }
> >
> > complete:
> > + if (conn)
> > + hci_conn_put(conn);
> > +
> > mgmt_cmd_complete(cmd->sk, cmd->hdev->id, cmd->opcode, status, &rp,
> > sizeof(rp));
> >
> > @@ -7548,15 +7576,21 @@ static int get_clock_info_sync(struct hci_dev *hdev, void *data)
> > memset(&hci_cp, 0, sizeof(hci_cp));
> > hci_read_clock_sync(hdev, &hci_cp);
> >
> > + hci_dev_lock(hdev);
> > +
> > /* Make sure connection still exists */
> > conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &cp->addr.bdaddr);
> > - if (!conn || conn->state != BT_CONNECTED)
> > + if (!conn || conn->state != BT_CONNECTED) {
> > + hci_dev_unlock(hdev);
> > return MGMT_STATUS_NOT_CONNECTED;
> > + }
> >
> > - cmd->user_data = conn;
> > + cmd->user_data = hci_conn_get(conn);
> > hci_cp.handle = cpu_to_le16(conn->handle);
> > hci_cp.which = 0x01; /* Piconet clock */
> >
> > + hci_dev_unlock(hdev);
> > +
> > return hci_read_clock_sync(hdev, &hci_cp);
> > }
> >
> > --
> > 2.51.1
--
Pauli Virtanen
Powered by blists - more mailing lists