linux-kernel - Re: [PATCH v2] Bluetooth: hci_uart: fix race during initialization

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: <CABBYNZL4AAdr3HaOQheSmHr2TMWpH2bXNgwn20tGSbuewys_SQ@mail.gmail.com>
Date: Thu, 30 Jan 2025 13:37:22 -0500
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: Arseniy Krasnov <avkrasnov@...utedevices.com>
Cc: Marcel Holtmann <marcel@...tmann.org>, oxffffaa@...il.com, 
	linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org, 
	kernel@...utedevices.com
Subject: Re: [PATCH v2] Bluetooth: hci_uart: fix race during initialization

Hi Arseniy,

On Thu, Jan 30, 2025 at 1:34 PM Arseniy Krasnov
<avkrasnov@...utedevices.com> wrote:
>
> Hi, sorry ping
>
> Thanks
>
> On 08.01.2025 15:39, Arseniy Krasnov wrote:
> >
> >
> > On 19.12.2024 23:07, Arseniy Krasnov wrote:
> >>
> >>
> >> On 19.12.2024 22:18, Luiz Augusto von Dentz wrote:
> >>> Hi Arseniy,
> >>>
> >>> On Tue, Dec 17, 2024 at 3:12 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 moves setting of 'HCI_UART_PROTO_READY' before calling function
> >>>> 'hci_uart_register_dev()'.
> >>>>
> >>>> Signed-off-by: Arseniy Krasnov <avkrasnov@...utedevices.com>
> >>>> ---
> >>>>  Changelog v1->v2:
> >>>>  * Move 'set_bit()' before 'hci_uart_register_dev()' instead of
> >>>>    adding new bit 'HCI_UART_PROTO_INIT'.
> >>>
> >>> What drivers/controllers this was tested with? I want to make sure
> >>> this doesn't cause regressions to other drivers if there are perhaps
> >>> some drivers assuming HCI_UART_PROTO_READY was set in a certain order.
> >>
> >> Hi,
> >>
> >> I tested this on:
> >>
> >> CONFIG_BT=y
> >> CONFIG_BT_HCIUART=y
> >> CONFIG_BT_HCIUART_H4=y
> >
> > Hi, sorry, but is this enough info about my test env ?
> >
> > Thanks
> >
> >>
> >> Yes, my v1 patchset with extra INIT bit was targeted to keep
> >> original behaviour - e.g. PROTO_READY bit usage still the same,
> >> just adding extra bit to handle this specific case.
> >>
> >> Thanks
> >>
> >>>
> >>>>  drivers/bluetooth/hci_ldisc.c | 3 ++-
> >>>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> >>>> index 30192bb083549..07b9aa09bbe2e 100644
> >>>> --- a/drivers/bluetooth/hci_ldisc.c
> >>>> +++ b/drivers/bluetooth/hci_ldisc.c
> >>>> @@ -704,12 +704,13 @@ static int hci_uart_set_proto(struct hci_uart *hu, int id)
> >>>>
> >>>>         hu->proto = p;
> >>>>
> >>>> +       set_bit(HCI_UART_PROTO_READY, &hu->flags);
> >>>> +
> >>>>         err = hci_uart_register_dev(hu);
> >>>>         if (err) {
> >>>>                 return err;
> >>>>         }
> >>>>
> >>>> -       set_bit(HCI_UART_PROTO_READY, &hu->flags);
> >>>>         return 0;
> >>>>  }
> >>>>
> >>>> --
> >>>> 2.30.1
> >>>
> >>>
> >>>

Please resend it.

-- 
Luiz Augusto von Dentz