diff mbox series

[v2] Bluetooth: hci_qca: Fix driver shutdown on closed serdev

Message ID 20221229102829.403917-1-krzysztof.kozlowski@linaro.org
State New
Headers
Series [v2] Bluetooth: hci_qca: Fix driver shutdown on closed serdev |

Commit Message

Krzysztof Kozlowski Dec. 29, 2022, 10:28 a.m. UTC 
  The driver shutdown callback (which sends EDL_SOC_RESET to the device
over serdev) should not be invoked when HCI device is not open (e.g. if
hci_dev_open_sync() failed), because the serdev and its TTY are not open
either.  Also skip this step if device is powered off
(qca_power_shutdown()).

The shutdown callback causes use-after-free during system reboot with
Qualcomm Atheros Bluetooth:

  Unable to handle kernel paging request at virtual address 0072662f67726fd7
  ...
  CPU: 6 PID: 1 Comm: systemd-shutdow Tainted: G        W          6.1.0-rt5-00325-g8a5f56bcfcca #8
  Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
  Call trace:
   tty_driver_flush_buffer+0x4/0x30
   serdev_device_write_flush+0x24/0x34
   qca_serdev_shutdown+0x80/0x130 [hci_uart]
   device_shutdown+0x15c/0x260
   kernel_restart+0x48/0xac

KASAN report:

  BUG: KASAN: use-after-free in tty_driver_flush_buffer+0x1c/0x50
  Read of size 8 at addr ffff16270c2e0018 by task systemd-shutdow/1

  CPU: 7 PID: 1 Comm: systemd-shutdow Not tainted 6.1.0-next-20221220-00014-gb85aaf97fb01-dirty #28
  Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
  Call trace:
   dump_backtrace.part.0+0xdc/0xf0
   show_stack+0x18/0x30
   dump_stack_lvl+0x68/0x84
   print_report+0x188/0x488
   kasan_report+0xa4/0xf0
   __asan_load8+0x80/0xac
   tty_driver_flush_buffer+0x1c/0x50
   ttyport_write_flush+0x34/0x44
   serdev_device_write_flush+0x48/0x60
   qca_serdev_shutdown+0x124/0x274
   device_shutdown+0x1e8/0x350
   kernel_restart+0x48/0xb0
   __do_sys_reboot+0x244/0x2d0
   __arm64_sys_reboot+0x54/0x70
   invoke_syscall+0x60/0x190
   el0_svc_common.constprop.0+0x7c/0x160
   do_el0_svc+0x44/0xf0
   el0_svc+0x2c/0x6c
   el0t_64_sync_handler+0xbc/0x140
   el0t_64_sync+0x190/0x194

Fixes: 7e7bbddd029b ("Bluetooth: hci_qca: Fix qca6390 enable failure after warm reboot")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Changes since v1:
1. Drop serdev patch and fix it only on BT side.
2. Update commit msg.
---
 drivers/bluetooth/hci_qca.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Krzysztof Kozlowski Jan. 12, 2023, 3:20 p.m. UTC | #1 
On 29/12/2022 11:28, Krzysztof Kozlowski wrote:
> The driver shutdown callback (which sends EDL_SOC_RESET to the device
> over serdev) should not be invoked when HCI device is not open (e.g. if
> hci_dev_open_sync() failed), because the serdev and its TTY are not open
> either.  Also skip this step if device is powered off
> (qca_power_shutdown()).
> 
> The shutdown callback causes use-after-free during system reboot with
> Qualcomm Atheros Bluetooth:
> 
>   Unable to handle kernel paging request at virtual address 0072662f67726fd7
>   ...
>   CPU: 6 PID: 1 Comm: systemd-shutdow Tainted: G        W          6.1.0-rt5-00325-g8a5f56bcfcca #8
>   Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
>   Call trace:
>    tty_driver_flush_buffer+0x4/0x30
>    serdev_device_write_flush+0x24/0x34
>    qca_serdev_shutdown+0x80/0x130 [hci_uart]
>    device_shutdown+0x15c/0x260
>    kernel_restart+0x48/0xac
> 
> KASAN report:
> 
>   BUG: KASAN: use-after-free in tty_driver_flush_buffer+0x1c/0x50
>   Read of size 8 at addr ffff16270c2e0018 by task systemd-shutdow/1
> 
>   CPU: 7 PID: 1 Comm: systemd-shutdow Not tainted 6.1.0-next-20221220-00014-gb85aaf97fb01-dirty #28
>   Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
>   Call trace:
>    dump_backtrace.part.0+0xdc/0xf0
>    show_stack+0x18/0x30
>    dump_stack_lvl+0x68/0x84
>    print_report+0x188/0x488
>    kasan_report+0xa4/0xf0
>    __asan_load8+0x80/0xac
>    tty_driver_flush_buffer+0x1c/0x50
>    ttyport_write_flush+0x34/0x44
>    serdev_device_write_flush+0x48/0x60
>    qca_serdev_shutdown+0x124/0x274
>    device_shutdown+0x1e8/0x350
>    kernel_restart+0x48/0xb0
>    __do_sys_reboot+0x244/0x2d0
>    __arm64_sys_reboot+0x54/0x70
>    invoke_syscall+0x60/0x190
>    el0_svc_common.constprop.0+0x7c/0x160
>    do_el0_svc+0x44/0xf0
>    el0_svc+0x2c/0x6c
>    el0t_64_sync_handler+0xbc/0x140
>    el0t_64_sync+0x190/0x194
> 
> Fixes: 7e7bbddd029b ("Bluetooth: hci_qca: Fix qca6390 enable failure after warm reboot")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> ---

