netdev - Re: [GIT PULL] bluetooth 2025-09-20

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABBYNZJ6DcUPHgCrDquW+_q340yxJHfudVDzUFWuXMTFvqPEbA@mail.gmail.com>
Date: Mon, 22 Sep 2025 09:59:44 -0400
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, linux-bluetooth@...r.kernel.org, 
	netdev@...r.kernel.org
Subject: Re: [GIT PULL] bluetooth 2025-09-20

Hi Jakub,

On Sat, Sep 20, 2025 at 4:38 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Sat, 20 Sep 2025 11:04:53 -0400 Luiz Augusto von Dentz wrote:
> >       Bluetooth: MGMT: Fix possible UAFs
>
> Are you amenable to rewriting this one? The conditional locking really
> doesn't look great. It's just a few more lines for the caller to take
> the lock, below completely untested but to illustrate..

I guess the idea is to have it open coded to avoid mistakes like
unbalanced locking, etc, right?

> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 1e7886ccee40..23cb19b9915d 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -1358,8 +1358,10 @@ static int set_powered_sync(struct hci_dev *hdev, void *data)
>         struct mgmt_pending_cmd *cmd = data;
>         struct mgmt_mode cp;
>
> +       mutex_lock(&hdev->mgmt_pending_lock);
> +
>         /* Make sure cmd still outstanding. */
> -       if (!mgmt_pending_valid(hdev, cmd, false))
> +       if (!__mgmt_pending_listed(hdev, cmd))
>                 return -ECANCELED;

Sure, this does require calling unlocking also when it fails though,
but I guess that is to be expected with this kind of construct and we
could have a variant that does the locking inline in case the cmd
fields don't need to be accessed.

>
>         memcpy(&cp, cmd->param, sizeof(cp));
> diff --git a/net/bluetooth/mgmt_util.c b/net/bluetooth/mgmt_util.c
> index 258c22d38809..11b1d1667d08 100644
> --- a/net/bluetooth/mgmt_util.c
> +++ b/net/bluetooth/mgmt_util.c
> @@ -320,28 +320,38 @@ void mgmt_pending_remove(struct mgmt_pending_cmd *cmd)
>         mgmt_pending_free(cmd);
>  }
>
> -bool mgmt_pending_valid(struct hci_dev *hdev, struct mgmt_pending_cmd *cmd,
> -                       bool remove_unlock)
> +bool __mgmt_pending_listed(struct hci_dev *hdev, struct mgmt_pending_cmd *cmd)
>  {
>         struct mgmt_pending_cmd *tmp;
>
> +       lockdep_assert_held(&hdev->mgmt_pending_lock);
>         if (!cmd)
>                 return false;
>
> -       mutex_lock(&hdev->mgmt_pending_lock);
> -
>         list_for_each_entry(tmp, &hdev->mgmt_pending, list) {
> -               if (cmd == tmp) {
> -                       if (remove_unlock) {
> -                               list_del(&cmd->list);
> -                               mutex_unlock(&hdev->mgmt_pending_lock);
> -                       }
> +               if (cmd == tmp)
>                         return true;
> -               }
>         }
> +       return false;
> +}
> +
> +bool mgmt_pending_valid(struct hci_dev *hdev, struct mgmt_pending_cmd *cmd)
> +{
> +       struct mgmt_pending_cmd *tmp;
> +       bool listed;
> +
> +       if (!cmd)
> +               return false;
> +
> +       mutex_lock(&hdev->mgmt_pending_lock);
> +
> +       listed = __mgmt_pending_listed(hdev, cmd);
> +       if (listed)
> +               list_del(&cmd->list);
>
>         mutex_unlock(&hdev->mgmt_pending_lock);
> -       return false;
> +
> +       return listed;
>  }
>
>  void mgmt_mesh_foreach(struct hci_dev *hdev,



-- 
Luiz Augusto von Dentz