2015-06-16 12:55:20

by Chan-yeol Park

Subject: [PATCH 1/2] Bluetooth: hci_uart: Include vendor headers if required

Vendor specific headers should be included only when enabled because
hci_uart does not need it always.

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

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index ac87346..231c622 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -45,8 +45,12 @@
#include <net/bluetooth/bluetooth.h>
#include <net/bluetooth/hci_core.h>

+#ifdef CONFIG_BT_HCIUART_INTEL
#include "btintel.h"
+#endif
+#ifdef CONFIG_BT_HCIUART_BCM
#include "btbcm.h"
+#endif
#include "hci_uart.h"

#define VERSION "2.3"
--
2.1.4

2015-06-17 09:37:58

by Marcel Holtmann

Subject: Re: [PATCH 2/2] Bluetooth: hci_uart: Fix dereferencing of ERR_PTR

Hi Chan-yeol,

>>> If h4_recv() return ERR_PTR instead sk_buff pointer, it should be
>>> cleared once dereference is completed for the further reference such as
>>> h4_recv(), or h4_close().
>>
>> I have no idea what the h4_close has to do with it? Can you explain.
> If h4->rx_skb has ERR_PTR , kfree_skb() would dereference of ERR_PTR. This is easily reproduced on my board when I turn off BT.

I see. kfree_skb is not smart enough.

>>
>>>
>>> Signed-off-by: Chan-yeol Park <[email protected]>
>>> ---
>>> drivers/bluetooth/hci_h4.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/bluetooth/hci_h4.c b/drivers/bluetooth/hci_h4.c
>>> index f7190f0..a8acd99 100644
>>> --- a/drivers/bluetooth/hci_h4.c
>>> +++ b/drivers/bluetooth/hci_h4.c
>>> @@ -133,6 +133,7 @@ static int h4_recv(struct hci_uart *hu, const void *data, int count)
>>> if (IS_ERR(h4->rx_skb)) {
>>> int err = PTR_ERR(h4->rx_skb);
>>> BT_ERR("%s: Frame reassembly failed (%d)", hu->hdev->name, err);
>>> + h4->rx_skb = NULL;
>>> return err;
>>> }

actually lets go with this fix. It is cleaner since you do not need to touch the close functions. However this issue exists in all drivers. So we need to fix all of them.

Regards

Marcel

2015-06-17 04:54:18

by Chan-yeol Park

Subject: Re: [PATCH 1/2] Bluetooth: hci_uart: Include vendor headers if required

Hi Marcel,

On 06/16/2015 11:21 PM, Marcel Holtmann wrote:
> Hi Chan-yeol,
>
>> Vendor specific headers should be included only when enabled because
>> hci_uart does not need it always.
>>
>> Signed-off-by: Chan-yeol Park <[email protected]>
>> ---
>> drivers/bluetooth/hci_ldisc.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>> index ac87346..231c622 100644
>> --- a/drivers/bluetooth/hci_ldisc.c
>> +++ b/drivers/bluetooth/hci_ldisc.c
>> @@ -45,8 +45,12 @@
>> #include <net/bluetooth/bluetooth.h>
>> #include <net/bluetooth/hci_core.h>
>>
>> +#ifdef CONFIG_BT_HCIUART_INTEL
>> #include "btintel.h"
>> +#endif
>
> empty lines here,
>
>> +#ifdef CONFIG_BT_HCIUART_BCM
>> #include "btbcm.h"
>> +#endif
>
> And another empty line here.
>
>> #include "hci_uart.h"
>>
>
> However is this really needed? I did not do this since it essentially results into an empty include which will be optimized out. And it just makes the including code more complex.
>
> If we worry about the extra structs, then we can just move them down into the IS_ENABLED section into the header itself. I did not worry since my assumption is that compiler optimizes unused structs.
>
As you explained, I check my compiler remove unused structure.

Personally I think it's recommended to move extra structs into the
IS_ENABLED section because it make clear.
> Regards
>
> Marcel
>
>
>
Thanks
Chanyeol

2015-06-17 04:39:03

by Chan-yeol Park

Subject: Re: [PATCH 2/2] Bluetooth: hci_uart: Fix dereferencing of ERR_PTR

Hi Marcel,

