Skip to contentSkip to conversation

How to reproduce the BUG general protection fault in hci_uart_tty_ioctl?

DISCUSSION joeyli 3 participants 7 messages Jun 28, 2023 – Jul 5, 2023 Bluetooth drivers

lore
Share & tools
lore

joeyli

author
#0 lore raw

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

#1 lore raw

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

Quoted 17 lines · 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

author
#2 lore raw

Hi,

Quoted 5 lines · Yu Hao

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

Quoted 19 lines · joeyli
Quoted 17 lines

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

#3 lore raw

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

Quoted 40 lines · joeyli

Hi,

Quoted 5 lines · Yu Hao

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

Quoted 19 lines · joeyli
Quoted 17 lines

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

author
#4 lore raw

Hi Weiteng Chen, Yu Hao,

Quoted 24 lines · 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;
Quoted 4 lines

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

Quoted 43 lines

Best,
Weiteng Chen

Quoted 40 lines · joeyli

Hi,

Quoted 5 lines · Yu Hao

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

Quoted 19 lines · joeyli
Quoted 17 lines

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

author
#5 lore raw
Quoted 29 lines · joeyli

Hi Weiteng Chen, Yu Hao,

Quoted 26 lines · 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.

Quoted 8 lines
Quoted 2 lines

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:

drivers/bluetooth/hci_ldisc.c +1 -1
@@ -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

Quoted 45 lines
Quoted 45 lines

Best,
Weiteng Chen

Quoted 40 lines · joeyli

Hi,

Quoted 5 lines · Yu Hao

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

Quoted 19 lines · joeyli
Quoted 17 lines

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

author
#6 lore raw
Quoted 53 lines · joeyli
Quoted 29 lines · joeyli

Hi Weiteng Chen, Yu Hao,

Quoted 26 lines · 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.

Quoted 8 lines
Quoted 2 lines

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:

drivers/bluetooth/hci_ldisc.c +1 -1
@@ -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;
Quoted 2 lines

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:

drivers/bluetooth/hci_ldisc.c +2 -1
@@ -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

Quoted 45 lines
Quoted 45 lines
Quoted 45 lines

Best,
Weiteng Chen

Quoted 40 lines · joeyli

Hi,

Quoted 5 lines · Yu Hao

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

Quoted 19 lines · joeyli
Quoted 17 lines

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