linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH] Bluetooth: hci_event: Ignore multiple conn complete events
@ 2022-01-21 17:36 Soenke Huster
  2022-01-21 18:04 ` [RFC] " bluez.test.bot
  2022-01-21 18:22 ` [RFC PATCH] " Sönke Huster
  0 siblings, 2 replies; 8+ messages in thread
From: Soenke Huster @ 2022-01-21 17:36 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz,
	David S. Miller, Jakub Kicinski
  Cc: Soenke Huster, linux-bluetooth, netdev, linux-kernel

When a HCI_CONNECTION_COMPLETE event is received multiple times
for the same handle, the device is registered multiple times which leads
to memory corruptions. Therefore, consequent events for a single
connection are ignored.

The conn->state can hold different values so conn->handle is
checked to detect whether a connection is already set up.

Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=215497
Signed-off-by: Soenke Huster <soenke.huster@eknoes.de>
---
This fixes the referenced bug and several use-after-free issues I discovered.
I tagged it as RFC, as I am not 100% sure if checking the existence of the
handle is the correct approach, but to the best of my knowledge it must be
set for the first time in this function for valid connections of this event,
therefore it should be fine.

net/bluetooth/hci_event.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 681c623aa380..71ccb12c928d 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3106,6 +3106,17 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
 		}
 	}
 
