2013-03-27 17:09:07

by Chan-yeol Park

[permalink] [raw]
Subject: [PATCH v4 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 17:09:09

by Chan-yeol Park

[permalink] [raw]
Subject: [PATCH v4 3/3] Bluetooth: Remove trivial white space

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

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index d710d8b..bc68a44 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)
@@ -365,15 +365,15 @@ static void hci_uart_tty_wakeup(struct tty_struct *tty)
}

/* hci_uart_tty_receive()
- *
+ *
* Called by tty low level driver when receive data is
* available.
- *
+ *
* Arguments: tty pointer to tty isntance data
* data pointer to received data
* flags pointer to flags for data
* count count of received data in bytes
- *
+ *
* Return Value: None
*/
static void hci_uart_tty_receive(struct tty_struct *tty, const u8 *data, char *flags, int count)
--
1.7.10.4


2013-03-27 17:09:08

by Chan-yeol Park

[permalink] [raw]
Subject: [PATCH v4 2/3] Bluetooth: Fix possible NULL dereference

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

hci h4 driever's backtrace is attached.

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 | 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-04-04 08:25:25

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH v4 1/3] Bluetooth: Fix hci-uart-h4 crash from incoming uart packet

Hi Chan-yeol,

On Tue, Apr 02, 2013, Chan-yeol Park wrote:
> 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(+)

After some language fixes to the commit messages all three patches in
this set have now been pushed to the bluetooth-next tree.

Johan

2013-04-03 07:46:57

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH v4 1/3] Bluetooth: Fix hci-uart-h4 crash from incoming uart packet

Hi Chan-yeol,

On Thu, Mar 28, 2013, Chan-yeol Park wrote:
> 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(+)

All patches in this set look fine to me.

Acked-by: Johan Hedberg <[email protected]>

Johan

2013-04-02 09:23:32

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH v4 2/3] Bluetooth: Fix possible NULL dereference

Hi Chan-yeol,

On Thu, Mar 28, 2013, Chan-yeol Park wrote:
> This patch adds NULL check for hci uart ldisc driver because some of
> hci uart driver allow hci_uart_tty_receive function could be called
> though hci uart driver is not registered properly.
>
> hci h4 driever's backtrace is attached.
>
> 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 | 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);

All patches in this set seem fine to me, except that the backtrace
you've got in this commit message doesn't seem to match the issue that
it is fixing. If there's a NULL pointer dereference related issue (if
hu->hdev is NULL) then the last function in the trace should be
hci_uart_tty_receive and not hci_recv_stream_fragment.

Johan