On Wed, Jul 05, 2023 at 05:36:36PM +0800, joeyli wrote: > On Wed, Jul 05, 2023 at 04:41:48PM +0800, joeyli wrote: > > Hi Weiteng Chen, Yu Hao, > > > > On Mon, Jul 03, 2023 at 09:07:38PM -0700, Weiteng Chen wrote: > > > Hi Joey, > > > > > > Sorry for my late response. > > > > > > https://elixir.bootlin.com/linux/v6.3-rc7/source/drivers/bluetooth/hci_ldisc.c#L764 > > > > > > switch (cmd) { > > > case HCIUARTSETPROTO: > > > if (!test_and_set_bit(HCI_UART_PROTO_SET, &hu->flags)) { > > > printk(“test_and_set_bit…”) // insert a prink to make the race easy to trigger > > > err = hci_uart_set_proto(hu, arg); > > > if (err) > > > clear_bit(HCI_UART_PROTO_SET, &hu->flags); > > > } else > > > err = -EBUSY; > > > break; > > > > > > case HCIUARTGETPROTO: > > > if (test_bit(HCI_UART_PROTO_SET, &hu->flags)) > > > err = hu->proto->id; ←- null pointer deference > > > else > > > err = -EUNATCH; > > > break; > > > > > > This is a race condition between HCIUARTSETPROTO and HCIUARTGETPROTO. HCI_UART_PROTO_SET is set before hu->proto is set and thus it may dereference a null pointer. > > > > > > To easily trigger this bug, I inserted a prink in the source code so that the C producer can easily trigger the bug. Please let me know if you have any questions. > > > > > > > Thanks! I can reproduce the issue now. > > > > Weiteng, Yu Hao, do you have plan for sending patch to fix this problem? > > > > Joey Lee > > Looks that check HCI_UART_PROTO_READY is enough to avoid problem: > > --- linux.orig/drivers/bluetooth/hci_ldisc.c > +++ linux/drivers/bluetooth/hci_ldisc.c > @@ -771,7 +771,7 @@ static int hci_uart_tty_ioctl(struct tty > break; > > case HCIUARTGETPROTO: > - if (test_bit(HCI_UART_PROTO_SET, &hu->flags)) > + if (test_bit(HCI_UART_PROTO_READY, &hu->flags)) > err = hu->proto->id; > else > err = -EUNATCH; > > If you do not have plan to send patch, then I will send the above change. > Updated patch. The HCI_UART_PROTO_SET should still be checked with HCI_UART_PROTO_READY: --- linux.orig/drivers/bluetooth/hci_ldisc.c +++ linux/drivers/bluetooth/hci_ldisc.c @@ -771,7 +771,8 @@ static int hci_uart_tty_ioctl(struct tty break; case HCIUARTGETPROTO: - if (test_bit(HCI_UART_PROTO_SET, &hu->flags)) + if (test_bit(HCI_UART_PROTO_SET, &hu->flags) && + test_bit(HCI_UART_PROTO_READY, &hu->flags)) err = hu->proto->id; else err = -EUNATCH; I have tested this patch a couple of hours. I didn't reproduce issue. Regards Joey Lee > > > > > > > Best, > > > Weiteng Chen > > > > > > > On Jul 3, 2023, at 8:01 PM, joeyli <jlee@xxxxxxxx> wrote: > > > > > > > > Hi, > > > > > > > > On Wed, Jun 28, 2023 at 06:57:47PM -0700, Yu Hao wrote: > > > >> Hi Weiteng, > > > >> > > > >> Could you give more info about the bug, e.g., kernel configuration, > > > >> qemu arguments. > > > >> > > > > > > > > Base on kernel code, looks that the HCIUARTSETPROTO and HCIUARTGETPROTO > > > > blocks in hci_uart_tty_ioctl() should use hci_uart->proto_lock. > > > > > > > > I have run the C reproducer a couple of days in qemu, but it did not > > > > reproduce issue until now. > > > > > > > > Does anyone know how to reproduce this issue easily? > > > > > > > > Thanks > > > > Joey Lee > > > >> > > > >> On Wed, Jun 28, 2023 at 8:02 AM joeyli <jlee@xxxxxxxx> wrote: > > > >>> > > > >>> Hi Yu Hao, > > > >>> > > > >>> I am looking at your "BUG: general protection fault in hci_uart_tty_ioctl": > > > >>> > > > >>> https://lore.kernel.org/all/CA+UBctC3p49aTgzbVgkSZ2+TQcqq4fPDO7yZitFT5uBPDeCO2g@xxxxxxxxxxxxxx/ > > > >>> > > > >>> I am trying the C reproducer in your URL, but it is not success yet: > > > >>> https://gist.github.com/ZHYfeng/a3e3ff2bdfea5ed5de5475f0b54d55cb > > > >>> > > > >>> I am using v6.2 mainline kernel to run the C reproducer. > > > >>> > > > >>> Could you please provide suggestions for how to reproduce this issue? > > > >>> And what is your qemu environment for reproducing issue? > > > >>> > > > >>> Thanks a lot! > > > >>> Joey Lee