+	/* The HCI_Connection_Complete event is only sent once per connection.
+	 * Processing it more than once per connection can corrupt kernel memory.
+	 *
+	 * As the connection handle is set here for the first time, it indicates
+	 * whether the connection is already set up.
+	 */
+	if (conn->handle) {
+		bt_dev_err(hdev, "Ignoring HCI_Connection_Complete for existing connection");
+		goto unlock;
+	}
+
 	if (!ev->status) {
 		conn->handle = __le16_to_cpu(ev->handle);
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* RE: [RFC] Bluetooth: hci_event: Ignore multiple conn complete events
  2022-01-21 17:36 [RFC PATCH] Bluetooth: hci_event: Ignore multiple conn complete events Soenke Huster
@ 2022-01-21 18:04 ` bluez.test.bot
  2022-01-21 18:22 ` [RFC PATCH] " Sönke Huster
  1 sibling, 0 replies; 8+ messages in thread
From: bluez.test.bot @ 2022-01-21 18:04 UTC (permalink / raw)
  To: linux-bluetooth, soenke.huster

[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=607341

---Test result---

Test Summary:
CheckPatch                    PASS      0.76 seconds
GitLint                       PASS      0.37 seconds
SubjectPrefix                 PASS      0.23 seconds
BuildKernel                   PASS      35.48 seconds
BuildKernel32                 PASS      32.61 seconds
Incremental Build with patchesPASS      43.24 seconds
TestRunner: Setup             PASS      547.21 seconds
TestRunner: l2cap-tester      PASS      14.72 seconds
TestRunner: bnep-tester       PASS      6.56 seconds
TestRunner: mgmt-tester       PASS      118.55 seconds
TestRunner: rfcomm-tester     PASS      8.44 seconds
TestRunner: sco-tester        PASS      8.91 seconds
TestRunner: smp-tester        PASS      8.40 seconds
TestRunner: userchan-tester   PASS      7.11 seconds



---
Regards,
Linux Bluetooth


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] Bluetooth: hci_event: Ignore multiple conn complete events
  2022-01-21 17:36 [RFC PATCH] Bluetooth: hci_event: Ignore multiple conn complete events Soenke Huster
  2022-01-21 18:04 ` [RFC] " bluez.test.bot
@ 2022-01-21 18:22 ` Sönke Huster
  2022-01-21 21:31   ` Luiz Augusto von Dentz
  1 sibling, 1 reply; 8+ messages in thread
From: Sönke Huster @ 2022-01-21 18:22 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz,
	David S. Miller, Jakub Kicinski
  Cc: linux-bluetooth, netdev, linux-kernel

I just noticed that just checking for handle does not work, as obviously 0x0 could also be a handle value and therefore it can't be distinguished, whether it is not set yet or it is 0x0.

On 21.01.22 18:36, Soenke Huster wrote:
> When a HCI_CONNECTION_COMPLETE event is received multiple times
> for the same handle, the device is registered multiple times which leads
> to memory corruptions. Therefore, consequent events for a single
> connection are ignored.
> 
> The conn->state can hold different values so conn->handle is
> checked to detect whether a connection is already set up.
> 
> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=215497
> Signed-off-by: Soenke Huster <soenke.huster@eknoes.de>
> ---
> This fixes the referenced bug and several use-after-free issues I discovered.
> I tagged it as RFC, as I am not 100% sure if checking the existence of the
> handle is the correct approach, but to the best of my knowledge it must be
> set for the first time in this function for valid connections of this event,
> therefore it should be fine.
> 
> net/bluetooth/hci_event.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 681c623aa380..71ccb12c928d 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3106,6 +3106,17 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
>  		}
>  	}
>  
> +	/* The HCI_Connection_Complete event is only sent once per connection.
> +	 * Processing it more than once per connection can corrupt kernel memory.
> +	 *
> +	 * As the connection handle is set here for the first time, it indicates
> +	 * whether the connection is already set up.
> +	 */
> +	if (conn->handle) {
> +		bt_dev_err(hdev, "Ignoring HCI_Connection_Complete for existing connection");
> +		goto unlock;
> +	}
> +
>  	if (!ev->status) {
>  		conn->handle = __le16_to_cpu(ev->handle);
>  

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] Bluetooth: hci_event: Ignore multiple conn complete events
  2022-01-21 18:22 ` [RFC PATCH] " Sönke Huster
@ 2022-01-21 21:31   ` Luiz Augusto von Dentz
  2022-01-21 23:18     ` Sönke Huster
  0 siblings, 1 reply; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2022-01-21 21:31 UTC (permalink / raw)
  To: Sönke Huster
  Cc: Marcel Holtmann, Johan Hedberg, David S. Miller, Jakub Kicinski,
	linux-bluetooth, open list:NETWORKING [GENERAL],
	Linux Kernel Mailing List

Hi Sönke,

On Fri, Jan 21, 2022 at 10:22 AM Sönke Huster <soenke.huster@eknoes.de> wrote:
>
> I just noticed that just checking for handle does not work, as obviously 0x0 could also be a handle value and therefore it can't be distinguished, whether it is not set yet or it is 0x0.

Yep, we should probably check its state, check for state != BT_OPEN
since that is what hci_conn_add initialize the state.

> On 21.01.22 18:36, Soenke Huster wrote:
> > When a HCI_CONNECTION_COMPLETE event is received multiple times
> > for the same handle, the device is registered multiple times which leads
> > to memory corruptions. Therefore, consequent events for a single
> > connection are ignored.
> >
> > The conn->state can hold different values so conn->handle is
> > checked to detect whether a connection is already set up.
> >
> > Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=215497
> > Signed-off-by: Soenke Huster <soenke.huster@eknoes.de>
> > ---
> > This fixes the referenced bug and several use-after-free issues I discovered.
> > I tagged it as RFC, as I am not 100% sure if checking the existence of the
> > handle is the correct approach, but to the best of my knowledge it must be
> > set for the first time in this function for valid connections of this event,
> > therefore it should be fine.
> >
> > net/bluetooth/hci_event.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 681c623aa380..71ccb12c928d 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -3106,6 +3106,17 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
> >               }
> >       }
> >
> > +     /* The HCI_Connection_Complete event is only sent once per connection.
> > +      * Processing it more than once per connection can corrupt kernel memory.
> > +      *
> > +      * As the connection handle is set here for the first time, it indicates
> > +      * whether the connection is already set up.
> > +      */
> > +     if (conn->handle) {
> > +             bt_dev_err(hdev, "Ignoring HCI_Connection_Complete for existing connection");
> > +             goto unlock;
> > +     }
> > +
> >       if (!ev->status) {
> >               conn->handle = __le16_to_cpu(ev->handle);
> >



-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] Bluetooth: hci_event: Ignore multiple conn complete events
  2022-01-21 21:31   ` Luiz Augusto von Dentz
@ 2022-01-21 23:18     ` Sönke Huster
  2022-01-21 23:32       ` Luiz Augusto von Dentz
  0 siblings, 1 reply; 8+ messages in thread
From: Sönke Huster @ 2022-01-21 23:18 UTC (permalink / raw)
  To: Luiz Augusto von Dentz
  Cc: Marcel Holtmann, Johan Hedberg, David S. Miller, Jakub Kicinski,
	linux-bluetooth, open list:NETWORKING [GENERAL],
	Linux Kernel Mailing List

Hi Luiz,

On 21.01.22 22:31, Luiz Augusto von Dentz wrote:
> Hi Sönke,
> 
> On Fri, Jan 21, 2022 at 10:22 AM Sönke Huster <soenke.huster@eknoes.de> wrote:
>>
>> I just noticed that just checking for handle does not work, as obviously 0x0 could also be a handle value and therefore it can't be distinguished, whether it is not set yet or it is 0x0.
> 
> Yep, we should probably check its state, check for state != BT_OPEN
> since that is what hci_conn_add initialize the state.
> 

I thought there are more valid connection states for the first HCI_CONNECTION_COMPLETE event, as it also occurs e.g. after an HCI_Create_Connection command, see Core 5.3 p.2170:
> This event also indicates to the Host which issued the HCI_Create_Connection, HCI_Accept_-
> Connection_Request, or HCI_Reject_Connection_Request command, and
> then received an HCI_Command_Status event, if the issued command failed or
> was successful.

For example in hci_conn.c hci_acl_create_connection (which triggers a HCI_Create_Connection command as far as I understand), the state of the connection is changed to BT_CONNECT or BT_CONNECT2.
But as I am quite new in the (Linux) Bluetooth world, I might have a wrong understanding of that.

>> On 21.01.22 18:36, Soenke Huster wrote:
>>> When a HCI_CONNECTION_COMPLETE event is received multiple times
>>> for the same handle, the device is registered multiple times which leads
>>> to memory corruptions. Therefore, consequent events for a single
>>> connection are ignored.
>>>
>>> The conn->state can hold different values so conn->handle is
>>> checked to detect whether a connection is already set up.
>>>
>>> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=215497
>>> Signed-off-by: Soenke Huster <soenke.huster@eknoes.de>
>>> ---
>>> This fixes the referenced bug and several use-after-free issues I discovered.
>>> I tagged it as RFC, as I am not 100% sure if checking the existence of the
>>> handle is the correct approach, but to the best of my knowledge it must be
>>> set for the first time in this function for valid connections of this event,
>>> therefore it should be fine.
>>>
>>> net/bluetooth/hci_event.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>>> index 681c623aa380..71ccb12c928d 100644
>>> --- a/net/bluetooth/hci_event.c
>>> +++ b/net/bluetooth/hci_event.c
>>> @@ -3106,6 +3106,17 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
>>>               }
>>>       }
>>>
>>> +     /* The HCI_Connection_Complete event is only sent once per connection.
>>> +      * Processing it more than once per connection can corrupt kernel memory.
>>> +      *
>>> +      * As the connection handle is set here for the first time, it indicates
>>> +      * whether the connection is already set up.
>>> +      */
>>> +     if (conn->handle) {
>>> +             bt_dev_err(hdev, "Ignoring HCI_Connection_Complete for existing connection");
>>> +             goto unlock;
>>> +     }
>>> +
>>>       if (!ev->status) {
>>>               conn->handle = __le16_to_cpu(ev->handle);
>>>
> 
> 
> 

Best
Sönke

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] Bluetooth: hci_event: Ignore multiple conn complete events
  2022-01-21 23:18     ` Sönke Huster
