[PATCH v2 0/4] Bluetooth: Fix initiator/responder addresses for SMP

[PATCH v2 0/4] Bluetooth: Fix initiator/responder addresses for SMP

[johan.hedberg]

Hi,

Here's a second attempt to fix the initiator/responder addresses when
using privacy. The main difference to the previous attempt is an added
protection to HCI_Set_Random_Address so we don't do that while
connecting or advertising, and a clarification for the white list corner
case in aptch 3/4 that we're taking a "best effort" approach while
lacking full tracking of white list initiated connections.

Johan

----------------------------------------------------------------
Johan Hedberg (4):
      Bluetooth: Add protections for updating local random address
      Bluetooth: Fix updating connection state to BT_CONNECT too early
      Bluetooth: Track LE initiator and responder address information
      Bluetooth: Use hdev->init/resp_addr values for smp_c1 function

 include/net/bluetooth/hci_core.h |  4 ++
 net/bluetooth/hci_conn.c         |  3 +-
 net/bluetooth/hci_core.c         | 27 ++++++++++++-
 net/bluetooth/hci_event.c        | 78 ++++++++++++++++++++++++++++++++++++
 net/bluetooth/mgmt.c             |  7 ++++
 net/bluetooth/smp.c              | 22 +++-------
 6 files changed, 122 insertions(+), 19 deletions(-)

[PATCH v2 1/4] Bluetooth: Add protections for updating local random address

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

Different controllers behave differently when HCI_Set_Random_Address is
called while they are advertising or have a HCI_LE_Create_Connection in
progress. Some take the newly written address into use for the pending
operation while others use the random address that we had at the time
that the operation started.

Due to this undefined behavior and for the fact that we want to reliably
determine the initiator address of all connections for the sake of SMP
it's best to simply prevent the random address update if we have these
problematic operations in progress.

This patch adds a set_random_addr() helper function for the use of
hci_update_random_address which contains the necessary checks for
advertising and ongoing LE connections.

One extra thing we need to do is to clear the HCI_ADVERTISING flag in
the enable_advertising() function before sending any commands. Since
re-enabling advertising happens by calling first disable_advertising()
and then enable_advertising() all while having the HCI_ADVERTISING flag
set. Clearing the flag lets the set_random_addr() function know that
it's safe to write a new address at least as far as advertising is
concerned.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_core.c | 27 +++++++++++++++++++++++++--
 net/bluetooth/mgmt.c     |  7 +++++++
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 32c0c2c58f66..8bbfdea9cbec 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3649,6 +3649,29 @@ static void le_scan_disable_work(struct work_struct *work)
 		BT_ERR("Disable LE scanning request failed: err %d", err);
 }
 