Any comments on this? Patchwork tools complain on longer line, but
without it checkpatch would complain as well, so I assume you do not
expect to fix it?

Best regards,
Krzysztof
Luiz Augusto von Dentz Jan. 12, 2023, 4:07 p.m. UTC | #2 
Hi Krzysztof,

On Thu, Jan 12, 2023 at 7:20 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 29/12/2022 11:28, Krzysztof Kozlowski wrote:
> > The driver shutdown callback (which sends EDL_SOC_RESET to the device
> > over serdev) should not be invoked when HCI device is not open (e.g. if
> > hci_dev_open_sync() failed), because the serdev and its TTY are not open
> > either.  Also skip this step if device is powered off
> > (qca_power_shutdown()).
> >
> > The shutdown callback causes use-after-free during system reboot with
> > Qualcomm Atheros Bluetooth:
> >
> >   Unable to handle kernel paging request at virtual address 0072662f67726fd7
> >   ...
> >   CPU: 6 PID: 1 Comm: systemd-shutdow Tainted: G        W          6.1.0-rt5-00325-g8a5f56bcfcca #8
> >   Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
> >   Call trace:
> >    tty_driver_flush_buffer+0x4/0x30
> >    serdev_device_write_flush+0x24/0x34
> >    qca_serdev_shutdown+0x80/0x130 [hci_uart]
> >    device_shutdown+0x15c/0x260
> >    kernel_restart+0x48/0xac
> >
> > KASAN report:
> >
> >   BUG: KASAN: use-after-free in tty_driver_flush_buffer+0x1c/0x50
> >   Read of size 8 at addr ffff16270c2e0018 by task systemd-shutdow/1
> >
> >   CPU: 7 PID: 1 Comm: systemd-shutdow Not tainted 6.1.0-next-20221220-00014-gb85aaf97fb01-dirty #28
> >   Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
> >   Call trace:
> >    dump_backtrace.part.0+0xdc/0xf0
> >    show_stack+0x18/0x30
> >    dump_stack_lvl+0x68/0x84
> >    print_report+0x188/0x488
> >    kasan_report+0xa4/0xf0
> >    __asan_load8+0x80/0xac
> >    tty_driver_flush_buffer+0x1c/0x50
> >    ttyport_write_flush+0x34/0x44
> >    serdev_device_write_flush+0x48/0x60
> >    qca_serdev_shutdown+0x124/0x274
> >    device_shutdown+0x1e8/0x350
> >    kernel_restart+0x48/0xb0
> >    __do_sys_reboot+0x244/0x2d0
> >    __arm64_sys_reboot+0x54/0x70
> >    invoke_syscall+0x60/0x190
> >    el0_svc_common.constprop.0+0x7c/0x160
> >    do_el0_svc+0x44/0xf0
> >    el0_svc+0x2c/0x6c
> >    el0t_64_sync_handler+0xbc/0x140
> >    el0t_64_sync+0x190/0x194
> >
> > Fixes: 7e7bbddd029b ("Bluetooth: hci_qca: Fix qca6390 enable failure after warm reboot")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >
> > ---
>
> Any comments on this? Patchwork tools complain on longer line, but
> without it checkpatch would complain as well, so I assume you do not
> expect to fix it?

I did apply it already:

https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=a18fca670e14f0c09c2ed332cf2c6d77a4ae05f9

Not sure why the bot hasn't responded to you.

> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Jan. 12, 2023, 4:13 p.m. UTC | #3 
On 12/01/2023 17:07, Luiz Augusto von Dentz wrote:
>>> Fixes: 7e7bbddd029b ("Bluetooth: hci_qca: Fix qca6390 enable failure after warm reboot")
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>
>>> ---
>>
>> Any comments on this? Patchwork tools complain on longer line, but
>> without it checkpatch would complain as well, so I assume you do not
>> expect to fix it?
> 
> I did apply it already:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=a18fca670e14f0c09c2ed332cf2c6d77a4ae05f9
> 
> Not sure why the bot hasn't responded to you.


Ah, thank you then. I looked at Patchwork and it was still in state "New".

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index bb7623fe53a8..157fc4d024c1 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -2166,10 +2166,16 @@  static void qca_serdev_shutdown(struct device *dev)
 	int timeout = msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS);
 	struct serdev_device *serdev = to_serdev_device(dev);
 	struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
+	struct hci_uart *hu = &qcadev->serdev_hu;
+	struct hci_dev *hdev = hu->hdev;
+	struct qca_data *qca = hu->priv;
 	const u8 ibs_wake_cmd[] = { 0xFD };
 	const u8 edl_reset_soc_cmd[] = { 0x01, 0x00, 0xFC, 0x01, 0x05 };
 
 	if (qcadev->btsoc_type == QCA_QCA6390) {
+		if (test_bit(QCA_BT_OFF, &qca->flags) || !test_bit(HCI_RUNNING, &hdev->flags))
+			return;
+
 		serdev_device_write_flush(serdev);
 		ret = serdev_device_write_buf(serdev, ibs_wake_cmd,
 					      sizeof(ibs_wake_cmd));