@ 2022-01-21 23:32       ` Luiz Augusto von Dentz
  2022-01-21 23:52         ` Sönke Huster
  0 siblings, 1 reply; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2022-01-21 23:32 UTC (permalink / raw)
  To: Sönke Huster
  Cc: Marcel Holtmann, Johan Hedberg, David S. Miller, Jakub Kicinski,
	linux-bluetooth, open list:NETWORKING [GENERAL],
	Linux Kernel Mailing List

Hi Sönke,

On Fri, Jan 21, 2022 at 3:18 PM Sönke Huster <soenke.huster@eknoes.de> wrote:
>
> Hi Luiz,
>
> On 21.01.22 22:31, Luiz Augusto von Dentz wrote:
> > Hi Sönke,
> >
> > On Fri, Jan 21, 2022 at 10:22 AM Sönke Huster <soenke.huster@eknoes.de> wrote:
> >>
> >> I just noticed that just checking for handle does not work, as obviously 0x0 could also be a handle value and therefore it can't be distinguished, whether it is not set yet or it is 0x0.
> >
> > Yep, we should probably check its state, check for state != BT_OPEN
> > since that is what hci_conn_add initialize the state.
> >
>
> I thought there are more valid connection states for the first HCI_CONNECTION_COMPLETE event, as it also occurs e.g. after an HCI_Create_Connection command, see Core 5.3 p.2170:
> > This event also indicates to the Host which issued the HCI_Create_Connection, HCI_Accept_-
> > Connection_Request, or HCI_Reject_Connection_Request command, and
> > then received an HCI_Command_Status event, if the issued command failed or
> > was successful.
>
> For example in hci_conn.c hci_acl_create_connection (which triggers a HCI_Create_Connection command as far as I understand), the state of the connection is changed to BT_CONNECT or BT_CONNECT2.
> But as I am quite new in the (Linux) Bluetooth world, I might have a wrong understanding of that.

