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: <CABBYNZL6ZJkf=jQpdUyxYxZZwqRjHxWzDk4nCb74xdB78-33yg@mail.gmail.com>
Date: Mon, 16 Dec 2024 09:52:10 -0500
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: Arseniy Krasnov <avkrasnov@...utedevices.com>
Cc: Marcel Holtmann <marcel@...tmann.org>, linux-bluetooth@...r.kernel.org, 
	linux-kernel@...r.kernel.org, oxffffaa@...il.com, ddrokosov@...utedevices.com
Subject: Re: [PATCH v1] Bluetooth: hci_uart: fix race during initialization

Hi Arseniy,

On Wed, Nov 27, 2024 at 7:38 AM Arseniy Krasnov
<avkrasnov@...utedevices.com> wrote:
>
> 'hci_register_dev()' calls power up function, which is executed by
> kworker - 'hci_power_on()'. This function does access to bluetooth chip
> using callbacks from 'hci_ldisc.c', for example 'hci_uart_send_frame()'.
> Now 'hci_uart_send_frame()' checks 'HCI_UART_PROTO_READY' bit set, and
> if not - it fails. Problem is that 'HCI_UART_PROTO_READY' is set after
> 'hci_register_dev()', and there is tiny chance that 'hci_power_on()' will
> be executed before setting this bit. In that case HCI init logic fails.
>
> Patch adds one more bit in addition to 'HCI_UART_PROTO_READY' which
> allows to perform power up logic without original bit set.
>
> Signed-off-by: Arseniy Krasnov <avkrasnov@...utedevices.com>
> ---
>  drivers/bluetooth/hci_ldisc.c | 19 ++++++++++++++-----
>  drivers/bluetooth/hci_uart.h  |  1 +
>  2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index 30192bb083549..2e20f00649bd7 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -102,7 +102,8 @@ static inline struct sk_buff *hci_uart_dequeue(struct hci_uart *hu)
>         if (!skb) {
>                 percpu_down_read(&hu->proto_lock);
>
> -               if (test_bit(HCI_UART_PROTO_READY, &hu->flags))
> +               if (test_bit(HCI_UART_PROTO_READY, &hu->flags) ||
> +                   test_bit(HCI_UART_PROTO_INIT, &hu->flags))
>                         skb = hu->proto->dequeue(hu);
>
>                 percpu_up_read(&hu->proto_lock);
> @@ -124,7 +125,8 @@ int hci_uart_tx_wakeup(struct hci_uart *hu)
>         if (!percpu_down_read_trylock(&hu->proto_lock))
>                 return 0;
>
> -       if (!test_bit(HCI_UART_PROTO_READY, &hu->flags))
> +       if (!test_bit(HCI_UART_PROTO_READY, &hu->flags) &&
> +           !test_bit(HCI_UART_PROTO_INIT, &hu->flags))
>                 goto no_schedule;
>
>         set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
> @@ -278,7 +280,8 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
>
>         percpu_down_read(&hu->proto_lock);
>
> -       if (!test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
> +       if (!test_bit(HCI_UART_PROTO_READY, &hu->flags) &&
> +           !test_bit(HCI_UART_PROTO_INIT, &hu->flags)) {
>                 percpu_up_read(&hu->proto_lock);
>                 return -EUNATCH;
>         }
> @@ -582,7 +585,8 @@ static void hci_uart_tty_wakeup(struct tty_struct *tty)
>         if (tty != hu->tty)
>                 return;
>
> -       if (test_bit(HCI_UART_PROTO_READY, &hu->flags))
> +       if (test_bit(HCI_UART_PROTO_READY, &hu->flags) ||
> +           test_bit(HCI_UART_PROTO_INIT, &hu->flags))
>                 hci_uart_tx_wakeup(hu);
>  }
>
> @@ -608,7 +612,8 @@ static void hci_uart_tty_receive(struct tty_struct *tty, const u8 *data,
>
>         percpu_down_read(&hu->proto_lock);
>
> -       if (!test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
> +       if (!test_bit(HCI_UART_PROTO_READY, &hu->flags) &&
> +           !test_bit(HCI_UART_PROTO_INIT, &hu->flags)) {
>                 percpu_up_read(&hu->proto_lock);
>                 return;
>         }
> @@ -704,12 +709,16 @@ static int hci_uart_set_proto(struct hci_uart *hu, int id)
>
>         hu->proto = p;
>
> +       set_bit(HCI_UART_PROTO_INIT, &hu->flags);
> +
>         err = hci_uart_register_dev(hu);
>         if (err) {
>                 return err;
>         }
>
>         set_bit(HCI_UART_PROTO_READY, &hu->flags);
> +       clear_bit(HCI_UART_PROTO_INIT, &hu->flags);

This should be quite obvious, why don't you just move the
HCI_UART_PROTO_READY above hci_uart_register_dev?

>         return 0;
>  }
>
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index 00bf7ae82c5b7..39f39704be41f 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -89,6 +89,7 @@ struct hci_uart {
>  #define HCI_UART_REGISTERED            1
>  #define HCI_UART_PROTO_READY           2
>  #define HCI_UART_NO_SUSPEND_NOTIFIER   3
> +#define HCI_UART_PROTO_INIT            4
>
>  /* TX states  */
>  #define HCI_UART_SENDING       1
> --
> 2.30.1



-- 
Luiz Augusto von Dentz

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