2013-03-22 13:44:13

by Chan-yeol Park

Subject: [PATCH] Bluetooth: Fix crash with incoming uart packet

This patch adds check HCI_UART_REGISTERED before reading uart packet because
BT uart data could be read before hci_uart is registered.

Backtrace:
[<c05f27ec>] (hci_recv_stream_fragment+0x0/0x74) from [<c04126f4>] (h4_recv+0x18/0x40)
r7:eb1d4d1c r6:eb7683b0 r5:eae8e800 r4:0000000c
[<c04126dc>] (h4_recv+0x0/0x40) from [<c0411870>] (hci_uart_tty_receive+0x6c/0x94)
r5:eae8e800 r4:eb768380
[<c0411804>] (hci_uart_tty_receive+0x0/0x94) from [<c027be88>] (flush_to_ldisc+0x16c/0x17c)
r6:eae8e8d8 r5:eae8e800 r4:eae8e8c8
[<c027bd1c>] (flush_to_ldisc+0x0/0x17c) from [<c0050ae8>] (process_one_work+0x144/0x4d4)
[<c00509a4>] (process_one_work+0x0/0x4d4) from [<c0051208>] (worker_thread+0x180/0x370)
[<c0051088>] (worker_thread+0x0/0x370) from [<c005617c>] (kthread+0x90/0x9c)
[<c00560ec>] (kthread+0x0/0x9c) from [<c003a3a0>] (do_exit+0x0/0x7ec)