Yep, we would probably need a switch to capture which states are valid
and which are not or we initialize the handle with something outside
of the valid range of handles (0x0000 to 0x0EFF) so we can initialize
it to e.g. 0xffff (using something like define HCI_CONN_HANDLE_UNSET)
so we can really tell when it has been set or not.

> >> On 21.01.22 18:36, Soenke Huster wrote:
> >>> When a HCI_CONNECTION_COMPLETE event is received multiple times
> >>> for the same handle, the device is registered multiple times which leads
> >>> to memory corruptions. Therefore, consequent events for a single
> >>> connection are ignored.
> >>>
> >>> The conn->state can hold different values so conn->handle is
> >>> checked to detect whether a connection is already set up.
> >>>
> >>> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=215497
> >>> Signed-off-by: Soenke Huster <soenke.huster@eknoes.de>
> >>> ---
> >>> This fixes the referenced bug and several use-after-free issues I discovered.
> >>> I tagged it as RFC, as I am not 100% sure if checking the existence of the
> >>> handle is the correct approach, but to the best of my knowledge it must be
> >>> set for the first time in this function for valid connections of this event,
> >>> therefore it should be fine.
> >>>
> >>> net/bluetooth/hci_event.c | 11 +++++++++++
> >>>  1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> >>> index 681c623aa380..71ccb12c928d 100644
> >>> --- a/net/bluetooth/hci_event.c
> >>> +++ b/net/bluetooth/hci_event.c
> >>> @@ -3106,6 +3106,17 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
> >>>               }
> >>>       }
> >>>
> >>> +     /* The HCI_Connection_Complete event is only sent once per connection.
> >>> +      * Processing it more than once per connection can corrupt kernel memory.
> >>> +      *
> >>> +      * As the connection handle is set here for the first time, it indicates
> >>> +      * whether the connection is already set up.
> >>> +      */
> >>> +     if (conn->handle) {
> >>> +             bt_dev_err(hdev, "Ignoring HCI_Connection_Complete for existing connection");
> >>> +             goto unlock;
> >>> +     }
> >>> +
> >>>       if (!ev->status) {
> >>>               conn->handle = __le16_to_cpu(ev->handle);
> >>>
> >
> >
> >
>
> Best
> Sönke



-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] Bluetooth: hci_event: Ignore multiple conn complete events
  2022-01-21 23:32       ` Luiz Augusto von Dentz
@ 2022-01-21 23:52         ` Sönke Huster
  2022-01-22  0:06           ` Luiz Augusto von Dentz
  0 siblings, 1 reply; 8+ messages in thread
From: Sönke Huster @ 2022-01-21 23:52 UTC (permalink / raw)
  To: Luiz Augusto von Dentz
  Cc: Marcel Holtmann, Johan Hedberg, David S. Miller, Jakub Kicinski,
	linux-bluetooth, open list:NETWORKING [GENERAL],
	Linux Kernel Mailing List

Hi Luiz,

On 22.01.22 00:32, Luiz Augusto von Dentz wrote:
> Hi Sönke,
> 
> On Fri, Jan 21, 2022 at 3:18 PM Sönke Huster <soenke.huster@eknoes.de> wrote:
>>
>> Hi Luiz,
>>
>> On 21.01.22 22:31, Luiz Augusto von Dentz wrote:
>>> Hi Sönke,
>>>
>>> On Fri, Jan 21, 2022 at 10:22 AM Sönke Huster <soenke.huster@eknoes.de> wrote:
>>>>
>>>> I just noticed that just checking for handle does not work, as obviously 0x0 could also be a handle value and therefore it can't be distinguished, whether it is not set yet or it is 0x0.
>>>
>>> Yep, we should probably check its state, check for state != BT_OPEN
>>> since that is what hci_conn_add initialize the state.
>>>
>>
>> I thought there are more valid connection states for the first HCI_CONNECTION_COMPLETE event, as it also occurs e.g. after an HCI_Create_Connection command, see Core 5.3 p.2170:
>>> This event also indicates to the Host which issued the HCI_Create_Connection, HCI_Accept_-
>>> Connection_Request, or HCI_Reject_Connection_Request command, and
>>> then received an HCI_Command_Status event, if the issued command failed or
>>> was successful.
>>
>> For example in hci_conn.c hci_acl_create_connection (which triggers a HCI_Create_Connection command as far as I understand), the state of the connection is changed to BT_CONNECT or BT_CONNECT2.
>> But as I am quite new in the (Linux) Bluetooth world, I might have a wrong understanding of that.
> 
> Yep, we would probably need a switch to capture which states are valid
> and which are not or we initialize the handle with something outside
> of the valid range of handles (0x0000 to 0x0EFF) so we can initialize
> it to e.g. 0xffff (using something like define HCI_CONN_HANDLE_UNSET)
> so we can really tell when it has been set or not.
> 

I think the state switch is just possible if there is no possibility
to change a connection state back into one of the accepted states.
Unless changing the state back into an accepted state includes a call
to "hci_conn_del_sysfs", as the real issue when getting a duplicate
HCI_Create_Connection event is that device_add in hci_conn_add_sysfs
is called twice for the same connection.

There might be other issues as well in processing a duplicate event,
but as far as I can see the bugs I trigger rely on multiple calls to
device_add which lead in the long run to multiple user-after frees
or null-pointer derefs. I tried to write that up in the bugzilla report
here: https://bugzilla.kernel.org/show_bug.cgi?id=215497


When using something like HCI_CONN_HANDLE_UNSET, we need to make sure
that everywhere where we receive a handle from an event and use it to
set conn->handle, it is a valid one. Otherwise a hacked / malicious
controller would just send multiple events for the invalid handle.

What solution do you prefer? If you don't mind I'd like to try to
create a patch.  

>>>> On 21.01.22 18:36, Soenke Huster wrote:
>>>>> When a HCI_CONNECTION_COMPLETE event is received multiple times
>>>>> for the same handle, the device is registered multiple times which leads
>>>>> to memory corruptions. Therefore, consequent events for a single
>>>>> connection are ignored.
>>>>>
>>>>> The conn->state can hold different values so conn->handle is
>>>>> checked to detect whether a connection is already set up.
>>>>>
>>>>> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=215497
>>>>> Signed-off-by: Soenke Huster <soenke.huster@eknoes.de>
>>>>> ---
>>>>> This fixes the referenced bug and several use-after-free issues I discovered.
>>>>> I tagged it as RFC, as I am not 100% sure if checking the existence of the
>>>>> handle is the correct approach, but to the best of my knowledge it must be
>>>>> set for the first time in this function for valid connections of this event,
>>>>> therefore it should be fine.
>>>>>
>>>>> net/bluetooth/hci_event.c | 11 +++++++++++
>>>>>  1 file changed, 11 insertions(+)
>>>>>
>>>>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>>>>> index 681c623aa380..71ccb12c928d 100644
>>>>> --- a/net/bluetooth/hci_event.c
>>>>> +++ b/net/bluetooth/hci_event.c
>>>>> @@ -3106,6 +3106,17 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
>>>>>               }
>>>>>       }
>>>>>
>>>>> +     /* The HCI_Connection_Complete event is only sent once per connection.
>>>>> +      * Processing it more than once per connection can corrupt kernel memory.
>>>>> +      *
>>>>> +      * As the connection handle is set here for the first time, it indicates
>>>>> +      * whether the connection is already set up.
>>>>> +      */
>>>>> +     if (conn->handle) {
>>>>> +             bt_dev_err(hdev, "Ignoring HCI_Connection_Complete for existing connection");
>>>>> +             goto unlock;
>>>>> +     }
>>>>> +
>>>>>       if (!ev->status) {
>>>>>               conn->handle = __le16_to_cpu(ev->handle);
>>>>>
>>>
>>>
>>>
>>
>> Best
>> Sönke
> 
> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] Bluetooth: hci_event: Ignore multiple conn complete events
  2022-01-21 23:52         ` Sönke Huster
