How to reproduce the BUG general protection fault in hci_uart_tty_ioctl?

How to reproduce the BUG general protection fault in hci_uart_tty_ioctl?

[joeyli]

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@mail.gmail.com/

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

Re: How to reproduce the BUG general protection fault in hci_uart_tty_ioctl?

[Yu Hao]

Hi Weiteng,

Could you give more info about the bug, e.g., kernel configuration,
qemu arguments.

Thanks.
Yu Hao
Ph.D. student
Department of Computer Science & Engineering
University of California, Riverside

On Wed, Jun 28, 2023 at 8:02 AM joeyli <jlee@suse.com> 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@mail.gmail.com/
>
> 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

Re: How to reproduce the BUG general protection fault in hci_uart_tty_ioctl?

[joeyli]

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@suse.com> 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@mail.gmail.com/
> >
> > 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

Re: How to reproduce the BUG general protection fault in hci_uart_tty_ioctl?

[Weiteng Chen]

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.

Best,
Weiteng Chen

> On Jul 3, 2023, at 8:01 PM, joeyli <jlee@suse.com> 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@suse.com> 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@mail.gmail.com/
>>> 
>>> 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

Re: How to reproduce the BUG general protection fault in hci_uart_tty_ioctl?

[joeyli]

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


> Best,
> Weiteng Chen
> 
> > On Jul 3, 2023, at 8:01 PM, joeyli <jlee@suse.com> 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@suse.com> 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@mail.gmail.com/
> >>> 
> >>> 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

Re: How to reproduce the BUG general protection fault in hci_uart_tty_ioctl?

[joeyli]

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.

Thanks
Joey Lee

> 
> 
> > Best,
> > Weiteng Chen
> > 
> > > On Jul 3, 2023, at 8:01 PM, joeyli <jlee@suse.com> 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@suse.com> 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@mail.gmail.com/
> > >>> 
> > >>> 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

Re: How to reproduce the BUG general protection fault in hci_uart_tty_ioctl?

[joeyli]

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@suse.com> 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@suse.com> 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@mail.gmail.com/
> > > >>> 
> > > >>> 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