Hi Dmitry,
On Tue, May 20, 2025 at 10:50 AM Dmitry Antipov <dmantipov@xxxxxxxxx> wrote:
>
> An overall idea is that 'mgmt_pending' of 'struct hci_dev' may be altered
> only in 'mgmt_pending_add()' under 'mgmt_lock' protection, where processed
> commands are removed each time when the new command is added. All other
> users of 'mgmt_pending' (except 'mgmt_pending_cleanup()' where no
> concurrent accesses are expected) are read-side critical sections running
> under 'rcu_read_lock()'. (I'm also trying to fix socket UAFs observed when
> running this code, and most likely these fixes should go to the separate
> patch).
>
> Suggested-by: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> Signed-off-by: Dmitry Antipov <dmantipov@xxxxxxxxx>
> ---
> include/net/bluetooth/bluetooth.h | 1 +
> include/net/bluetooth/hci_core.h | 1 +
> net/bluetooth/hci_core.c | 6 +--
> net/bluetooth/mgmt.c | 65 ++++++++++++++++++++++---------
> net/bluetooth/mgmt_util.c | 55 ++++++++++++++++++++------
> net/bluetooth/mgmt_util.h | 3 ++
> 6 files changed, 96 insertions(+), 35 deletions(-)
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index bbefde319f95..cee6be23acc8 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -655,6 +655,7 @@ static inline bool iso_enabled(void)
> int mgmt_init(void);
> void mgmt_exit(void);
> void mgmt_cleanup(struct sock *sk);
> +void mgmt_pending_cleanup(struct hci_dev *hdev);
>
> void bt_sock_reclassify_lock(struct sock *sk, int proto);
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 54bfeeaa0995..2fb586c6d684 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -546,6 +546,7 @@ struct hci_dev {
>
> struct list_head mesh_pending;
> struct list_head mgmt_pending;
> + struct mutex mgmt_lock;
> struct list_head reject_list;
> struct list_head accept_list;
> struct list_head uuids;
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 5eb0600bbd03..0172ec45f2df 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2492,6 +2492,7 @@ struct hci_dev *hci_alloc_dev_priv(int sizeof_priv)
>
> INIT_LIST_HEAD(&hdev->mesh_pending);
> INIT_LIST_HEAD(&hdev->mgmt_pending);
> + mutex_init(&hdev->mgmt_lock);
> INIT_LIST_HEAD(&hdev->reject_list);
> INIT_LIST_HEAD(&hdev->accept_list);
> INIT_LIST_HEAD(&hdev->uuids);
> @@ -2685,10 +2686,7 @@ void hci_unregister_dev(struct hci_dev *hdev)
> hci_dev_unlock(hdev);
> }
>
> - /* mgmt_index_removed should take care of emptying the
> - * pending list */
> - BUG_ON(!list_empty(&hdev->mgmt_pending));
> -
> + mgmt_pending_cleanup(hdev);
> hci_sock_dev_event(hdev, HCI_DEV_UNREG);
>
> if (hdev->rfkill) {
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 46b22708dfbd..e2eccc44076d 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -1444,25 +1444,30 @@ struct cmd_lookup {
> static void settings_rsp(struct mgmt_pending_cmd *cmd, void *data)
> {
> struct cmd_lookup *match = data;
> + struct sock *sk = cmd->sk;
>
> - send_settings_rsp(cmd->sk, cmd->opcode, match->hdev);
> -
> - list_del(&cmd->list);
> + sock_hold(sk);
>
> - if (match->sk == NULL) {
> - match->sk = cmd->sk;
> - sock_hold(match->sk);
> - }
> + send_settings_rsp(sk, cmd->opcode, match->hdev);
> + mgmt_pending_remove(cmd);
>
> - mgmt_pending_free(cmd);
> + if (match->sk == NULL)
> + match->sk = sk;
> + else
> + sock_put(sk);
> }
>
> static void cmd_status_rsp(struct mgmt_pending_cmd *cmd, void *data)
> {
> + struct sock *sk = cmd->sk;
> u8 *status = data;
>
> + sock_hold(sk);
> +
> mgmt_cmd_status(cmd->sk, cmd->index, cmd->opcode, *status);
> mgmt_pending_remove(cmd);
> +
> + sock_put(sk);
> }
>
> static void cmd_complete_rsp(struct mgmt_pending_cmd *cmd, void *data)
> @@ -2598,18 +2603,25 @@ static int mgmt_hci_cmd_sync(struct sock *sk, struct hci_dev *hdev,
> static bool pending_eir_or_class(struct hci_dev *hdev)
> {
> struct mgmt_pending_cmd *cmd;
> + bool found = false;
> +
> + rcu_read_lock();
>
> - list_for_each_entry(cmd, &hdev->mgmt_pending, list) {
> + list_for_each_entry_rcu(cmd, &hdev->mgmt_pending, list) {
> + if (atomic_read(&cmd->deleted))
> + continue;
> switch (cmd->opcode) {
> case MGMT_OP_ADD_UUID:
> case MGMT_OP_REMOVE_UUID:
> case MGMT_OP_SET_DEV_CLASS:
> case MGMT_OP_SET_POWERED:
> - return true;
> + found = true;
> + break;
> }
> }
>
> - return false;
> + rcu_read_unlock();
> + return found;
> }
>
> static const u8 bluetooth_base_uuid[] = {
> @@ -3401,19 +3413,23 @@ static int set_io_capability(struct sock *sk, struct hci_dev *hdev, void *data,
> static struct mgmt_pending_cmd *find_pairing(struct hci_conn *conn)
> {
> struct hci_dev *hdev = conn->hdev;
> - struct mgmt_pending_cmd *cmd;
> + struct mgmt_pending_cmd *tmp, *cmd = NULL;
>
> - list_for_each_entry(cmd, &hdev->mgmt_pending, list) {
> - if (cmd->opcode != MGMT_OP_PAIR_DEVICE)
> - continue;
> + rcu_read_lock();
>
> - if (cmd->user_data != conn)
> + list_for_each_entry(tmp, &hdev->mgmt_pending, list) {
> + if (atomic_read(&tmp->deleted))
> continue;
> -
> - return cmd;
> + if (tmp->opcode != MGMT_OP_PAIR_DEVICE)
> + continue;
> + if (tmp->user_data != conn)
> + continue;
> + cmd = tmp;
> + break;
> }
>
> - return NULL;
> + rcu_read_unlock();
> + return cmd;
> }
>
> static int pairing_complete(struct mgmt_pending_cmd *cmd, u8 status)
> @@ -10476,3 +10492,14 @@ void mgmt_cleanup(struct sock *sk)
>
> read_unlock(&hci_dev_list_lock);
> }
> +
> +void mgmt_pending_cleanup(struct hci_dev *hdev)
> +{
> + struct mgmt_pending_cmd *cmd, *tmp;
> +
> + list_for_each_entry_safe(cmd, tmp, &hdev->mgmt_pending, list) {
> + BUG_ON(atomic_read(&cmd->deleted) == 0);
> + list_del_rcu(&cmd->list);
> + mgmt_pending_free(cmd);
> + }
> +}
> diff --git a/net/bluetooth/mgmt_util.c b/net/bluetooth/mgmt_util.c
> index 3713ff490c65..3442bd37fa52 100644
> --- a/net/bluetooth/mgmt_util.c
> +++ b/net/bluetooth/mgmt_util.c
> @@ -217,30 +217,45 @@ int mgmt_cmd_complete(struct sock *sk, u16 index, u16 cmd, u8 status,
> struct mgmt_pending_cmd *mgmt_pending_find(unsigned short channel, u16 opcode,
> struct hci_dev *hdev)
> {
> - struct mgmt_pending_cmd *cmd;
> + struct mgmt_pending_cmd *tmp, *cmd = NULL;
> +
> + rcu_read_lock();
>
> - list_for_each_entry(cmd, &hdev->mgmt_pending, list) {
> - if (hci_sock_get_channel(cmd->sk) != channel)
> + list_for_each_entry_rcu(tmp, &hdev->mgmt_pending, list) {
> + if (atomic_read(&tmp->deleted))
> continue;
> - if (cmd->opcode == opcode)
> - return cmd;
> + if (hci_sock_get_channel(tmp->sk) != channel)
> + continue;
> + if (tmp->opcode == opcode) {
> + cmd = tmp;
> + break;
> + }
> }
>
> - return NULL;
> + rcu_read_unlock();
> + return cmd;
> }
>
> void mgmt_pending_foreach(u16 opcode, struct hci_dev *hdev,
> void (*cb)(struct mgmt_pending_cmd *cmd, void *data),
> void *data)
> {
> - struct mgmt_pending_cmd *cmd, *tmp;
> + struct mgmt_pending_cmd *cmd;
>
> - list_for_each_entry_safe(cmd, tmp, &hdev->mgmt_pending, list) {
> + rcu_read_lock();
> +
> + list_for_each_entry_rcu(cmd, &hdev->mgmt_pending, list) {
> + if (atomic_read(&cmd->deleted))
> + continue;
> if (opcode > 0 && cmd->opcode != opcode)
> continue;
>
> + rcu_read_unlock();
> cb(cmd, data);
> + rcu_read_lock();
> }
> +
> + rcu_read_unlock();
> }
>
> struct mgmt_pending_cmd *mgmt_pending_new(struct sock *sk, u16 opcode,
> @@ -270,17 +285,34 @@ struct mgmt_pending_cmd *mgmt_pending_new(struct sock *sk, u16 opcode,
> return cmd;
> }
>
> +static void mgmt_pending_delayed_free(struct rcu_head *rcu)
> +{
> + struct mgmt_pending_cmd *cmd =
> + container_of(rcu, struct mgmt_pending_cmd, head);
> + kfree(cmd->param);
> + kfree(cmd);
> +}
> +
> struct mgmt_pending_cmd *mgmt_pending_add(struct sock *sk, u16 opcode,
> struct hci_dev *hdev,
> void *data, u16 len)
> {
> - struct mgmt_pending_cmd *cmd;
> + struct mgmt_pending_cmd *cmd, *old, *tmp;
>
> cmd = mgmt_pending_new(sk, opcode, hdev, data, len);
> if (!cmd)
> return NULL;
>
> - list_add_tail(&cmd->list, &hdev->mgmt_pending);
> + mutex_lock(&hdev->mgmt_lock);
> + list_for_each_entry_safe(old, tmp, &hdev->mgmt_pending, list)
> + if (atomic_read(&old->deleted)) {
> + list_del_rcu(&old->list);
> + sock_put(old->sk);
> + call_rcu(&old->head, mgmt_pending_delayed_free);
> + }
> +
> + list_add_tail_rcu(&cmd->list, &hdev->mgmt_pending);
> + mutex_unlock(&hdev->mgmt_lock);
>
> return cmd;
> }
> @@ -294,8 +326,7 @@ void mgmt_pending_free(struct mgmt_pending_cmd *cmd)
>
> void mgmt_pending_remove(struct mgmt_pending_cmd *cmd)
> {
> - list_del(&cmd->list);
> - mgmt_pending_free(cmd);
> + atomic_set(&cmd->deleted, 1);
I'm afraid you were not looking into other places of the code
regarding this, what you should have done is:
list_del_rcu
synchronize_rcu
free
synchronize_rcu will wait the threads holding rcu_read_lock so by the
time it returns we can proceed to free because all existing readers
should be done already and if in the meantime another thread attempts
to iterate in the list that shall already been update given that
list_del_rcu has updated the list.
> }
>
> void mgmt_mesh_foreach(struct hci_dev *hdev,
> diff --git a/net/bluetooth/mgmt_util.h b/net/bluetooth/mgmt_util.h
> index f2ba994ab1d8..5e681bc74220 100644
> --- a/net/bluetooth/mgmt_util.h
> +++ b/net/bluetooth/mgmt_util.h
> @@ -32,6 +32,8 @@ struct mgmt_mesh_tx {
>
> struct mgmt_pending_cmd {
> struct list_head list;
> + struct rcu_head head;
> + atomic_t deleted;
> u16 opcode;
> int index;
> void *param;
> @@ -65,6 +67,7 @@ struct mgmt_pending_cmd *mgmt_pending_new(struct sock *sk, u16 opcode,
> void *data, u16 len);
> void mgmt_pending_free(struct mgmt_pending_cmd *cmd);
> void mgmt_pending_remove(struct mgmt_pending_cmd *cmd);
> +void mgmt_pending_cleanup(struct hci_dev *hdev);
> void mgmt_mesh_foreach(struct hci_dev *hdev,
> void (*cb)(struct mgmt_mesh_tx *mesh_tx, void *data),
> void *data, struct sock *sk);
> --
> 2.49.0
>
--
Luiz Augusto von Dentz