@ 2022-01-22  0:06           ` Luiz Augusto von Dentz
  0 siblings, 0 replies; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2022-01-22  0:06 UTC (permalink / raw)
  To: Sönke Huster
  Cc: Marcel Holtmann, Johan Hedberg, David S. Miller, Jakub Kicinski,
	linux-bluetooth, open list:NETWORKING [GENERAL],
	Linux Kernel Mailing List

Hi Sönke,

On Fri, Jan 21, 2022 at 3:52 PM Sönke Huster <soenke.huster@eknoes.de> wrote:
>
> Hi Luiz,
>
> On 22.01.22 00:32, Luiz Augusto von Dentz wrote:
> > Hi Sönke,
> >
> > On Fri, Jan 21, 2022 at 3:18 PM Sönke Huster <soenke.huster@eknoes.de> wrote:
> >>
> >> Hi Luiz,
> >>
> >> On 21.01.22 22:31, Luiz Augusto von Dentz wrote:
> >>> Hi Sönke,
> >>>
> >>> On Fri, Jan 21, 2022 at 10:22 AM Sönke Huster <soenke.huster@eknoes.de> wrote:
> >>>>
> >>>> I just noticed that just checking for handle does not work, as obviously 0x0 could also be a handle value and therefore it can't be distinguished, whether it is not set yet or it is 0x0.
> >>>
> >>> Yep, we should probably check its state, check for state != BT_OPEN
> >>> since that is what hci_conn_add initialize the state.
> >>>
> >>
> >> I thought there are more valid connection states for the first HCI_CONNECTION_COMPLETE event, as it also occurs e.g. after an HCI_Create_Connection command, see Core 5.3 p.2170:
> >>> This event also indicates to the Host which issued the HCI_Create_Connection, HCI_Accept_-
> >>> Connection_Request, or HCI_Reject_Connection_Request command, and
> >>> then received an HCI_Command_Status event, if the issued command failed or
> >>> was successful.
> >>
> >> For example in hci_conn.c hci_acl_create_connection (which triggers a HCI_Create_Connection command as far as I understand), the state of the connection is changed to BT_CONNECT or BT_CONNECT2.
> >> But as I am quite new in the (Linux) Bluetooth world, I might have a wrong understanding of that.
> >
> > Yep, we would probably need a switch to capture which states are valid
> > and which are not or we initialize the handle with something outside
> > of the valid range of handles (0x0000 to 0x0EFF) so we can initialize
> > it to e.g. 0xffff (using something like define HCI_CONN_HANDLE_UNSET)
> > so we can really tell when it has been set or not.
> >
>
> I think the state switch is just possible if there is no possibility
> to change a connection state back into one of the accepted states.
> Unless changing the state back into an accepted state includes a call
> to "hci_conn_del_sysfs", as the real issue when getting a duplicate
> HCI_Create_Connection event is that device_add in hci_conn_add_sysfs
> is called twice for the same connection.
>
> There might be other issues as well in processing a duplicate event,
> but as far as I can see the bugs I trigger rely on multiple calls to
> device_add which lead in the long run to multiple user-after frees
> or null-pointer derefs. I tried to write that up in the bugzilla report
> here: https://bugzilla.kernel.org/show_bug.cgi?id=215497
>
>
> When using something like HCI_CONN_HANDLE_UNSET, we need to make sure
> that everywhere where we receive a handle from an event and use it to
> set conn->handle, it is a valid one. Otherwise a hacked / malicious
> controller would just send multiple events for the invalid handle.

Well that is already the case for 0x0000, so one can in fact already
abuse such a thing, so yes we probably should check if the received
handle is within the valid range or not.

> What solution do you prefer? If you don't mind I'd like to try to
> create a patch.

I'd start by initializing the conn->handle to 0xffff and add checks
for it later we can add checks for received handle as well.

> >>>> On 21.01.22 18:36, Soenke Huster wrote:
> >>>>> When a HCI_CONNECTION_COMPLETE event is received multiple times
> >>>>> for the same handle, the device is registered multiple times which leads
> >>>>> to memory corruptions. Therefore, consequent events for a single
> >>>>> connection are ignored.
> >>>>>
> >>>>> The conn->state can hold different values so conn->handle is
> >>>>> checked to detect whether a connection is already set up.
> >>>>>
> >>>>> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=215497
> >>>>> Signed-off-by: Soenke Huster <soenke.huster@eknoes.de>
> >>>>> ---
> >>>>> This fixes the referenced bug and several use-after-free issues I discovered.
> >>>>> I tagged it as RFC, as I am not 100% sure if checking the existence of the
> >>>>> handle is the correct approach, but to the best of my knowledge it must be
> >>>>> set for the first time in this function for valid connections of this event,
> >>>>> therefore it should be fine.
> >>>>>
> >>>>> net/bluetooth/hci_event.c | 11 +++++++++++
> >>>>>  1 file changed, 11 insertions(+)
> >>>>>
> >>>>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> >>>>> index 681c623aa380..71ccb12c928d 100644
> >>>>> --- a/net/bluetooth/hci_event.c
> >>>>> +++ b/net/bluetooth/hci_event.c
> >>>>> @@ -3106,6 +3106,17 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
> >>>>>               }
> >>>>>       }
> >>>>>
> >>>>> +     /* The HCI_Connection_Complete event is only sent once per connection.
> >>>>> +      * Processing it more than once per connection can corrupt kernel memory.
> >>>>> +      *
> >>>>> +      * As the connection handle is set here for the first time, it indicates
> >>>>> +      * whether the connection is already set up.
> >>>>> +      */
> >>>>> +     if (conn->handle) {
> >>>>> +             bt_dev_err(hdev, "Ignoring HCI_Connection_Complete for existing connection");
> >>>>> +             goto unlock;
> >>>>> +     }
> >>>>> +
> >>>>>       if (!ev->status) {
> >>>>>               conn->handle = __le16_to_cpu(ev->handle);
> >>>>>
> >>>
> >>>
> >>>
> >>
> >> Best
> >> Sönke
> >
> >
> >



-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-01-22  0:07 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-21 17:36 [RFC PATCH] Bluetooth: hci_event: Ignore multiple conn complete events Soenke Huster
2022-01-21 18:04 ` [RFC] " bluez.test.bot
2022-01-21 18:22 ` [RFC PATCH] " Sönke Huster
2022-01-21 21:31   ` Luiz Augusto von Dentz
2022-01-21 23:18     ` Sönke Huster
2022-01-21 23:32       ` Luiz Augusto von Dentz
2022-01-21 23:52         ` Sönke Huster
2022-01-22  0:06           ` Luiz Augusto von Dentz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).