+static void set_random_addr(struct hci_request *req, bdaddr_t *rpa)
+{
+	struct hci_dev *hdev = req->hdev;
+
+	/* If we're advertising or initiating an LE connection we can't
+	 * go ahead and change the random address at this time. This is
+	 * because the eventual initiator address used for the
+	 * subsequently created connection will be undefined (some
+	 * controllers use the new address and others the one we had
+	 * when the operation started).
+	 *
+	 * In this kind of scenario skip the update and let the random
+	 * address be updated at the next cycle.
+	 */
+	if (test_bit(HCI_ADVERTISING, &hdev->dev_flags) ||
+	    hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT)) {
+		BT_DBG("Deferring random address update");
+		return;
+	}
+
+	hci_req_add(req, HCI_OP_LE_SET_RANDOM_ADDR, 6, rpa);
+}
+
 int hci_update_random_address(struct hci_request *req, bool require_privacy,
 			      u8 *own_addr_type)
 {
@@ -3674,7 +3697,7 @@ int hci_update_random_address(struct hci_request *req, bool require_privacy,
 			return err;
 		}
 
-		hci_req_add(req, HCI_OP_LE_SET_RANDOM_ADDR, 6, &hdev->rpa);
+		set_random_addr(req, &hdev->rpa);
 
 		to = msecs_to_jiffies(hdev->rpa_timeout * 1000);
 		queue_delayed_work(hdev->workqueue, &hdev->rpa_expired, to);
@@ -3693,7 +3716,7 @@ int hci_update_random_address(struct hci_request *req, bool require_privacy,
 		urpa.b[5] &= 0x3f;	/* Clear two most significant bits */
 
 		*own_addr_type = ADDR_LE_DEV_RANDOM;
-		hci_req_add(req, HCI_OP_LE_SET_RANDOM_ADDR, 6, &urpa);
+		set_random_addr(req, &urpa);
 		return 0;
 	}
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 2d11c817d082..98e9df3556e7 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -840,6 +840,13 @@ static void enable_advertising(struct hci_request *req)
 	u8 own_addr_type, enable = 0x01;
 	bool connectable;
 
+	/* Clear the HCI_ADVERTISING bit temporarily so that the
+	 * hci_update_random_address knows that it's safe to go ahead
+	 * and write a new random address. The flag will be set back on
+	 * as soon as the SET_ADV_ENABLE HCI command completes.
+	 */
+	clear_bit(HCI_ADVERTISING, &hdev->dev_flags);
+
 	connectable = get_connectable(hdev);
 
 	/* Set require_privacy to true only when non-connectable
-- 
1.8.5.3

[PATCH v2 2/4] Bluetooth: Fix updating connection state to BT_CONNECT too early

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

We shouldn't update the hci_conn state to BT_CONNECT until the moment
that we're ready to send the initiating HCI command for it. If the
connection has the BT_CONNECT state too early the code responsible for
updating the local random address may incorrectly think there's a
pending connection in progress and refuse to update the address.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_conn.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 5b0802994cbb..818330c1b2a2 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -588,6 +588,8 @@ static void hci_req_add_le_create_conn(struct hci_request *req,
 	cp.max_ce_len = __constant_cpu_to_le16(0x0000);
 
 	hci_req_add(req, HCI_OP_LE_CREATE_CONN, sizeof(cp), &cp);
+
+	conn->state = BT_CONNECT;
 }
 
 static void stop_scan_complete(struct hci_dev *hdev, u8 status)
@@ -689,7 +691,6 @@ struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
 
 	conn->dst_type = dst_type;
 
-	conn->state = BT_CONNECT;
 	conn->out = true;
 	conn->link_mode |= HCI_LM_MASTER;
 	conn->sec_level = BT_SECURITY_LOW;
-- 
1.8.5.3

[PATCH v2 3/4] Bluetooth: Track LE initiator and responder address information

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

For SMP we need the local and remote addresses (and their types) that
were used to establish the connection. These may be different from the
Identity Addresses or even the current RPA. To guarantee that we have
this information available and it is correct track these values
separately from the very beginning of the connection.

For outgoing connections we set the values as soon as we get a
successful command status for HCI_LE_Create_Connection (for which the
patch adds a command status handler function) and for incoming
connections as soon as we get a LE Connection Complete HCI event.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci_core.h |  4 +++
 net/bluetooth/hci_event.c        | 78 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 0c63a7e12d90..edf194679b7d 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -332,6 +332,10 @@ struct hci_conn {
 	__u8		dst_type;
 	bdaddr_t	src;
 	__u8		src_type;
+	bdaddr_t	init_addr;
+	__u8		init_addr_type;
+	bdaddr_t	resp_addr;
+	__u8		resp_addr_type;
 	__u16		handle;
 	__u16		state;
 	__u8		mode;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index e3d7151e808e..3ae8ae1a029c 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1641,6 +1641,47 @@ static void hci_cs_accept_phylink(struct hci_dev *hdev, u8 status)
 	amp_write_remote_assoc(hdev, cp->phy_handle);
 }
 
+static void hci_cs_le_create_conn(struct hci_dev *hdev, u8 status)
+{
+	struct hci_cp_le_create_conn *cp;
+	struct hci_conn *conn;
+
+	BT_DBG("%s status 0x%2.2x", hdev->name, status);
+
+	/* All connection failure handling is taken care of by the
+	 * hci_le_conn_failed function which is triggered by the HCI
+	 * request completion callbacks used for connecting.
+	 */
+	if (status)
+		return;
+
+	cp = hci_sent_cmd_data(hdev, HCI_OP_LE_CREATE_CONN);
+	if (!cp)
+		return;
+
+	hci_dev_lock(hdev);
+
+	conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &cp->peer_addr);
+	if (!conn)
+		goto unlock;
+
+	/* Store the initiator and responder address information which
+	 * is needed for SMP. These values will not change during the
+	 * lifetime of the connection.
+	 */
+	conn->init_addr_type = cp->own_address_type;
+	if (cp->own_address_type == ADDR_LE_DEV_RANDOM)
+		bacpy(&conn->init_addr, &hdev->random_addr);
+	else
+		bacpy(&conn->init_addr, &hdev->bdaddr);
+
+	conn->resp_addr_type = cp->peer_addr_type;
+	bacpy(&conn->resp_addr, &cp->peer_addr);
+
+unlock:
+	hci_dev_unlock(hdev);
+}
+
 static void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	__u8 status = *((__u8 *) skb->data);
@@ -2532,6 +2573,10 @@ static void hci_cmd_status_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_cs_accept_phylink(hdev, ev->status);
 		break;
 
+	case HCI_OP_LE_CREATE_CONN:
+		hci_cs_le_create_conn(hdev, ev->status);
+		break;
+
 	default:
 		BT_DBG("%s opcode 0x%4.4x", hdev->name, opcode);
 		break;
@@ -3716,6 +3761,39 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 			conn->out = true;
 			conn->link_mode |= HCI_LM_MASTER;
 		}