On 06/16/2015 11:29 PM, Marcel Holtmann wrote:
> Hi Chan-yeol,
>
>> If h4_recv() return ERR_PTR instead sk_buff pointer, it should be
>> cleared once dereference is completed for the further reference such as
>> h4_recv(), or h4_close().
>
> I have no idea what the h4_close has to do with it? Can you explain.
If h4->rx_skb has ERR_PTR , kfree_skb() would dereference of ERR_PTR.
This is easily reproduced on my board when I turn off BT.
>
>>
>> Signed-off-by: Chan-yeol Park <[email protected]>
>> ---
>> drivers/bluetooth/hci_h4.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/bluetooth/hci_h4.c b/drivers/bluetooth/hci_h4.c
>> index f7190f0..a8acd99 100644
>> --- a/drivers/bluetooth/hci_h4.c
>> +++ b/drivers/bluetooth/hci_h4.c
>> @@ -133,6 +133,7 @@ static int h4_recv(struct hci_uart *hu, const void *data, int count)
>> if (IS_ERR(h4->rx_skb)) {
>> int err = PTR_ERR(h4->rx_skb);
>> BT_ERR("%s: Frame reassembly failed (%d)", hu->hdev->name, err);
>> + h4->rx_skb = NULL;
>> return err;
>> }
>
> Isn't this better fixed in h4_recv_buf directly:
Yes it's better. I think if we use ERR_PTR this would be right.
>
> @@ -173,7 +173,7 @@ struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb,
> while (count) {
> int i, len;
>
> - if (!skb) {
> + if (IS_ERR_OR_NULL(skb)) {
> for (i = 0; i < pkts_count; i++) {
> if (buffer[0] != (&pkts[i])->type)
> continue;
>
>>
>> @@ -248,6 +249,7 @@ struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb,
>> break;
>> default:
>> /* Unsupported variable length */
>> +
>
> This change seems totally unrelated.
Sorry, Unexpectedly this blank line is added.
>
>> kfree_skb(skb);
>> return ERR_PTR(-EILSEQ);
>> }
>
> Regards
>
> Marcel
>
>
I would raise v2.

Thanks
Chanyeol

2015-06-16 14:29:27

by Marcel Holtmann

Subject: Re: [PATCH 2/2] Bluetooth: hci_uart: Fix dereferencing of ERR_PTR

Hi Chan-yeol,

> If h4_recv() return ERR_PTR instead sk_buff pointer, it should be
> cleared once dereference is completed for the further reference such as
> h4_recv(), or h4_close().

I have no idea what the h4_close has to do with it? Can you explain.

>
> Signed-off-by: Chan-yeol Park <[email protected]>
> ---
> drivers/bluetooth/hci_h4.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/bluetooth/hci_h4.c b/drivers/bluetooth/hci_h4.c
> index f7190f0..a8acd99 100644
> --- a/drivers/bluetooth/hci_h4.c
> +++ b/drivers/bluetooth/hci_h4.c
> @@ -133,6 +133,7 @@ static int h4_recv(struct hci_uart *hu, const void *data, int count)
> if (IS_ERR(h4->rx_skb)) {
> int err = PTR_ERR(h4->rx_skb);
> BT_ERR("%s: Frame reassembly failed (%d)", hu->hdev->name, err);
> + h4->rx_skb = NULL;
> return err;
> }

Isn't this better fixed in h4_recv_buf directly:

@@ -173,7 +173,7 @@ struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb,
while (count) {
int i, len;

- if (!skb) {
+ if (IS_ERR_OR_NULL(skb)) {
for (i = 0; i < pkts_count; i++) {
if (buffer[0] != (&pkts[i])->type)
continue;

>
> @@ -248,6 +249,7 @@ struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb,
> break;
> default:
> /* Unsupported variable length */
> +

This change seems totally unrelated.

> kfree_skb(skb);
> return ERR_PTR(-EILSEQ);
> }

Regards

Marcel

2015-06-16 14:21:45

by Marcel Holtmann

Subject: Re: [PATCH 1/2] Bluetooth: hci_uart: Include vendor headers if required

Hi Chan-yeol,

> Vendor specific headers should be included only when enabled because
> hci_uart does not need it always.
>
> Signed-off-by: Chan-yeol Park <[email protected]>
> ---
> drivers/bluetooth/hci_ldisc.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index ac87346..231c622 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -45,8 +45,12 @@
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
>
> +#ifdef CONFIG_BT_HCIUART_INTEL
> #include "btintel.h"
> +#endif

empty lines here,

> +#ifdef CONFIG_BT_HCIUART_BCM
> #include "btbcm.h"
> +#endif

And another empty line here.

> #include "hci_uart.h"
>

However is this really needed? I did not do this since it essentially results into an empty include which will be optimized out. And it just makes the including code more complex.

If we worry about the extra structs, then we can just move them down into the IS_ENABLED section into the header itself. I did not worry since my assumption is that compiler optimizes unused structs.

Regards

Marcel

2015-06-16 12:55:21

by Chan-yeol Park

Subject: [PATCH 2/2] Bluetooth: hci_uart: Fix dereferencing of ERR_PTR

If h4_recv() return ERR_PTR instead sk_buff pointer, it should be
cleared once dereference is completed for the further reference such as
h4_recv(), or h4_close().

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

diff --git a/drivers/bluetooth/hci_h4.c b/drivers/bluetooth/hci_h4.c
index f7190f0..a8acd99 100644
--- a/drivers/bluetooth/hci_h4.c
+++ b/drivers/bluetooth/hci_h4.c
@@ -133,6 +133,7 @@ static int h4_recv(struct hci_uart *hu, const void *data, int count)
if (IS_ERR(h4->rx_skb)) {
int err = PTR_ERR(h4->rx_skb);
BT_ERR("%s: Frame reassembly failed (%d)", hu->hdev->name, err);
+ h4->rx_skb = NULL;
return err;
}

@@ -248,6 +249,7 @@ struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb,
break;
default:
/* Unsupported variable length */
+
kfree_skb(skb);
return ERR_PTR(-EILSEQ);
}
--
2.1.4