Hi Marcel,
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(-)
--
2.7.4
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[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 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