diff mbox series

[v2,4/5] Bluetooth: hci_conn: Fix modifying handle while aborting

Message ID 20230804001115.907885-4-luiz.dentz@gmail.com
State New
Headers show
Series [v2,1/5] Bluetooth: hci_sync: Fix handling of HCI_OP_CREATE_CONN_CANCEL | expand

Commit Message

Luiz Augusto von Dentz Aug. 4, 2023, 12:11 a.m. UTC 
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This introduces hci_conn_set_handle which takes care of verifying the
conditions where the hci_conn handle can be modified, including when
hci_conn_abort has been called and also checks that the handles is
valid as well.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/hci_conn.c         | 30 ++++++++++++++++++++++++++++++
 net/bluetooth/hci_event.c        | 29 +++++++++++------------------
 3 files changed, 42 insertions(+), 18 deletions(-)

Comments

Pauli Virtanen Aug. 4, 2023, 4:28 p.m. UTC | #1 
Hi Luiz,

to, 2023-08-03 kello 17:11 -0700, Luiz Augusto von Dentz kirjoitti:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> This introduces hci_conn_set_handle which takes care of verifying the
> conditions where the hci_conn handle can be modified, including when
> hci_conn_abort has been called and also checks that the handles is
> valid as well.

It looks there could still be problem vs. sequences of the type

[kernel] hci_acl_create_connection(AA:AA:AA:AA:AA:AA)
[controller] < Create Connection AA:AA:AA:AA:AA:AA
[controller] > Connection Complete handle X
[kernel] hci_conn_complete_evt(handle X)
[kernel] hci_acl_create_connection(BB:BB:BB:BB:BB:BB)
[kernel] hci_abort_conn(handle X)
[controller] > Disconnect Complete handle X (from remote)
[kernel] hci_disconn_complete_evt(handle X)
[controller] < Create Connection BB:BB:BB:BB:BB:BB
[controller] > Connection Complete handle X  (same handle value)
[kernel] hci_conn_complete_evt(handle X)
...
[kernel] hci_abort_conn_sync(handle X)

This would seem to terminate the wrong connection.

Some flag/abort_reason could be checked to see if the looked up conn is
to be aborted before doing it. This can also be used to make
hci_disconnect_all_sync iteration UAF-safe.

It's unclear to me if you agree that tasks from hdev->workqueue and
hdev->req_workqueue can run concurrently, so that locking is needed.

> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
>  include/net/bluetooth/hci_core.h |  1 +
>  net/bluetooth/hci_conn.c         | 30 ++++++++++++++++++++++++++++++
>  net/bluetooth/hci_event.c        | 29 +++++++++++------------------
>  3 files changed, 42 insertions(+), 18 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 8200a6689b39..d2a3a2a9fd7d 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1425,6 +1425,7 @@ int hci_conn_switch_role(struct hci_conn *conn, __u8 role);
>  void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
>  
>  void hci_conn_failed(struct hci_conn *conn, u8 status);
> +u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle);
>  
>  /*
>   * hci_conn_get() and hci_conn_put() are used to control the life-time of an
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 923bb7e7be2b..13bd2753abbb 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -1231,6 +1231,36 @@ void hci_conn_failed(struct hci_conn *conn, u8 status)
>  	hci_conn_del(conn);
>  }
>  
> +/* This function requires the caller holds hdev->lock */
> +u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle)
> +{
> +	struct hci_dev *hdev = conn->hdev;
> +
> +	bt_dev_dbg(hdev, "hcon %p handle 0x%4.4x", conn, handle);
> +
> +	if (conn->handle == handle)
> +		return 0;
> +
> +	if (handle > HCI_CONN_HANDLE_MAX) {
> +		bt_dev_err(hdev, "Invalid handle: 0x%4.4x > 0x%4.4x",
> +			   handle, HCI_CONN_HANDLE_MAX);
> +		return HCI_ERROR_INVALID_PARAMETERS;
> +	}
> +
> +	/* If abort_reason has been sent it means the connection is being
> +	 * aborted and the handle shall not be changed.
> +	 */
> +	if (conn->abort_reason) {
> +		bt_dev_err(hdev, "hcon %p abort_reason 0x%2.2x", conn,
> +			   conn->abort_reason);
> +		return conn->abort_reason;
> +	}
> +
> +	conn->handle = handle;
> +
> +	return 0;
> +}
> +
>  static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
>  {
>  	struct hci_conn *conn;
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index f1fcece29e7d..218da9b0fe8f 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3179,13 +3179,9 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
>  	}
>  
>  	if (!status) {
> -		conn->handle = __le16_to_cpu(ev->handle);
> -		if (conn->handle > HCI_CONN_HANDLE_MAX) {
> -			bt_dev_err(hdev, "Invalid handle: 0x%4.4x > 0x%4.4x",
> -				   conn->handle, HCI_CONN_HANDLE_MAX);
> -			status = HCI_ERROR_INVALID_PARAMETERS;
> +		status = hci_conn_set_handle(conn, __le16_to_cpu(ev->handle));
> +		if (status)
>  			goto done;
> -		}
>  
>  		if (conn->type == ACL_LINK) {
>  			conn->state = BT_CONFIG;
> @@ -3849,11 +3845,9 @@ static u8 hci_cc_le_set_cig_params(struct hci_dev *hdev, void *data,
>  		if (conn->state != BT_BOUND && conn->state != BT_CONNECT)
>  			continue;
>  
> -		conn->handle = __le16_to_cpu(rp->handle[i]);
> +		if (hci_conn_set_handle(conn, __le16_to_cpu(rp->handle[i])))
> +			continue;
>  
> -		bt_dev_dbg(hdev, "%p handle 0x%4.4x parent %p", conn,
> -			   conn->handle, conn->parent);
> -		
>  		if (conn->state == BT_CONNECT)
>  			pending = true;
>  	}
> @@ -5039,11 +5033,8 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev, void *data,
>  
>  	switch (status) {
>  	case 0x00:
> -		conn->handle = __le16_to_cpu(ev->handle);
> -		if (conn->handle > HCI_CONN_HANDLE_MAX) {
> -			bt_dev_err(hdev, "Invalid handle: 0x%4.4x > 0x%4.4x",
> -				   conn->handle, HCI_CONN_HANDLE_MAX);
> -			status = HCI_ERROR_INVALID_PARAMETERS;
> +		status = hci_conn_set_handle(conn, __le16_to_cpu(ev->handle));
> +		if (status) {
>  			conn->state = BT_CLOSED;
>  			break;
>  		}
> @@ -6978,7 +6969,7 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
>  {
>  	struct hci_evt_le_create_big_complete *ev = data;
>  	struct hci_conn *conn;
> -	__u8 bis_idx = 0;
> +	__u8 i = 0;
>  
>  	BT_DBG("%s status 0x%2.2x", hdev->name, ev->status);
>  
> @@ -6996,7 +6987,9 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
>  		    conn->iso_qos.bcast.big != ev->handle)
>  			continue;
>  
> -		conn->handle = __le16_to_cpu(ev->bis_handle[bis_idx++]);
> +		if (hci_conn_set_handle(conn,
> +					__le16_to_cpu(ev->bis_handle[i++])))
> +			continue;
>  
>  		if (!ev->status) {
>  			conn->state = BT_CONNECTED;
> @@ -7015,7 +7008,7 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
>  		rcu_read_lock();
>  	}
>  
> -	if (!ev->status && !bis_idx)
> +	if (!ev->status && !i)
>  		/* If no BISes have been connected for the BIG,
>  		 * terminate. This is in case all bound connections
>  		 * have been closed before the BIG creation
Luiz Augusto von Dentz Aug. 5, 2023, 12:30 a.m. UTC | #2 
Hi Pauli,

On Fri, Aug 4, 2023 at 9:28 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi Luiz,
>
> to, 2023-08-03 kello 17:11 -0700, Luiz Augusto von Dentz kirjoitti:
> > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >
> > This introduces hci_conn_set_handle which takes care of verifying the
> > conditions where the hci_conn handle can be modified, including when
> > hci_conn_abort has been called and also checks that the handles is
> > valid as well.
>
> It looks there could still be problem vs. sequences of the type
>
> [kernel] hci_acl_create_connection(AA:AA:AA:AA:AA:AA)
> [controller] < Create Connection AA:AA:AA:AA:AA:AA
> [controller] > Connection Complete handle X
> [kernel] hci_conn_complete_evt(handle X)
> [kernel] hci_acl_create_connection(BB:BB:BB:BB:BB:BB)
> [kernel] hci_abort_conn(handle X)
> [controller] > Disconnect Complete handle X (from remote)
> [kernel] hci_disconn_complete_evt(handle X)
> [controller] < Create Connection BB:BB:BB:BB:BB:BB
> [controller] > Connection Complete handle X  (same handle value)
> [kernel] hci_conn_complete_evt(handle X)
> ...
> [kernel] hci_abort_conn_sync(handle X)
>
> This would seem to terminate the wrong connection.

Perhaps we need a test for it, it shall also be possible to cancel the callback.

> Some flag/abort_reason could be checked to see if the looked up conn is
> to be aborted before doing it. This can also be used to make
> hci_disconnect_all_sync iteration UAF-safe.

Yep, I am also thinking of introducing some functions to lookup on the
cmd_sync_queue to avoid submitting duplicated commands, or for example
if we want to cancel a callback.

> It's unclear to me if you agree that tasks from hdev->workqueue and
> hdev->req_workqueue can run concurrently, so that locking is needed.

I think what we should do is to check if we can have a more clear
separation, because the workqueue is used for example by the likes of
rx_work which does process everything we receive from the controller,
which perhaps is not the best idea given in case of HCI events it
might be better to process them in the same context of commands for
example, to ensure that HCI states is not being modified concurrently
which seems to be what causes the most problems.

> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> >  include/net/bluetooth/hci_core.h |  1 +
> >  net/bluetooth/hci_conn.c         | 30 ++++++++++++++++++++++++++++++
> >  net/bluetooth/hci_event.c        | 29 +++++++++++------------------
> >  3 files changed, 42 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index 8200a6689b39..d2a3a2a9fd7d 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -1425,6 +1425,7 @@ int hci_conn_switch_role(struct hci_conn *conn, __u8 role);
> >  void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
> >
> >  void hci_conn_failed(struct hci_conn *conn, u8 status);
> > +u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle);
> >
> >  /*
> >   * hci_conn_get() and hci_conn_put() are used to control the life-time of an
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index 923bb7e7be2b..13bd2753abbb 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -1231,6 +1231,36 @@ void hci_conn_failed(struct hci_conn *conn, u8 status)
> >       hci_conn_del(conn);
> >  }
> >
> > +/* This function requires the caller holds hdev->lock */
> > +u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle)
> > +{
> > +     struct hci_dev *hdev = conn->hdev;
> > +
> > +     bt_dev_dbg(hdev, "hcon %p handle 0x%4.4x", conn, handle);
> > +
> > +     if (conn->handle == handle)
> > +             return 0;
> > +
> > +     if (handle > HCI_CONN_HANDLE_MAX) {
> > +             bt_dev_err(hdev, "Invalid handle: 0x%4.4x > 0x%4.4x",
> > +                        handle, HCI_CONN_HANDLE_MAX);
> > +             return HCI_ERROR_INVALID_PARAMETERS;
> > +     }
> > +
> > +     /* If abort_reason has been sent it means the connection is being
> > +      * aborted and the handle shall not be changed.
> > +      */
> > +     if (conn->abort_reason) {
> > +             bt_dev_err(hdev, "hcon %p abort_reason 0x%2.2x", conn,
> > +                        conn->abort_reason);
> > +             return conn->abort_reason;
> > +     }
> > +
> > +     conn->handle = handle;
> > +
> > +     return 0;
> > +}
> > +
> >  static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
> >  {
> >       struct hci_conn *conn;
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index f1fcece29e7d..218da9b0fe8f 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -3179,13 +3179,9 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
> >       }
> >
> >       if (!status) {
> > -             conn->handle = __le16_to_cpu(ev->handle);
> > -             if (conn->handle > HCI_CONN_HANDLE_MAX) {
> > -                     bt_dev_err(hdev, "Invalid handle: 0x%4.4x > 0x%4.4x",
> > -                                conn->handle, HCI_CONN_HANDLE_MAX);
> > -                     status = HCI_ERROR_INVALID_PARAMETERS;
> > +             status = hci_conn_set_handle(conn, __le16_to_cpu(ev->handle));
> > +             if (status)
> >                       goto done;
> > -             }
> >
> >               if (conn->type == ACL_LINK) {
> >                       conn->state = BT_CONFIG;
> > @@ -3849,11 +3845,9 @@ static u8 hci_cc_le_set_cig_params(struct hci_dev *hdev, void *data,
> >               if (conn->state != BT_BOUND && conn->state != BT_CONNECT)
> >                       continue;
> >
> > -             conn->handle = __le16_to_cpu(rp->handle[i]);
> > +             if (hci_conn_set_handle(conn, __le16_to_cpu(rp->handle[i])))
> > +                     continue;
> >
> > -             bt_dev_dbg(hdev, "%p handle 0x%4.4x parent %p", conn,
> > -                        conn->handle, conn->parent);
> > -
> >               if (conn->state == BT_CONNECT)
> >                       pending = true;
> >       }
> > @@ -5039,11 +5033,8 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev, void *data,
> >
> >       switch (status) {
> >       case 0x00:
> > -             conn->handle = __le16_to_cpu(ev->handle);
> > -             if (conn->handle > HCI_CONN_HANDLE_MAX) {
> > -                     bt_dev_err(hdev, "Invalid handle: 0x%4.4x > 0x%4.4x",
> > -                                conn->handle, HCI_CONN_HANDLE_MAX);
> > -                     status = HCI_ERROR_INVALID_PARAMETERS;
> > +             status = hci_conn_set_handle(conn, __le16_to_cpu(ev->handle));
> > +             if (status) {
> >                       conn->state = BT_CLOSED;
> >                       break;
> >               }
> > @@ -6978,7 +6969,7 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
> >  {
> >       struct hci_evt_le_create_big_complete *ev = data;
> >       struct hci_conn *conn;
> > -     __u8 bis_idx = 0;
> > +     __u8 i = 0;
> >
> >       BT_DBG("%s status 0x%2.2x", hdev->name, ev->status);
> >
> > @@ -6996,7 +6987,9 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
> >                   conn->iso_qos.bcast.big != ev->handle)
> >                       continue;
> >
> > -             conn->handle = __le16_to_cpu(ev->bis_handle[bis_idx++]);
> > +             if (hci_conn_set_handle(conn,
> > +                                     __le16_to_cpu(ev->bis_handle[i++])))
> > +                     continue;
> >
> >               if (!ev->status) {
> >                       conn->state = BT_CONNECTED;
> > @@ -7015,7 +7008,7 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
> >               rcu_read_lock();
> >       }
> >
> > -     if (!ev->status && !bis_idx)
> > +     if (!ev->status && !i)
> >               /* If no BISes have been connected for the BIG,
> >                * terminate. This is in case all bound connections
> >                * have been closed before the BIG creation
>
diff mbox series

Patch

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 8200a6689b39..d2a3a2a9fd7d 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1425,6 +1425,7 @@  int hci_conn_switch_role(struct hci_conn *conn, __u8 role);
 void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
 
 void hci_conn_failed(struct hci_conn *conn, u8 status);
+u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle);
 
 /*
  * hci_conn_get() and hci_conn_put() are used to control the life-time of an
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 923bb7e7be2b..13bd2753abbb 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1231,6 +1231,36 @@  void hci_conn_failed(struct hci_conn *conn, u8 status)
 	hci_conn_del(conn);
 }
 
+/* This function requires the caller holds hdev->lock */
+u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle)
+{
+	struct hci_dev *hdev = conn->hdev;
+
+	bt_dev_dbg(hdev, "hcon %p handle 0x%4.4x", conn, handle);
+
+	if (conn->handle == handle)
+		return 0;
+
+	if (handle > HCI_CONN_HANDLE_MAX) {
+		bt_dev_err(hdev, "Invalid handle: 0x%4.4x > 0x%4.4x",
+			   handle, HCI_CONN_HANDLE_MAX);
+		return HCI_ERROR_INVALID_PARAMETERS;
+	}
+
+	/* If abort_reason has been sent it means the connection is being
+	 * aborted and the handle shall not be changed.
+	 */
+	if (conn->abort_reason) {
+		bt_dev_err(hdev, "hcon %p abort_reason 0x%2.2x", conn,
+			   conn->abort_reason);
+		return conn->abort_reason;
+	}
+
+	conn->handle = handle;
+
+	return 0;
+}
+
 static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
 {
 	struct hci_conn *conn;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index f1fcece29e7d..218da9b0fe8f 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3179,13 +3179,9 @@  static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
 	}
 
 	if (!status) {
-		conn->handle = __le16_to_cpu(ev->handle);
-		if (conn->handle > HCI_CONN_HANDLE_MAX) {
-			bt_dev_err(hdev, "Invalid handle: 0x%4.4x > 0x%4.4x",
-				   conn->handle, HCI_CONN_HANDLE_MAX);
-			status = HCI_ERROR_INVALID_PARAMETERS;
+		status = hci_conn_set_handle(conn, __le16_to_cpu(ev->handle));
+		if (status)
 			goto done;
-		}
 
 		if (conn->type == ACL_LINK) {
 			conn->state = BT_CONFIG;
@@ -3849,11 +3845,9 @@  static u8 hci_cc_le_set_cig_params(struct hci_dev *hdev, void *data,
 		if (conn->state != BT_BOUND && conn->state != BT_CONNECT)
 			continue;
 
-		conn->handle = __le16_to_cpu(rp->handle[i]);
+		if (hci_conn_set_handle(conn, __le16_to_cpu(rp->handle[i])))
+			continue;
 
-		bt_dev_dbg(hdev, "%p handle 0x%4.4x parent %p", conn,
-			   conn->handle, conn->parent);
-		
 		if (conn->state == BT_CONNECT)
 			pending = true;
 	}
@@ -5039,11 +5033,8 @@  static void hci_sync_conn_complete_evt(struct hci_dev *hdev, void *data,
 
 	switch (status) {
 	case 0x00:
-		conn->handle = __le16_to_cpu(ev->handle);
-		if (conn->handle > HCI_CONN_HANDLE_MAX) {
-			bt_dev_err(hdev, "Invalid handle: 0x%4.4x > 0x%4.4x",
-				   conn->handle, HCI_CONN_HANDLE_MAX);
-			status = HCI_ERROR_INVALID_PARAMETERS;
+		status = hci_conn_set_handle(conn, __le16_to_cpu(ev->handle));
+		if (status) {
 			conn->state = BT_CLOSED;
 			break;
 		}
@@ -6978,7 +6969,7 @@  static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
 {
 	struct hci_evt_le_create_big_complete *ev = data;
 	struct hci_conn *conn;
-	__u8 bis_idx = 0;
+	__u8 i = 0;
 
 	BT_DBG("%s status 0x%2.2x", hdev->name, ev->status);
 
@@ -6996,7 +6987,9 @@  static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
 		    conn->iso_qos.bcast.big != ev->handle)
 			continue;
 
-		conn->handle = __le16_to_cpu(ev->bis_handle[bis_idx++]);
+		if (hci_conn_set_handle(conn,
+					__le16_to_cpu(ev->bis_handle[i++])))
+			continue;
 
 		if (!ev->status) {
 			conn->state = BT_CONNECTED;
@@ -7015,7 +7008,7 @@  static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
 		rcu_read_lock();
 	}
 
-	if (!ev->status && !bis_idx)
+	if (!ev->status && !i)
 		/* If no BISes have been connected for the BIG,
 		 * terminate. This is in case all bound connections
 		 * have been closed before the BIG creation