+
+		/* If we didn't have a hci_conn object previously
+		 * but we're in master role this must be something
+		 * initiated using a white list. Since white list based
+		 * connections are not "first class citizens" we don't
+		 * have full tracking of them. Therefore, we go ahead
+		 * with a "best effort" approach of determining the
+		 * initiator address based on the HCI_PRIVACY flag.
+		 */
+		if (conn->out) {
+			conn->resp_addr_type = ev->bdaddr_type;
+			bacpy(&conn->resp_addr, &ev->bdaddr);
+			if (test_bit(HCI_PRIVACY, &hdev->dev_flags)) {
+				conn->init_addr_type = ADDR_LE_DEV_RANDOM;
+				bacpy(&conn->init_addr, &hdev->rpa);
+			} else {
+				hci_copy_identity_address(hdev,
+							  &conn->init_addr,
+							  &conn->init_addr_type);
+			}
+		} else {
+			/* Set the responder (our side) address type based on
+			 * the advertising address type.
+			 */
+			conn->resp_addr_type = hdev->adv_addr_type;
+			if (hdev->adv_addr_type == ADDR_LE_DEV_RANDOM)
+				bacpy(&conn->resp_addr, &hdev->random_addr);
+			else
+				bacpy(&conn->resp_addr, &hdev->bdaddr);
+
+			conn->init_addr_type = ev->bdaddr_type;
+			bacpy(&conn->init_addr, &ev->bdaddr);
+		}
 	}
 
 	/* Ensure that the hci_conn contains the identity address type
-- 
1.8.5.3

[PATCH v2 4/4] Bluetooth: Use hdev->init/resp_addr values for smp_c1 function

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

Now that we have nicely tracked values of the initiator and responder
address information we can pass that directly to the smp_c1 function
without worrying e.g. about who initiated the connection. This patch
updates the two places in smp.c to use the new variables.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/smp.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index f1cb6a32e93f..4f4ff36f5f34 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -445,14 +445,9 @@ static void confirm_work(struct work_struct *work)
 	/* Prevent mutual access to hdev->tfm_aes */
 	hci_dev_lock(hdev);
 
-	if (conn->hcon->out)
-		ret = smp_c1(tfm, smp->tk, smp->prnd, smp->preq, smp->prsp,
-			     conn->hcon->src_type, &conn->hcon->src,
-			     conn->hcon->dst_type, &conn->hcon->dst, res);
-	else
-		ret = smp_c1(tfm, smp->tk, smp->prnd, smp->preq, smp->prsp,
-			     conn->hcon->dst_type, &conn->hcon->dst,
-			     conn->hcon->src_type, &conn->hcon->src, res);
+	ret = smp_c1(tfm, smp->tk, smp->prnd, smp->preq, smp->prsp,
+		     conn->hcon->init_addr_type, &conn->hcon->init_addr,
+		     conn->hcon->resp_addr_type, &conn->hcon->resp_addr, res);
 
 	hci_dev_unlock(hdev);
 
@@ -492,14 +487,9 @@ static void random_work(struct work_struct *work)
 	/* Prevent mutual access to hdev->tfm_aes */
 	hci_dev_lock(hdev);
 
-	if (hcon->out)
-		ret = smp_c1(tfm, smp->tk, smp->rrnd, smp->preq, smp->prsp,
-			     hcon->src_type, &hcon->src,
-			     hcon->dst_type, &hcon->dst, res);
-	else
-		ret = smp_c1(tfm, smp->tk, smp->rrnd, smp->preq, smp->prsp,
-			     hcon->dst_type, &hcon->dst,
-			     hcon->src_type, &hcon->src, res);
+	ret = smp_c1(tfm, smp->tk, smp->rrnd, smp->preq, smp->prsp,
+		     hcon->init_addr_type, &hcon->init_addr,
+		     hcon->resp_addr_type, &hcon->resp_addr, res);
 
 	hci_dev_unlock(hdev);
 
-- 
1.8.5.3

Re: [PATCH v2 0/4] Bluetooth: Fix initiator/responder addresses for SMP

[Marcel Holtmann]

Hi Johan,

> Here's a second attempt to fix the initiator/responder addresses when
> using privacy. The main difference to the previous attempt is an added
> protection to HCI_Set_Random_Address so we don't do that while
> connecting or advertising, and a clarification for the white list corner
> case in aptch 3/4 that we're taking a "best effort" approach while
> lacking full tracking of white list initiated connections.
> 
> Johan
> 
> ----------------------------------------------------------------
> Johan Hedberg (4):
>      Bluetooth: Add protections for updating local random address
>      Bluetooth: Fix updating connection state to BT_CONNECT too early
>      Bluetooth: Track LE initiator and responder address information
>      Bluetooth: Use hdev->init/resp_addr values for smp_c1 function
> 
> include/net/bluetooth/hci_core.h |  4 ++
> net/bluetooth/hci_conn.c         |  3 +-
> net/bluetooth/hci_core.c         | 27 ++++++++++++-
> net/bluetooth/hci_event.c        | 78 ++++++++++++++++++++++++++++++++++++
> net/bluetooth/mgmt.c             |  7 ++++
> net/bluetooth/smp.c              | 22 +++-------
> 6 files changed, 122 insertions(+), 19 deletions(-)

all 4 patches have been applied to bluetooth-next tree.

Regards

Marcel