Signed-off-by: Chan-yeol Park <[email protected]>
---
drivers/bluetooth/hci_ldisc.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index ed0fade..476a307 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -386,6 +386,9 @@ static void hci_uart_tty_receive(struct tty_struct *tty, const u8 *data, char *f
if (!test_bit(HCI_UART_PROTO_SET, &hu->flags))
return;

+ if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
+ return;
+
spin_lock(&hu->rx_lock);
hu->proto->recv(hu, (void *) data, count);
hu->hdev->stat.byte_rx += count;
--
1.7.10.4

2013-03-27 16:55:00

by Chan-yeol Park

Subject: [PATCH v3 3/3] Bluetooth: Remove trivial white space

---
drivers/bluetooth/hci_ldisc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index d710d8b..69fa0d2 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -260,12 +260,12 @@ static int hci_uart_send_frame(struct sk_buff *skb)

/* ------ LDISC part ------ */
/* hci_uart_tty_open
- *
+ *
* Called when line discipline changed to HCI_UART.
*
* Arguments:
* tty pointer to tty info structure
- * Return Value:
+ * Return Value:
* 0 if success, otherwise error code
*/
static int hci_uart_tty_open(struct tty_struct *tty)
--
1.7.10.4

2013-03-27 16:54:59

by Chan-yeol Park

Subject: [PATCH v3 2/3] Bluetooth: Fix possible NULL dereference

This patch adds NULL check for hci uart ldisc because some of hci uart driver
allow hci_uart_tty_receive function could be called though hci uar driver is
not registered properly.

Signed-off-by: Chan-yeol Park <[email protected]>
---
drivers/bluetooth/hci_ldisc.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index ed0fade..d710d8b 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -388,7 +388,10 @@ static void hci_uart_tty_receive(struct tty_struct *tty, const u8 *data, char *f

spin_lock(&hu->rx_lock);
hu->proto->recv(hu, (void *) data, count);
- hu->hdev->stat.byte_rx += count;
+
+ if (hu->hdev)
+ hu->hdev->stat.byte_rx += count;
+
spin_unlock(&hu->rx_lock);

tty_unthrottle(tty);
--
1.7.10.4

2013-03-27 16:54:58

by Chan-yeol Park

Subject: [PATCH v3 1/3] Bluetooth: Fix hci-uart-h4 crash from incoming uart packet

This patch adds check HCI_UART_REGISTERED before reading uart data for hci uart
h4 driver. Uart data could be arrived when inside the hci_uart_tty_ioctl
function after calling test_and_set_bit for HCI_UART_PROTO_SET but before the
hci_uart_set_proto function has returned.

Backtrace:
[<c05f27ec>] (hci_recv_stream_fragment+0x0/0x74) from [<c04126f4>] (h4_recv+0x18/0x40)
r7:eb1d4d1c r6:eb7683b0 r5:eae8e800 r4:0000000c
[<c04126dc>] (h4_recv+0x0/0x40) from [<c0411870>] (hci_uart_tty_receive+0x6c/0x94)
r5:eae8e800 r4:eb768380
[<c0411804>] (hci_uart_tty_receive+0x0/0x94) from [<c027be88>] (flush_to_ldisc+0x16c/0x17c)
r6:eae8e8d8 r5:eae8e800 r4:eae8e8c8
[<c027bd1c>] (flush_to_ldisc+0x0/0x17c) from [<c0050ae8>] (process_one_work+0x144/0x4d4)
[<c00509a4>] (process_one_work+0x0/0x4d4) from [<c0051208>] (worker_thread+0x180/0x370)
[<c0051088>] (worker_thread+0x0/0x370) from [<c005617c>] (kthread+0x90/0x9c)
[<c00560ec>] (kthread+0x0/0x9c) from [<c003a3a0>] (do_exit+0x0/0x7ec)

Signed-off-by: Chan-yeol Park <[email protected]>
---
drivers/bluetooth/hci_h4.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/bluetooth/hci_h4.c b/drivers/bluetooth/hci_h4.c
index c60623f..8ae9f1e 100644
--- a/drivers/bluetooth/hci_h4.c
+++ b/drivers/bluetooth/hci_h4.c
@@ -153,6 +153,9 @@ static int h4_recv(struct hci_uart *hu, void *data, int count)
{
int ret;

+ if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
+ return -EUNATCH;
+
ret = hci_recv_stream_fragment(hu->hdev, data, count);
if (ret < 0) {
BT_ERR("Frame Reassembly Failed");
--
1.7.10.4

2013-03-27 16:26:15

by Chan-yeol Park

Subject: [PATCH v2] Bluetooth: Fix hci-uart-h4 crash from incoming uart packet

This patch adds check HCI_UART_REGISTERED before reading uart data for hci uart
h4 driver. Uart data could be arrived when inside the hci_uart_tty_ioctl
function after calling test_and_set_bit for HCI_UART_PROTO_SET but before the
hci_uart_set_proto function has returned.

Backtrace:
[<c05f27ec>] (hci_recv_stream_fragment+0x0/0x74) from [<c04126f4>] (h4_recv+0x18/0x40)
r7:eb1d4d1c r6:eb7683b0 r5:eae8e800 r4:0000000c
[<c04126dc>] (h4_recv+0x0/0x40) from [<c0411870>] (hci_uart_tty_receive+0x6c/0x94)
r5:eae8e800 r4:eb768380
[<c0411804>] (hci_uart_tty_receive+0x0/0x94) from [<c027be88>] (flush_to_ldisc+0x16c/0x17c)
r6:eae8e8d8 r5:eae8e800 r4:eae8e8c8
[<c027bd1c>] (flush_to_ldisc+0x0/0x17c) from [<c0050ae8>] (process_one_work+0x144/0x4d4)
[<c00509a4>] (process_one_work+0x0/0x4d4) from [<c0051208>] (worker_thread+0x180/0x370)
[<c0051088>] (worker_thread+0x0/0x370) from [<c005617c>] (kthread+0x90/0x9c)
[<c00560ec>] (kthread+0x0/0x9c) from [<c003a3a0>] (do_exit+0x0/0x7ec)

Signed-off-by: Chan-yeol Park <[email protected]>
---
drivers/bluetooth/hci_h4.c | 3 +++
drivers/bluetooth/hci_ldisc.c | 5 ++++-
2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_h4.c b/drivers/bluetooth/hci_h4.c
index c60623f..8ae9f1e 100644
--- a/drivers/bluetooth/hci_h4.c
+++ b/drivers/bluetooth/hci_h4.c
@@ -153,6 +153,9 @@ static int h4_recv(struct hci_uart *hu, void *data, int count)
{
int ret;

+ if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
+ return -EUNATCH;
+
ret = hci_recv_stream_fragment(hu->hdev, data, count);
if (ret < 0) {
BT_ERR("Frame Reassembly Failed");
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index ed0fade..d710d8b 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -388,7 +388,10 @@ static void hci_uart_tty_receive(struct tty_struct *tty, const u8 *data, char *f

spin_lock(&hu->rx_lock);
hu->proto->recv(hu, (void *) data, count);
- hu->hdev->stat.byte_rx += count;
+
+ if (hu->hdev)
+ hu->hdev->stat.byte_rx += count;
+
spin_unlock(&hu->rx_lock);

tty_unthrottle(tty);
--
1.7.10.4

2013-03-25 09:59:56

by Johan Hedberg

Subject: Re: [PATCH] Bluetooth: Fix crash with incoming uart packet

Hi Chan-yeol,

On Fri, Mar 22, 2013, Chan-yeol Park wrote:
> This patch adds check HCI_UART_REGISTERED before reading uart packet because
> BT uart data could be read before hci_uart is registered.
>
> Backtrace:
> [<c05f27ec>] (hci_recv_stream_fragment+0x0/0x74) from [<c04126f4>] (h4_recv+0x18/0x40)
> r7:eb1d4d1c r6:eb7683b0 r5:eae8e800 r4:0000000c
> [<c04126dc>] (h4_recv+0x0/0x40) from [<c0411870>] (hci_uart_tty_receive+0x6c/0x94)
> r5:eae8e800 r4:eb768380
> [<c0411804>] (hci_uart_tty_receive+0x0/0x94) from [<c027be88>] (flush_to_ldisc+0x16c/0x17c)
> r6:eae8e8d8 r5:eae8e800 r4:eae8e8c8
> [<c027bd1c>] (flush_to_ldisc+0x0/0x17c) from [<c0050ae8>] (process_one_work+0x144/0x4d4)
> [<c00509a4>] (process_one_work+0x0/0x4d4) from [<c0051208>] (worker_thread+0x180/0x370)
> [<c0051088>] (worker_thread+0x0/0x370) from [<c005617c>] (kthread+0x90/0x9c)
> [<c00560ec>] (kthread+0x0/0x9c) from [<c003a3a0>] (do_exit+0x0/0x7ec)
>
> Signed-off-by: Chan-yeol Park <[email protected]>
> ---
> drivers/bluetooth/hci_ldisc.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index ed0fade..476a307 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -386,6 +386,9 @@ static void hci_uart_tty_receive(struct tty_struct *tty, const u8 *data, char *f
> if (!test_bit(HCI_UART_PROTO_SET, &hu->flags))
> return;
>
> + if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
> + return;
> +
> spin_lock(&hu->rx_lock);
> hu->proto->recv(hu, (void *) data, count);
> hu->hdev->stat.byte_rx += count;

Wouldn't this break HCI drivers that have a transport specific init
sequence, like H5, i.e. those that rely on transmitting data before the
HCI device is registered?

I think your commit message should more clearly explain how you manage
to reproduce this condition. It seems to me like data arrives when
inside the hci_uart_tty_ioctl function after calling test_and_set_bit
for HCI_UART_PROTO_SET but before the hci_uart_set_proto function has
returned (and thus called hci_register_dev in the case of H4). Is that
correct?

Johan