* [PATCH V3 1/3] Bluetooth: hci_ldisc: Add missing return in hci_uart_init_work()
2017-04-20 17:06 [PATCH V3 0/3] hci_ldisc hci_uart_init_work() fixes Dean Jenkins
@ 2017-04-20 17:06 ` Dean Jenkins
2017-04-20 17:06 ` [PATCH V3 2/3] Bluetooth: hci_ldisc: Ensure hu->hdev set to NULL before freeing hdev Dean Jenkins
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Dean Jenkins @ 2017-04-20 17:06 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Dean Jenkins, Gustavo F . Padovan, Johan Hedberg, linux-bluetooth
If hci_register_dev() returns an error in hci_uart_init_work()
then the HCI_UART_REGISTERED bit gets erroneously set due to
a missing return statement. Therefore, add the missing return
statement.
The consequence of the missing return is that the HCI UART is not
registered but HCI_UART_REGISTERED is set which allows the code
to think that hu->hdev is safe to access but hu->hdev has been
freed so could lead to a crash.
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
drivers/bluetooth/hci_ldisc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index cec4438..1166e3f 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -187,6 +187,7 @@ static void hci_uart_init_work(struct work_struct *work)
hci_free_dev(hu->hdev);
hu->hdev = NULL;
hu->proto->close(hu);
+ return;
}
set_bit(HCI_UART_REGISTERED, &hu->flags);
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH V3 2/3] Bluetooth: hci_ldisc: Ensure hu->hdev set to NULL before freeing hdev
2017-04-20 17:06 [PATCH V3 0/3] hci_ldisc hci_uart_init_work() fixes Dean Jenkins
2017-04-20 17:06 ` [PATCH V3 1/3] Bluetooth: hci_ldisc: Add missing return in hci_uart_init_work() Dean Jenkins
@ 2017-04-20 17:06 ` Dean Jenkins
2017-04-20 17:06 ` [PATCH V3 3/3] Bluetooth: hci_ldisc: Add missing clear HCI_UART_PROTO_READY Dean Jenkins
2017-04-21 16:26 ` [PATCH V3 0/3] hci_ldisc hci_uart_init_work() fixes Marcel Holtmann
3 siblings, 0 replies; 5+ messages in thread
From: Dean Jenkins @ 2017-04-20 17:06 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Dean Jenkins, Gustavo F . Padovan, Johan Hedberg, linux-bluetooth
When hci_register_dev() fails, hu->hdev should be set to NULL before
freeing hdev. This avoids potential use of hu->hdev after it has been
freed.
This commit sets hu->hdev to NULL before calling hci_free_dev() in error
handling scenarios in hci_uart_init_work() and hci_uart_register_dev().
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
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 1166e3f..b1096d1 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -177,6 +177,7 @@ static void hci_uart_init_work(struct work_struct *work)
{
struct hci_uart *hu = container_of(work, struct hci_uart, init_ready);
int err;
+ struct hci_dev *hdev;
if (!test_and_clear_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags))
return;
@@ -184,8 +185,9 @@ static void hci_uart_init_work(struct work_struct *work)
err = hci_register_dev(hu->hdev);
if (err < 0) {
BT_ERR("Can't register HCI device");
- hci_free_dev(hu->hdev);
+ hdev = hu->hdev;
hu->hdev = NULL;
+ hci_free_dev(hdev);
hu->proto->close(hu);
return;
}
@@ -603,6 +605,7 @@ static int hci_uart_register_dev(struct hci_uart *hu)
if (hci_register_dev(hdev) < 0) {
BT_ERR("Can't register HCI device");
+ hu->hdev = NULL;
hci_free_dev(hdev);
return -ENODEV;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH V3 3/3] Bluetooth: hci_ldisc: Add missing clear HCI_UART_PROTO_READY
2017-04-20 17:06 [PATCH V3 0/3] hci_ldisc hci_uart_init_work() fixes Dean Jenkins
2017-04-20 17:06 ` [PATCH V3 1/3] Bluetooth: hci_ldisc: Add missing return in hci_uart_init_work() Dean Jenkins
2017-04-20 17:06 ` [PATCH V3 2/3] Bluetooth: hci_ldisc: Ensure hu->hdev set to NULL before freeing hdev Dean Jenkins
@ 2017-04-20 17:06 ` Dean Jenkins
2017-04-21 16:26 ` [PATCH V3 0/3] hci_ldisc hci_uart_init_work() fixes Marcel Holtmann
3 siblings, 0 replies; 5+ messages in thread
From: Dean Jenkins @ 2017-04-20 17:06 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Dean Jenkins, Gustavo F . Padovan, Johan Hedberg, linux-bluetooth
Ensure that HCI_UART_PROTO_READY is cleared before close(hu) is
called which closes the Data Link protocol layer.
Therefore, add the missing bit clear of HCI_UART_PROTO_READY to
hci_uart_init_work() so that the flag is cleared when
hci_register_dev fails.
Without the fix, the functions of the Data Link protocol layer could
potentially be accessed after that layer has been closed. This
could lead to a crash as memory would have been freed in that layer.
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
drivers/bluetooth/hci_ldisc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index b1096d1..c53513c 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -188,6 +188,7 @@ static void hci_uart_init_work(struct work_struct *work)
hdev = hu->hdev;
hu->hdev = NULL;
hci_free_dev(hdev);
+ clear_bit(HCI_UART_PROTO_READY, &hu->flags);
hu->proto->close(hu);
return;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH V3 0/3] hci_ldisc hci_uart_init_work() fixes
2017-04-20 17:06 [PATCH V3 0/3] hci_ldisc hci_uart_init_work() fixes Dean Jenkins
` (2 preceding siblings ...)
2017-04-20 17:06 ` [PATCH V3 3/3] Bluetooth: hci_ldisc: Add missing clear HCI_UART_PROTO_READY Dean Jenkins
@ 2017-04-21 16:26 ` Marcel Holtmann
3 siblings, 0 replies; 5+ messages in thread
From: Marcel Holtmann @ 2017-04-21 16:26 UTC (permalink / raw)
To: Dean Jenkins; +Cc: Gustavo F. Padovan, Johan Hedberg, linux-bluetooth
Hi Dean,
> To make things easier for you, I am going to break up my V2 patchset of 16
> patches into smaller patchsets. I think this approach will increase the
> probability of you taking the patches and providing me feedback. Therefore, you
> can concentrate on a few tightly related patches in one go.
>
> This is patchset V3 which are needed fixes before hci_uart_tty_close() can be
> reorganised to overcome a design flaw related to the proper closure of the
> Data Link protocol layer. In the worst case scenario a kernel crash occurs.
>
> Previous Discussions
> ====================
>
> Please refer to the discussion on my patchset V2 that can be found here:
> https://www.spinics.net/lists/linux-bluetooth/msg70183.html
>
> Please refer to the discussion on my RFC patchset V1 that can be found here:
> https://www.spinics.net/lists/linux-bluetooth/msg70077.html
>
>
> Changes between V2 and V3
> =========================
>
> 1. Only presenting the first 3 patches of the V2 patchset. These changes relate
> mainly to handling of the failure to register the hdev device for the h5
> Protocol Data layer protocol.
>
> 2. The remaining 13 patches will be put into smaller patchsets after this first
> patchset has been accepted.
>
> 3. Changed the patchset title from "hci_ldisc hci_uart_tty_close() fixes" to
> "hci_ldisc hci_uart_init_work() fixes" because this patchset does not modify
> hci_uart_tty_close().
>
>
> Changes between V1 and V2
> =========================
>
> 1. Implemented some minor code style changes that were suggested by Marcel
> Holtmann.
> 2. Reordered some of the patches to better group together related changes.
>
> Analysis
> ========
>
> hci_uart_init_work() is used with the h5 Data Link layer protocol. Static code
> inspection reveals issues. Instead of testing with h5, test code was used to
> show that a kernel crash occurs in HCI LDISC.
>
> hci_uart_init_work() is used to register the HCI UART device and sets the
> HCI_UART_REGISTERED flag after the h5 protocol has reached the H5_ACTIVE state
> and calls hci_uart_init_ready(). This procedure is armed by setting the
> HCI_UART_INIT_PENDING flag via hci_uart_tty_ioctl() using HCIUARTSETFLAGS.
>
> During my analysis, it became clear that the hci_uart_init_work() function was
> incomplete. Initially, it caused me confusion in how the design allowed
> HCI_UART_REGISTERED to be set despite hci_register_dev() failing.
>
>
> Test code was added to:
> a) set flag HCI_UART_INIT_PENDING - used with h5 to delay hdev registration
> b) call hci_uart_init_ready() - simulates h5 reaching the H5_ACTIVE state
> c) force hci_register_dev() to fail
>
> This testcase caused a kernel crash because hdev was freed in
> hci_uart_init_work() but the HCI_UART_REGISTERED flag was set. Therefore,
> the HCI_UART_REGISTERED flag is erroneously set on the failure of
> hci_register_dev(). This is fixed in the following patch by adding a missing
> return statement:
>
> Bluetooth: hci_ldisc: Add missing return in hci_uart_init_work()
>
>
> Analysis of hu->hdev showed that the hdev member of hu could remain present
> despite hdev having been freed. This is potentially dangerous as there is a risk
> of using hu->hdev after hdev was freed. This is fixed in patch:
>
> Bluetooth: hci_ldisc: Ensure hu->hdev set to NULL before freeing hdev
>
>
> Analysis of the flag HCI_UART_PROTO_READY showed an inconsistency for
> hci_register_dev() failing in hci_uart_init_work(). The oversight is that
> HCI_UART_PROTO_READY must be cleared before the close "proto" function pointer
> is called as per hci_uart_set_proto() and hci_uart_tty_close(). This is fixed
> in patch:
>
> Bluetooth: hci_ldisc: Add missing clear HCI_UART_PROTO_READY
>
>
> Patch list
> ==========
>
> The patches were rebased onto the bluetooth-next/master branch commit:
> 6493b63 ieee802154: don't select COMMON_CLK
>
> Dean Jenkins (3):
> Bluetooth: hci_ldisc: Add missing return in hci_uart_init_work()
> Bluetooth: hci_ldisc: Ensure hu->hdev set to NULL before freeing hdev
> Bluetooth: hci_ldisc: Add missing clear HCI_UART_PROTO_READY
>
> drivers/bluetooth/hci_ldisc.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
all 3 patches have been applied to bluetooth-next tree.
Regards
Marcel
^ permalink raw reply [flat|nested] 5+ messages in thread