[PATCH 00/15] Bluetooth: LE Privacy support

[PATCH 00/15] Bluetooth: LE Privacy support

[johan.hedberg]

Hi,

This set of patches implements support for the Set Privacy mgmt command,
i.e. the ability to have a local IRK and use it to generate a local RPA.

I'm not expecting most of the patches to have too many points of
discussion, except for the last one: we might want to consider adding
some restrictions to when Set Privacy can be called in order to make the
command implementation simpler. We could e.g. reject the command
whenever advertising is enabled or possibly even when the adapter is
powered on.

Johan

----------------------------------------------------------------
Johan Hedberg (15):
      Bluetooth: Add helper variables to smp_distribute_keys()
      Bluetooth: Add initial code for distributing local IRK
      Bluetooth: Move enable/disable_advertising higher up in mgmt.c
      Bluetooth: Add mgmt defines for privacy
      Bluetooth: Add Privacy lag to mgmt supported/current settings
      Bluetooth: Add skeleton for Set Privacy command
      Bluetooth: Add SMP function for generating RPAs
      Bluetooth: Don't write static address when privacy is enabled
      Bluetooth: Fix hdev->own_addr_type value when privacy is set
      Bluetooth: Add mechanism for updating the local RPA
      Bluetooth: Update local RPA if necessary when connecting LE
      Bluetooth: Update local RPA if necessary before enabling advertising
      Bluetooth: Update local RPA if necessary before starting LE scan
      Bluetooth: Add debugfs entry for RPA regeneration timeout
      Bluetooth: Add full support for updating privacy state

 include/net/bluetooth/hci.h      |   2 +
 include/net/bluetooth/hci_core.h |   9 ++
 include/net/bluetooth/mgmt.h     |   8 ++
 net/bluetooth/hci_conn.c         |   3 +
 net/bluetooth/hci_core.c         |  60 ++++++++++-
 net/bluetooth/mgmt.c             | 198 +++++++++++++++++++++++++++--------
 net/bluetooth/smp.c              |  40 +++++--
 net/bluetooth/smp.h              |   1 +
 8 files changed, 262 insertions(+), 59 deletions(-)

[PATCH 01/15] Bluetooth: Add helper variables to smp_distribute_keys()

[johan.hedberg]

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

This patch a couple of helper variables to the smp_distribute_keys
function in order to avoid long chains of dereferences and thereby help
readability.

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

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index f06068072bdd..6355a460e9d0 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -1132,22 +1132,24 @@ int smp_distribute_keys(struct l2cap_conn *conn, __u8 force)
 {
 	struct smp_cmd_pairing *req, *rsp;
 	struct smp_chan *smp = conn->smp_chan;
+	struct hci_conn *hcon = conn->hcon;
+	struct hci_dev *hdev = hcon->hdev;
 	__u8 *keydist;
 
 	BT_DBG("conn %p force %d", conn, force);
 
-	if (!test_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags))
+	if (!test_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags))
 		return 0;
 
 	rsp = (void *) &smp->prsp[1];
 
 	/* The responder sends its keys first */
-	if (!force && conn->hcon->out && (rsp->resp_key_dist & 0x07))
+	if (!force && hcon->out && (rsp->resp_key_dist & 0x07))
 		return 0;
 
 	req = (void *) &smp->preq[1];
 
-	if (conn->hcon->out) {
+	if (hcon->out) {
 		keydist = &rsp->init_key_dist;
 		*keydist &= req->init_key_dist;
 	} else {
@@ -1160,7 +1162,6 @@ int smp_distribute_keys(struct l2cap_conn *conn, __u8 force)
 	if (*keydist & SMP_DIST_ENC_KEY) {
 		struct smp_cmd_encrypt_info enc;
 		struct smp_cmd_master_ident ident;
-		struct hci_conn *hcon = conn->hcon;
 		struct smp_ltk *ltk;
 		u8 authenticated;
 		__le16 ediv;
@@ -1172,7 +1173,7 @@ int smp_distribute_keys(struct l2cap_conn *conn, __u8 force)
 		smp_send_cmd(conn, SMP_CMD_ENCRYPT_INFO, sizeof(enc), &enc);
 
 		authenticated = hcon->sec_level == BT_SECURITY_HIGH;
-		ltk = hci_add_ltk(hcon->hdev, &hcon->dst, hcon->dst_type,
+		ltk = hci_add_ltk(hdev, &hcon->dst, hcon->dst_type,
 				  HCI_SMP_LTK_SLAVE, authenticated, enc.ltk,
 				  smp->enc_key_size, ediv, ident.rand);
 		smp->slave_ltk = ltk;
@@ -1195,7 +1196,7 @@ int smp_distribute_keys(struct l2cap_conn *conn, __u8 force)
 
 		/* Just public address */
 		memset(&addrinfo, 0, sizeof(addrinfo));
-		bacpy(&addrinfo.bdaddr, &conn->hcon->src);
+		bacpy(&addrinfo.bdaddr, &hcon->src);
 
 		smp_send_cmd(conn, SMP_CMD_IDENT_ADDR_INFO, sizeof(addrinfo),
 			     &addrinfo);
@@ -1214,8 +1215,8 @@ int smp_distribute_keys(struct l2cap_conn *conn, __u8 force)
 		*keydist &= ~SMP_DIST_SIGN;
 	}
 
-	if (conn->hcon->out || force || !(rsp->init_key_dist & 0x07)) {
-		clear_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags);
+	if (hcon->out || force || !(rsp->init_key_dist & 0x07)) {
+		clear_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags);
 		cancel_delayed_work_sync(&conn->security_timer);
 		set_bit(SMP_FLAG_COMPLETE, &smp->smp_flags);
 		smp_notify_keys(conn);
-- 
1.8.5.3

[PATCH 02/15] Bluetooth: Add initial code for distributing local IRK

[johan.hedberg]

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

This code adds a HCI_PRIVACY flag to track whether Privacy support is
enabled (meaning we have a local IRK) and makes sure the IRK is
distributed during SMP key distribution in case this flag is set.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci.h      | 1 +
 include/net/bluetooth/hci_core.h | 2 ++
 net/bluetooth/smp.c              | 6 ++++--
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index fe4b06bfc150..5ff885ff29df 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -126,6 +126,7 @@ enum {
 	HCI_SSP_ENABLED,
 	HCI_SC_ENABLED,
 	HCI_SC_ONLY,
+	HCI_PRIVACY,
 	HCI_RPA_RESOLVING,
 	HCI_HS_ENABLED,
 	HCI_LE_ENABLED,
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index c0fcc041fbb5..68bbcabdd9fd 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -303,6 +303,8 @@ struct hci_dev {
 	__u8			scan_rsp_data[HCI_MAX_AD_LENGTH];
 	__u8			scan_rsp_data_len;
 
+	__u8			irk[16];
+
 	int (*open)(struct hci_dev *hdev);
 	int (*close)(struct hci_dev *hdev);
 	int (*flush)(struct hci_dev *hdev);
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 6355a460e9d0..8ef50c790b96 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -265,6 +265,9 @@ static void build_pairing_cmd(struct l2cap_conn *conn,
 	if (test_bit(HCI_RPA_RESOLVING, &hdev->dev_flags))
 		remote_dist |= SMP_DIST_ID_KEY;
 
+	if (test_bit(HCI_PRIVACY, &hdev->dev_flags))
+		local_dist |= SMP_DIST_ID_KEY;
+
 	if (rsp == NULL) {
 		req->io_capability = conn->hcon->io_capability;
 		req->oob_flag = SMP_OOB_NOT_PRESENT;
@@ -1189,8 +1192,7 @@ int smp_distribute_keys(struct l2cap_conn *conn, __u8 force)
 		struct smp_cmd_ident_addr_info addrinfo;
 		struct smp_cmd_ident_info idinfo;
 
-		/* Send a dummy key */
-		get_random_bytes(idinfo.irk, sizeof(idinfo.irk));
+		memcpy(idinfo.irk, hdev->irk, sizeof(idinfo.irk));
 
 		smp_send_cmd(conn, SMP_CMD_IDENT_INFO, sizeof(idinfo), &idinfo);
 
-- 
1.8.5.3

[PATCH 03/15] Bluetooth: Move enable/disable_advertising higher up in mgmt.c

[johan.hedberg]

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

These functions will soon be needed by the RPA regeneration timeout so
move them higher up in mgmt.c to avoid a forward declaration.

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index bc329d911706..37a6c4eab881 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -812,6 +812,50 @@ static void update_class(struct hci_request *req)
 	hci_req_add(req, HCI_OP_WRITE_CLASS_OF_DEV, sizeof(cod), cod);
 }
 
+static u8 get_adv_type(struct hci_dev *hdev)
+{
+	struct pending_cmd *cmd;
+	bool connectable;
+
+	/* If there's a pending mgmt command the flag will not yet have
+	 * it's final value, so check for this first.
+	 */
+	cmd = mgmt_pending_find(MGMT_OP_SET_CONNECTABLE, hdev);
+	if (cmd) {
+		struct mgmt_mode *cp = cmd->param;
+		connectable = !!cp->val;
+	} else {
+		connectable = test_bit(HCI_CONNECTABLE, &hdev->dev_flags);
+	}
+
+	return connectable ? LE_ADV_IND : LE_ADV_NONCONN_IND;
+}
+
+static void enable_advertising(struct hci_request *req)
+{
+	struct hci_dev *hdev = req->hdev;
+	struct hci_cp_le_set_adv_param cp;
+	u8 enable = 0x01;
+
+	memset(&cp, 0, sizeof(cp));
+	cp.min_interval = __constant_cpu_to_le16(0x0800);
+	cp.max_interval = __constant_cpu_to_le16(0x0800);
+	cp.type = get_adv_type(hdev);
+	cp.own_address_type = hdev->own_addr_type;
+	cp.channel_map = hdev->le_adv_channel_map;
+
+	hci_req_add(req, HCI_OP_LE_SET_ADV_PARAM, sizeof(cp), &cp);
+
+	hci_req_add(req, HCI_OP_LE_SET_ADV_ENABLE, sizeof(enable), &enable);
+}
+
+static void disable_advertising(struct hci_request *req)
+{
+	u8 enable = 0x00;
+
+	hci_req_add(req, HCI_OP_LE_SET_ADV_ENABLE, sizeof(enable), &enable);
+}
+
 static void service_cache_off(struct work_struct *work)
 {
 	struct hci_dev *hdev = container_of(work, struct hci_dev,
@@ -1345,50 +1389,6 @@ static void write_fast_connectable(struct hci_request *req, bool enable)
 		hci_req_add(req, HCI_OP_WRITE_PAGE_SCAN_TYPE, 1, &type);
 }
 
-static u8 get_adv_type(struct hci_dev *hdev)
-{
-	struct pending_cmd *cmd;
-	bool connectable;
-
-	/* If there's a pending mgmt command the flag will not yet have
-	 * it's final value, so check for this first.
-	 */
-	cmd = mgmt_pending_find(MGMT_OP_SET_CONNECTABLE, hdev);
-	if (cmd) {
-		struct mgmt_mode *cp = cmd->param;
-		connectable = !!cp->val;
-	} else {
-		connectable = test_bit(HCI_CONNECTABLE, &hdev->dev_flags);
-	}
-
-	return connectable ? LE_ADV_IND : LE_ADV_NONCONN_IND;
-}
-
-static void enable_advertising(struct hci_request *req)
-{
-	struct hci_dev *hdev = req->hdev;
-	struct hci_cp_le_set_adv_param cp;
-	u8 enable = 0x01;
-
-	memset(&cp, 0, sizeof(cp));
-	cp.min_interval = __constant_cpu_to_le16(0x0800);
-	cp.max_interval = __constant_cpu_to_le16(0x0800);
-	cp.type = get_adv_type(hdev);
-	cp.own_address_type = hdev->own_addr_type;
-	cp.channel_map = hdev->le_adv_channel_map;
-
-	hci_req_add(req, HCI_OP_LE_SET_ADV_PARAM, sizeof(cp), &cp);
-
-	hci_req_add(req, HCI_OP_LE_SET_ADV_ENABLE, sizeof(enable), &enable);
-}
-
-static void disable_advertising(struct hci_request *req)
-{
-	u8 enable = 0x00;
-
-	hci_req_add(req, HCI_OP_LE_SET_ADV_ENABLE, sizeof(enable), &enable);
-}
-
 static void set_connectable_complete(struct hci_dev *hdev, u8 status)
 {
 	struct pending_cmd *cmd;
-- 
1.8.5.3

[PATCH 04/15] Bluetooth: Add mgmt defines for privacy

[johan.hedberg]

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

This patch adds basic mgmt defines for enabling privacy. This includes a
new setting flag as well as the Set Privacy command.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/mgmt.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 2e46251e8aec..62d560624e3d 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -96,6 +96,7 @@ struct mgmt_rp_read_index_list {
 #define MGMT_SETTING_ADVERTISING	0x00000400
 #define MGMT_SETTING_SECURE_CONN	0x00000800
 #define MGMT_SETTING_DEBUG_KEYS		0x00001000
+#define MGMT_SETTING_PRIVACY		0x00002000
 
 #define MGMT_OP_READ_INFO		0x0004
 #define MGMT_READ_INFO_SIZE		0
@@ -389,6 +390,13 @@ struct mgmt_cp_set_scan_params {
 
 #define MGMT_OP_SET_DEBUG_KEYS		0x002E
 
+#define MGMT_OP_SET_PRIVACY		0x002F
+struct mgmt_cp_set_privacy {
+	__u8 privacy;
+	__u8 irk[16];
+} __packed;
+#define MGMT_SET_PRIVACY_SIZE		17
+
 struct mgmt_irk_info {
 	struct mgmt_addr_info addr;
 	__u8 val[16];
-- 
1.8.5.3

[PATCH 05/15] Bluetooth: Add Privacy lag to mgmt supported/current settings

[johan.hedberg]

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

This patch makes sure that the Privacy flag is available in the mgmt
supported settings for all LE capable controllers and in the current
settings whenever the HCI_PRIVACY flag is set.

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 37a6c4eab881..301b18a1c6a0 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -390,6 +390,7 @@ static u32 get_supported_settings(struct hci_dev *hdev)
 	if (lmp_le_capable(hdev)) {
 		settings |= MGMT_SETTING_LE;
 		settings |= MGMT_SETTING_ADVERTISING;
+		settings |= MGMT_SETTING_PRIVACY;
 	}
 
 	return settings;
@@ -438,6 +439,9 @@ static u32 get_current_settings(struct hci_dev *hdev)
 	if (test_bit(HCI_DEBUG_KEYS, &hdev->dev_flags))
 		settings |= MGMT_SETTING_DEBUG_KEYS;
 
+	if (test_bit(HCI_PRIVACY, &hdev->dev_flags))
+		settings |= MGMT_SETTING_PRIVACY;
+
 	return settings;
 }
 
-- 
1.8.5.3

[PATCH 06/15] Bluetooth: Add skeleton for Set Privacy command

[johan.hedberg]

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

This patch adds the basic skeleton for handing the Set Privacy mgmt
command, including copying the value to hdev->irk and toggling the
HCI_PRIVACY flag. The patch does not yet add any kind of RPA generation
and HCI command sending.

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 301b18a1c6a0..145254f84975 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -81,6 +81,7 @@ static const u16 mgmt_commands[] = {
 	MGMT_OP_SET_SCAN_PARAMS,
 	MGMT_OP_SET_SECURE_CONN,
 	MGMT_OP_SET_DEBUG_KEYS,
+	MGMT_OP_SET_PRIVACY,
 	MGMT_OP_LOAD_IRKS,
 };
 
@@ -4187,6 +4188,45 @@ unlock:
 	return err;
 }
 
+static int set_privacy(struct sock *sk, struct hci_dev *hdev, void *cp_data,
+		       u16 len)
+{
+	struct mgmt_cp_set_privacy *cp = cp_data;
+	bool changed;
+	int err;
+
+	BT_DBG("request for %s", hdev->name);
+
+	if (!lmp_le_capable(hdev))
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_PRIVACY,
+				  MGMT_STATUS_NOT_SUPPORTED);
+
+	if (cp->privacy != 0x00 && cp->privacy != 0x01)
+		return cmd_status(sk, hdev->id, MGMT_OP_SET_DEBUG_KEYS,
+				  MGMT_STATUS_INVALID_PARAMS);
+
+	hci_dev_lock(hdev);
+
+	if (cp->privacy) {
+		changed = !test_and_set_bit(HCI_PRIVACY, &hdev->dev_flags);
+		memcpy(hdev->irk, cp->irk, sizeof(hdev->irk));
+	} else {
+		changed = test_and_clear_bit(HCI_PRIVACY, &hdev->dev_flags);
+		memset(hdev->irk, 0, sizeof(hdev->irk));
+	}
+
+	err = send_settings_rsp(sk, MGMT_OP_SET_PRIVACY, hdev);
+	if (err < 0)
+		goto unlock;
+
+	if (changed)
+		err = new_settings(hdev, sk);
+
+unlock:
+	hci_dev_unlock(hdev);
+	return err;
+}
+
 static bool irk_is_valid(struct mgmt_irk_info *irk)
 {
 	switch (irk->addr.type) {
@@ -4401,7 +4441,7 @@ static const struct mgmt_handler {
 	{ set_scan_params,        false, MGMT_SET_SCAN_PARAMS_SIZE },
 	{ set_secure_conn,        false, MGMT_SETTING_SIZE },
 	{ set_debug_keys,         false, MGMT_SETTING_SIZE },
-	{ },
+	{ set_privacy,            false, MGMT_SET_PRIVACY_SIZE },
 	{ load_irks,              true,  MGMT_LOAD_IRKS_SIZE },
 };
 
-- 
1.8.5.3

Re: [PATCH 06/15] Bluetooth: Add skeleton for Set Privacy command

[Marcel Holtmann]

Hi Johan,

> This patch adds the basic skeleton for handing the Set Privacy mgmt
> command, including copying the value to hdev->irk and toggling the
> HCI_PRIVACY flag. The patch does not yet add any kind of RPA generation
> and HCI command sending.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/mgmt.c | 42 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 301b18a1c6a0..145254f84975 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -81,6 +81,7 @@ static const u16 mgmt_commands[] = {
> 	MGMT_OP_SET_SCAN_PARAMS,
> 	MGMT_OP_SET_SECURE_CONN,
> 	MGMT_OP_SET_DEBUG_KEYS,
> +	MGMT_OP_SET_PRIVACY,
> 	MGMT_OP_LOAD_IRKS,
> };
> 
> @@ -4187,6 +4188,45 @@ unlock:
> 	return err;
> }
> 
> +static int set_privacy(struct sock *sk, struct hci_dev *hdev, void *cp_data,
> +		       u16 len)
> +{
> +	struct mgmt_cp_set_privacy *cp = cp_data;
> +	bool changed;
> +	int err;
> +
> +	BT_DBG("request for %s", hdev->name);
> +
> +	if (!lmp_le_capable(hdev))
> +		return cmd_status(sk, hdev->id, MGMT_OP_SET_PRIVACY,
> +				  MGMT_STATUS_NOT_SUPPORTED);
> +
> +	if (cp->privacy != 0x00 && cp->privacy != 0x01)
> +		return cmd_status(sk, hdev->id, MGMT_OP_SET_DEBUG_KEYS,
> +				  MGMT_STATUS_INVALID_PARAMS);

copy&paste error ;)

And I think this command should only be allowed when powered off. Same as we do for setting the static address. If you change the IRK, then you screw your peers over. Something we should not even consider allowing.

If you want to change your identity, then power down and power up. For LE only devices without a public address, they have to power down to change their static address (which will be the identity for them) anyway.

Regards

Marcel

[PATCH 07/15] Bluetooth: Add SMP function for generating RPAs

[johan.hedberg]

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

We need a function in smp.c to generate Resolvable Random Addresses in
order to support privacy. The local RPA will need to be generated before
advertising, scanning or connecting and regenerated at periodic
intervals. This patch adds the necessary function for RPA generation.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/smp.c | 17 +++++++++++++++++
 net/bluetooth/smp.h |  1 +
 2 files changed, 18 insertions(+)

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 8ef50c790b96..7ec48e3d6065 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -124,6 +124,23 @@ bool smp_irk_matches(struct crypto_blkcipher *tfm, u8 irk[16],
 	return !memcmp(bdaddr->b, hash, 3);
 }
 
+int smp_generate_rpa(struct crypto_blkcipher *tfm, u8 irk[16], bdaddr_t *rpa)
+{
+	int err;
+
+	get_random_bytes(&rpa->b[3], 3);
+
+	rpa->b[5] &= 0x7f;
+
+	err = smp_ah(tfm, irk, &rpa->b[3], rpa->b);
+	if (err < 0)
+		return err;
+
+	BT_DBG("RPA %pMR", rpa);
+
+	return 0;
+}
+
 static int smp_c1(struct crypto_blkcipher *tfm, u8 k[16], u8 r[16],
 		  u8 preq[7], u8 pres[7], u8 _iat, bdaddr_t *ia,
 		  u8 _rat, bdaddr_t *ra, u8 res[16])
diff --git a/net/bluetooth/smp.h b/net/bluetooth/smp.h
index d8cc543f523c..f32f1212f650 100644
--- a/net/bluetooth/smp.h
+++ b/net/bluetooth/smp.h
@@ -152,5 +152,6 @@ void smp_chan_destroy(struct l2cap_conn *conn);
 
 bool smp_irk_matches(struct crypto_blkcipher *tfm, u8 irk[16],
 		     bdaddr_t *bdaddr);
+int smp_generate_rpa(struct crypto_blkcipher *tfm, u8 irk[16], bdaddr_t *rpa);
 
 #endif /* __SMP_H */
-- 
1.8.5.3

Re: [PATCH 07/15] Bluetooth: Add SMP function for generating RPAs

[Marcel Holtmann]

Hi Johan,

> We need a function in smp.c to generate Resolvable Random Addresses in
> order to support privacy. The local RPA will need to be generated before
> advertising, scanning or connecting and regenerated at periodic
> intervals. This patch adds the necessary function for RPA generation.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/smp.c | 17 +++++++++++++++++
> net/bluetooth/smp.h |  1 +
> 2 files changed, 18 insertions(+)
> 
> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
> index 8ef50c790b96..7ec48e3d6065 100644
> --- a/net/bluetooth/smp.c
> +++ b/net/bluetooth/smp.c
> @@ -124,6 +124,23 @@ bool smp_irk_matches(struct crypto_blkcipher *tfm, u8 irk[16],
> 	return !memcmp(bdaddr->b, hash, 3);
> }
> 
> +int smp_generate_rpa(struct crypto_blkcipher *tfm, u8 irk[16], bdaddr_t *rpa)
> +{
> +	int err;
> +
> +	get_random_bytes(&rpa->b[3], 3);
> +
> +	rpa->b[5] &= 0x7f;

I think this one is wrong. You can now end up with RPA or URPA based if the most significant bit coming from the random bytes is set or not. This is not reliably creating a RPA.

Regards

Marcel

[PATCH 08/15] Bluetooth: Don't write static address when privacy is enabled

[johan.hedberg]

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

If privacy is enabled we'll be using RPAs instead of the static address.
Therefore, only write the static address to HCI when the HCI_PRIVACY
flag is not set.

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 145254f84975..21d059968c32 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4609,7 +4609,8 @@ static int powered_update_hci(struct hci_dev *hdev)
 
 	if (lmp_le_capable(hdev)) {
 		/* Set random address to static address if configured */
-		if (bacmp(&hdev->static_addr, BDADDR_ANY))
+		if (!test_bit(HCI_PRIVACY, &hdev->dev_flags) &&
+		    bacmp(&hdev->static_addr, BDADDR_ANY))
 			hci_req_add(&req, HCI_OP_LE_SET_RANDOM_ADDR, 6,
 				    &hdev->static_addr);
 
-- 
1.8.5.3

Re: [PATCH 08/15] Bluetooth: Don't write static address when privacy is enabled

[Marcel Holtmann]

Hi Johan,

> If privacy is enabled we'll be using RPAs instead of the static address.
> Therefore, only write the static address to HCI when the HCI_PRIVACY
> flag is not set.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/mgmt.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 145254f84975..21d059968c32 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -4609,7 +4609,8 @@ static int powered_update_hci(struct hci_dev *hdev)
> 
> 	if (lmp_le_capable(hdev)) {
> 		/* Set random address to static address if configured */
> -		if (bacmp(&hdev->static_addr, BDADDR_ANY))
> +		if (!test_bit(HCI_PRIVACY, &hdev->dev_flags) &&
> +		    bacmp(&hdev->static_addr, BDADDR_ANY))
> 			hci_req_add(&req, HCI_OP_LE_SET_RANDOM_ADDR, 6,
> 				    &hdev->static_addr);

since we are changing to code to always write the random address before enabling scanning, before enabling advertising and before creating a connection, lets remove this support here completely.

If we are not using privacy and have a static address, lets write it before it gets used. I added tracing of the currently configured random address. So we always know when we have to update the address itself or not. That makes it a bit more error free and gives us less headaches to figure static vs RPA.

Regards

Marcel

[PATCH 09/15] Bluetooth: Fix hdev->own_addr_type value when privacy is set

[johan.hedberg]

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

When powering on the controller and deciding what value to set to
hdev->own_address_time take also into account the HCI_PRIVACY flag (in
which case random own address should be used).

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

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 964aa8deb009..753a73ba6764 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1477,14 +1477,16 @@ static void hci_init3_req(struct hci_request *req, unsigned long opt)
 
 	if (lmp_le_capable(hdev)) {
 		/* If the controller has a public BD_ADDR, then by default
-		 * use that one. If this is a LE only controller without
-		 * a public address, default to the random address.
+		 * use that one, unless Privacy is enabled. If this is a LE
+		 * only controller without a public address, default to
+		 * the random address.
 		 *
 		 * For debugging purposes it is possible to force
 		 * controllers with a public address to use the
 		 * random address instead.
 		 */
-		if (test_bit(HCI_FORCE_STATIC_ADDR, &hdev->dev_flags) ||
+		if (test_bit(HCI_PRIVACY, &hdev->dev_flags) ||
+		    test_bit(HCI_FORCE_STATIC_ADDR, &hdev->dev_flags) ||
 		    !bacmp(&hdev->bdaddr, BDADDR_ANY))
 			hdev->own_addr_type = ADDR_LE_DEV_RANDOM;
 		else
-- 
1.8.5.3

Re: [PATCH 09/15] Bluetooth: Fix hdev->own_addr_type value when privacy is set

[Marcel Holtmann]

Hi Johan,

> When powering on the controller and deciding what value to set to
> hdev->own_address_time take also into account the HCI_PRIVACY flag (in
> which case random own address should be used).
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/hci_core.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 964aa8deb009..753a73ba6764 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1477,14 +1477,16 @@ static void hci_init3_req(struct hci_request *req, unsigned long opt)
> 
> 	if (lmp_le_capable(hdev)) {
> 		/* If the controller has a public BD_ADDR, then by default
> -		 * use that one. If this is a LE only controller without
> -		 * a public address, default to the random address.
> +		 * use that one, unless Privacy is enabled. If this is a LE
> +		 * only controller without a public address, default to
> +		 * the random address.
> 		 *
> 		 * For debugging purposes it is possible to force
> 		 * controllers with a public address to use the
> 		 * random address instead.
> 		 */
> -		if (test_bit(HCI_FORCE_STATIC_ADDR, &hdev->dev_flags) ||
> +		if (test_bit(HCI_PRIVACY, &hdev->dev_flags) ||
> +		    test_bit(HCI_FORCE_STATIC_ADDR, &hdev->dev_flags) ||
> 		    !bacmp(&hdev->bdaddr, BDADDR_ANY))
> 			hdev->own_addr_type = ADDR_LE_DEV_RANDOM;

this patch is actually wrong. The hdev->own_addr_type should refer to the identity address. That one is either public address or static random address.

We need to check the address type we set for each command and set either hdev->own_addr_type if privacy is off or when privacy is on use ADDR_LE_DEV_RANDOM.

Please remember that we need to send the identity address type over SMP. So we need to know which one that is.

Regards

Marcel

[PATCH 10/15] Bluetooth: Add mechanism for updating the local RPA

[johan.hedberg]

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

We need to have a way of updating the local RPA periodically. This patch
adds a HCI_RPA_EXPIRED flag to track when the RPA needs to be
regenerated as well as a delayed work structure and callback to do the
updating periodically. By default the period for updating the RPA is set
to 15 minutes.

When the RPA gets regenerated we need to disable and re-enable
advertising if necessary. If there are any LE connections we do not
update the RPA.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci.h      |  1 +
 include/net/bluetooth/hci_core.h |  7 +++++++
 net/bluetooth/hci_core.c         | 25 +++++++++++++++++++++++++
 net/bluetooth/mgmt.c             | 23 +++++++++++++++++++++++
 4 files changed, 56 insertions(+)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 5ff885ff29df..1bb45a47a78a 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -127,6 +127,7 @@ enum {
 	HCI_SC_ENABLED,
 	HCI_SC_ONLY,
 	HCI_PRIVACY,
+	HCI_RPA_EXPIRED,
 	HCI_RPA_RESOLVING,
 	HCI_HS_ENABLED,
 	HCI_LE_ENABLED,
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 68bbcabdd9fd..94383dca6597 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -130,6 +130,9 @@ struct oob_data {
 
 #define HCI_MAX_SHORT_NAME_LENGTH	10
 
+/* Default LE RPA expiry time, 15 minutes */
+#define HCI_DEFAULT_RPA_TIMEOUT		(15 * 60)
+
 struct amp_assoc {
 	__u16	len;
 	__u16	offset;
@@ -304,6 +307,8 @@ struct hci_dev {
 	__u8			scan_rsp_data_len;
 
 	__u8			irk[16];
+	__u32			rpa_timeout;
+	struct delayed_work	rpa_expired;
 
 	int (*open)(struct hci_dev *hdev);
 	int (*close)(struct hci_dev *hdev);
@@ -1252,6 +1257,8 @@ void hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max,
 void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
 							__u8 ltk[16]);
 
+void hci_update_rpa(struct hci_request *req);
+
 #define SCO_AIRMODE_MASK       0x0003
 #define SCO_AIRMODE_CVSD       0x0000
 #define SCO_AIRMODE_TRANSP     0x0003
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 753a73ba6764..eeeeda7e6b70 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2202,6 +2202,9 @@ static int hci_dev_do_close(struct hci_dev *hdev)
 
 	cancel_delayed_work_sync(&hdev->le_scan_disable);
 
+	cancel_delayed_work_sync(&hdev->rpa_expired);
+	set_bit(HCI_RPA_EXPIRED, &hdev->dev_flags);
+
 	hci_dev_lock(hdev);
 	hci_inquiry_cache_flush(hdev);
 	hci_conn_hash_flush(hdev);
@@ -3276,6 +3279,26 @@ static void le_scan_disable_work(struct work_struct *work)
 		BT_ERR("Disable LE scanning request failed: err %d", err);
 }
 
+void hci_update_rpa(struct hci_request *req)
+{
+	struct hci_dev *hdev = req->hdev;
+	bdaddr_t rpa;
+
+	if (smp_generate_rpa(hdev->tfm_aes, hdev->irk, &rpa) < 0) {
+		BT_ERR("%s failed to generate new RPA", hdev->name);
+		return;
+	}
+
+	hci_req_add(req, HCI_OP_LE_SET_RANDOM_ADDR, 6, &rpa);
+
+	if (hdev->rpa_timeout) {
+		int to;
+
+		to = msecs_to_jiffies(hdev->rpa_timeout * 1000);
+		queue_delayed_work(hdev->workqueue, &hdev->rpa_expired, to);
+	}
+}
+
 /* Alloc HCI device */
 struct hci_dev *hci_alloc_dev(void)
 {
@@ -3302,6 +3325,8 @@ struct hci_dev *hci_alloc_dev(void)
 	hdev->le_conn_min_interval = 0x0028;
 	hdev->le_conn_max_interval = 0x0038;
 
+	hdev->rpa_timeout = HCI_DEFAULT_RPA_TIMEOUT;
+
 	mutex_init(&hdev->lock);
 	mutex_init(&hdev->req_lock);
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 21d059968c32..248f116e3d08 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -882,12 +882,35 @@ static void service_cache_off(struct work_struct *work)
 	hci_req_run(&req, NULL);
 }
 
+static void rpa_expired(struct work_struct *work)
+{
+	struct hci_dev *hdev = container_of(work, struct hci_dev,
+					    rpa_expired.work);
+	struct hci_request req;
+
+	BT_DBG("");
+
+	set_bit(HCI_RPA_EXPIRED, &hdev->dev_flags);
+
+	if (!test_bit(HCI_ADVERTISING, &hdev->dev_flags) ||
+	    hci_conn_num(hdev, LE_LINK) > 0)
+		return;
+
+	hci_req_init(&req, hdev);
+
+	disable_advertising(&req);
+	enable_advertising(&req);
+
+	hci_req_run(&req, NULL);
+}
+
 static void mgmt_init_hdev(struct sock *sk, struct hci_dev *hdev)
 {
 	if (test_and_set_bit(HCI_MGMT, &hdev->dev_flags))
 		return;
 
 	INIT_DELAYED_WORK(&hdev->service_cache, service_cache_off);
+	INIT_DELAYED_WORK(&hdev->rpa_expired, rpa_expired);
 
 	/* Non-mgmt controlled devices get this bit set
 	 * implicitly so that pairing works for them, however
-- 
1.8.5.3

Re: [PATCH 10/15] Bluetooth: Add mechanism for updating the local RPA

[Marcel Holtmann]

Hi Johan,

> We need to have a way of updating the local RPA periodically. This patch
> adds a HCI_RPA_EXPIRED flag to track when the RPA needs to be
> regenerated as well as a delayed work structure and callback to do the
> updating periodically. By default the period for updating the RPA is set
> to 15 minutes.
> 
> When the RPA gets regenerated we need to disable and re-enable
> advertising if necessary. If there are any LE connections we do not
> update the RPA.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/hci.h      |  1 +
> include/net/bluetooth/hci_core.h |  7 +++++++
> net/bluetooth/hci_core.c         | 25 +++++++++++++++++++++++++
> net/bluetooth/mgmt.c             | 23 +++++++++++++++++++++++
> 4 files changed, 56 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 5ff885ff29df..1bb45a47a78a 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -127,6 +127,7 @@ enum {
> 	HCI_SC_ENABLED,
> 	HCI_SC_ONLY,
> 	HCI_PRIVACY,
> +	HCI_RPA_EXPIRED,
> 	HCI_RPA_RESOLVING,
> 	HCI_HS_ENABLED,
> 	HCI_LE_ENABLED,
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 68bbcabdd9fd..94383dca6597 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -130,6 +130,9 @@ struct oob_data {
> 
> #define HCI_MAX_SHORT_NAME_LENGTH	10
> 
> +/* Default LE RPA expiry time, 15 minutes */
> +#define HCI_DEFAULT_RPA_TIMEOUT		(15 * 60)
> +
> struct amp_assoc {
> 	__u16	len;
> 	__u16	offset;
> @@ -304,6 +307,8 @@ struct hci_dev {
> 	__u8			scan_rsp_data_len;
> 
> 	__u8			irk[16];
> +	__u32			rpa_timeout;
> +	struct delayed_work	rpa_expired;
> 
> 	int (*open)(struct hci_dev *hdev);
> 	int (*close)(struct hci_dev *hdev);
> @@ -1252,6 +1257,8 @@ void hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max,
> void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
> 							__u8 ltk[16]);
> 
> +void hci_update_rpa(struct hci_request *req);
> +
> #define SCO_AIRMODE_MASK       0x0003
> #define SCO_AIRMODE_CVSD       0x0000
> #define SCO_AIRMODE_TRANSP     0x0003
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 753a73ba6764..eeeeda7e6b70 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2202,6 +2202,9 @@ static int hci_dev_do_close(struct hci_dev *hdev)
> 
> 	cancel_delayed_work_sync(&hdev->le_scan_disable);
> 
> +	cancel_delayed_work_sync(&hdev->rpa_expired);
> +	set_bit(HCI_RPA_EXPIRED, &hdev->dev_flags);
> +

why on power down. Why not just set this on power up.

> 	hci_dev_lock(hdev);
> 	hci_inquiry_cache_flush(hdev);
> 	hci_conn_hash_flush(hdev);
> @@ -3276,6 +3279,26 @@ static void le_scan_disable_work(struct work_struct *work)
> 		BT_ERR("Disable LE scanning request failed: err %d", err);
> }
> 
> +void hci_update_rpa(struct hci_request *req)
> +{
> +	struct hci_dev *hdev = req->hdev;
> +	bdaddr_t rpa;
> +
> +	if (smp_generate_rpa(hdev->tfm_aes, hdev->irk, &rpa) < 0) {
> +		BT_ERR("%s failed to generate new RPA", hdev->name);
> +		return;
> +	}
> +
> +	hci_req_add(req, HCI_OP_LE_SET_RANDOM_ADDR, 6, &rpa);
> +
> +	if (hdev->rpa_timeout) {
> +		int to;
> +
> +		to = msecs_to_jiffies(hdev->rpa_timeout * 1000);

I thought our rpa_timeout is in minutes? Are we allowing second granularity? I have nothing against it, but it will be rather useless to have a device that changes its RPA every second.

> +		queue_delayed_work(hdev->workqueue, &hdev->rpa_expired, to);
> +	}
> +}
> +
> /* Alloc HCI device */
> struct hci_dev *hci_alloc_dev(void)
> {
> @@ -3302,6 +3325,8 @@ struct hci_dev *hci_alloc_dev(void)
> 	hdev->le_conn_min_interval = 0x0028;
> 	hdev->le_conn_max_interval = 0x0038;
> 
> +	hdev->rpa_timeout = HCI_DEFAULT_RPA_TIMEOUT;
> +
> 	mutex_init(&hdev->lock);
> 	mutex_init(&hdev->req_lock);
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 21d059968c32..248f116e3d08 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -882,12 +882,35 @@ static void service_cache_off(struct work_struct *work)
> 	hci_req_run(&req, NULL);
> }
> 
> +static void rpa_expired(struct work_struct *work)
> +{
> +	struct hci_dev *hdev = container_of(work, struct hci_dev,
> +					    rpa_expired.work);
> +	struct hci_request req;
> +
> +	BT_DBG("");
> +
> +	set_bit(HCI_RPA_EXPIRED, &hdev->dev_flags);
> +
> +	if (!test_bit(HCI_ADVERTISING, &hdev->dev_flags) ||
> +	    hci_conn_num(hdev, LE_LINK) > 0)
> +		return;
> +
> +	hci_req_init(&req, hdev);
> +
> +	disable_advertising(&req);
> +	enable_advertising(&req);
> +

might want to add a comment that the RPA gets regenerated when enabling advertising. Just to make it easy for everybody to get that this code does the right thing.

> +	hci_req_run(&req, NULL);
> +}
> +
> static void mgmt_init_hdev(struct sock *sk, struct hci_dev *hdev)
> {
> 	if (test_and_set_bit(HCI_MGMT, &hdev->dev_flags))
> 		return;
> 
> 	INIT_DELAYED_WORK(&hdev->service_cache, service_cache_off);
> +	INIT_DELAYED_WORK(&hdev->rpa_expired, rpa_expired);
> 
> 	/* Non-mgmt controlled devices get this bit set
> 	 * implicitly so that pairing works for them, however

We should set the RPA_EXPIRED bit here.

Regards

Marcel

Re: [PATCH 10/15] Bluetooth: Add mechanism for updating the local RPA

[Johan Hedberg]

Hi Marcel,

On Sat, Feb 22, 2014, Marcel Holtmann wrote:
> > @@ -2202,6 +2202,9 @@ static int hci_dev_do_close(struct hci_dev *hdev)
> > 
> > 	cancel_delayed_work_sync(&hdev->le_scan_disable);
> > 
> > +	cancel_delayed_work_sync(&hdev->rpa_expired);
> > +	set_bit(HCI_RPA_EXPIRED, &hdev->dev_flags);
> > +
> 
> why on power down. Why not just set this on power up.

No reason. I can move it to power up.

> > @@ -3276,6 +3279,26 @@ static void le_scan_disable_work(struct work_struct *work)
> > 		BT_ERR("Disable LE scanning request failed: err %d", err);
> > }
> > 
> > +void hci_update_rpa(struct hci_request *req)
> > +{
> > +	struct hci_dev *hdev = req->hdev;
> > +	bdaddr_t rpa;
> > +
> > +	if (smp_generate_rpa(hdev->tfm_aes, hdev->irk, &rpa) < 0) {
> > +		BT_ERR("%s failed to generate new RPA", hdev->name);
> > +		return;
> > +	}
> > +
> > +	hci_req_add(req, HCI_OP_LE_SET_RANDOM_ADDR, 6, &rpa);
> > +
> > +	if (hdev->rpa_timeout) {
> > +		int to;
> > +
> > +		to = msecs_to_jiffies(hdev->rpa_timeout * 1000);
> 
> I thought our rpa_timeout is in minutes? Are we allowing second
> granularity? I have nothing against it, but it will be rather useless
> to have a device that changes its RPA every second.

Main reason is that we have other entries settable in seconds and it's
easier to verify that the timer is actually working if you don't have to
wait a full minute to change it. Not a big issue for me though, so I can
change it if you really want.

> > static void mgmt_init_hdev(struct sock *sk, struct hci_dev *hdev)
> > {
> > 	if (test_and_set_bit(HCI_MGMT, &hdev->dev_flags))
> > 		return;
> > 
> > 	INIT_DELAYED_WORK(&hdev->service_cache, service_cache_off);
> > +	INIT_DELAYED_WORK(&hdev->rpa_expired, rpa_expired);
> > 
> > 	/* Non-mgmt controlled devices get this bit set
> > 	 * implicitly so that pairing works for them, however
> 
> We should set the RPA_EXPIRED bit here.

I don't think so. All the test_and_clear_bit(RPA_EXPIRED) places assume
that the flag will only be set if HCI_PRIVACY is also set. So I think
the right place to initialize it for the first time is when HCI_PRIVACY
is first set, i.e. in the set_privacy mgmt handler.

Johan

Re: [PATCH 10/15] Bluetooth: Add mechanism for updating the local RPA

[Marcel Holtmann]

Hi Johan,

>>> @@ -2202,6 +2202,9 @@ static int hci_dev_do_close(struct hci_dev *hdev)
>>> 
>>> 	cancel_delayed_work_sync(&hdev->le_scan_disable);
>>> 
>>> +	cancel_delayed_work_sync(&hdev->rpa_expired);
>>> +	set_bit(HCI_RPA_EXPIRED, &hdev->dev_flags);
>>> +
>> 
>> why on power down. Why not just set this on power up.
> 
> No reason. I can move it to power up.

lets move it there to make sure we always start out with a fresh one.

>>> @@ -3276,6 +3279,26 @@ static void le_scan_disable_work(struct work_struct *work)
>>> 		BT_ERR("Disable LE scanning request failed: err %d", err);
>>> }
>>> 
>>> +void hci_update_rpa(struct hci_request *req)
>>> +{
>>> +	struct hci_dev *hdev = req->hdev;
>>> +	bdaddr_t rpa;
>>> +
>>> +	if (smp_generate_rpa(hdev->tfm_aes, hdev->irk, &rpa) < 0) {
>>> +		BT_ERR("%s failed to generate new RPA", hdev->name);
>>> +		return;
>>> +	}
>>> +
>>> +	hci_req_add(req, HCI_OP_LE_SET_RANDOM_ADDR, 6, &rpa);
>>> +
>>> +	if (hdev->rpa_timeout) {
>>> +		int to;
>>> +
>>> +		to = msecs_to_jiffies(hdev->rpa_timeout * 1000);
>> 
>> I thought our rpa_timeout is in minutes? Are we allowing second
>> granularity? I have nothing against it, but it will be rather useless
>> to have a device that changes its RPA every second.
> 
> Main reason is that we have other entries settable in seconds and it's
> easier to verify that the timer is actually working if you don't have to
> wait a full minute to change it. Not a big issue for me though, so I can
> change it if you really want.

Keep it in seconds then and let us restrict it to a sensible range mentioned in the debugfs comment.

> 
>>> static void mgmt_init_hdev(struct sock *sk, struct hci_dev *hdev)
>>> {
>>> 	if (test_and_set_bit(HCI_MGMT, &hdev->dev_flags))
>>> 		return;
>>> 
>>> 	INIT_DELAYED_WORK(&hdev->service_cache, service_cache_off);
>>> +	INIT_DELAYED_WORK(&hdev->rpa_expired, rpa_expired);
>>> 
>>> 	/* Non-mgmt controlled devices get this bit set
>>> 	 * implicitly so that pairing works for them, however
>> 
>> We should set the RPA_EXPIRED bit here.
> 
> I don't think so. All the test_and_clear_bit(RPA_EXPIRED) places assume
> that the flag will only be set if HCI_PRIVACY is also set. So I think
> the right place to initialize it for the first time is when HCI_PRIVACY
> is first set, i.e. in the set_privacy mgmt handler.

That might need changing. See my other comments. We should create a helper that can that check there.

Regards

Marcel

[PATCH 11/15] Bluetooth: Update local RPA if necessary when connecting LE

[johan.hedberg]

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

When we initiate LE connections we need to regenerate the local RPA in
case it has expired. This patch adds the necessary code for doing this
in the hci_create_le_conn function.

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

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index bd66c52eff95..325b42afc61e 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -560,6 +560,9 @@ static int hci_create_le_conn(struct hci_conn *conn)
 
 	hci_req_init(&req, hdev);
 
+	if (test_and_clear_bit(HCI_RPA_EXPIRED, &hdev->dev_flags))
+		hci_update_rpa(&req);
+
 	memset(&cp, 0, sizeof(cp));
 	cp.scan_interval = cpu_to_le16(hdev->le_scan_interval);
 	cp.scan_window = cpu_to_le16(hdev->le_scan_window);
-- 
1.8.5.3

Re: [PATCH 11/15] Bluetooth: Update local RPA if necessary when connecting LE

[Marcel Holtmann]

Hi Johan,

> When we initiate LE connections we need to regenerate the local RPA in
> case it has expired. This patch adds the necessary code for doing this
> in the hci_create_le_conn function.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/hci_conn.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index bd66c52eff95..325b42afc61e 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -560,6 +560,9 @@ static int hci_create_le_conn(struct hci_conn *conn)
> 
> 	hci_req_init(&req, hdev);
> 
> +	if (test_and_clear_bit(HCI_RPA_EXPIRED, &hdev->dev_flags))
> +		hci_update_rpa(&req);
> +

and if privacy is not enabled, we should make sure the static address is the one that the controller currently knows about.

As explained in my other comment, this will make it long term a lot simpler. Maybe you want to introduce a helper for updating the random address. That why we just call it in certain places and it takes care of either writing a new RPA or it makes sure the static is set in case it differs.

Regards

Marcel

[PATCH 12/15] Bluetooth: Update local RPA if necessary before enabling advertising

[johan.hedberg]

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

When we enable advertising we need to regenerate the local RPA in case
it has expired. This patch adds the necessary code for doing this in the
enable_advertising function.

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 248f116e3d08..e3d8371a2493 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -842,6 +842,9 @@ static void enable_advertising(struct hci_request *req)
 	struct hci_cp_le_set_adv_param cp;
 	u8 enable = 0x01;
 
+	if (test_and_clear_bit(HCI_RPA_EXPIRED, &hdev->dev_flags))
+		hci_update_rpa(req);
+
 	memset(&cp, 0, sizeof(cp));
 	cp.min_interval = __constant_cpu_to_le16(0x0800);
 	cp.max_interval = __constant_cpu_to_le16(0x0800);
-- 
1.8.5.3

Re: [PATCH 12/15] Bluetooth: Update local RPA if necessary before enabling advertising

[Marcel Holtmann]

Hi Johan,

> When we enable advertising we need to regenerate the local RPA in case
> it has expired. This patch adds the necessary code for doing this in the
> enable_advertising function.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/mgmt.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 248f116e3d08..e3d8371a2493 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -842,6 +842,9 @@ static void enable_advertising(struct hci_request *req)
> 	struct hci_cp_le_set_adv_param cp;
> 	u8 enable = 0x01;
> 
> +	if (test_and_clear_bit(HCI_RPA_EXPIRED, &hdev->dev_flags))
> +		hci_update_rpa(req);
> +

same comment as the others. Lets also take the static address into account and make sure it up-to-date.

Regards

Marcel

[PATCH 13/15] Bluetooth: Update local RPA if necessary before starting LE scan

[johan.hedberg]

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

When we start LE scanning we need to regenerate the local RPA in case it
has expired. This patch adds the necessary code for doing this in the
start_discovery function.

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index e3d8371a2493..2dd0c730f0e9 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3382,6 +3382,9 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
 			goto failed;
 		}
 
+		if (test_and_clear_bit(HCI_RPA_EXPIRED, &hdev->dev_flags))
+			hci_update_rpa(&req);
+
 		memset(&param_cp, 0, sizeof(param_cp));
 		param_cp.type = LE_SCAN_ACTIVE;
 		param_cp.interval = cpu_to_le16(DISCOV_LE_SCAN_INT);
-- 
1.8.5.3

Re: [PATCH 13/15] Bluetooth: Update local RPA if necessary before starting LE scan

[Marcel Holtmann]

Hi Johan,

> When we start LE scanning we need to regenerate the local RPA in case it
> has expired. This patch adds the necessary code for doing this in the
> start_discovery function.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/mgmt.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index e3d8371a2493..2dd0c730f0e9 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -3382,6 +3382,9 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
> 			goto failed;
> 		}
> 
> +		if (test_and_clear_bit(HCI_RPA_EXPIRED, &hdev->dev_flags))
> +			hci_update_rpa(&req);
> +

same comment here. We need to take care of the static address as well here.

Regards

Marcel

[PATCH 14/15] Bluetooth: Add debugfs entry for RPA regeneration timeout

[johan.hedberg]

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

This patch adds a rpa_timeout debugfs entry which can be used to set the
RPA regeneration timeout to something else than the default 15 minutes.

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

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index eeeeda7e6b70..ecd147e51f45 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -492,6 +492,31 @@ static int idle_timeout_get(void *data, u64 *val)
 DEFINE_SIMPLE_ATTRIBUTE(idle_timeout_fops, idle_timeout_get,
 			idle_timeout_set, "%llu\n");
 
+static int rpa_timeout_set(void *data, u64 val)
+{
+	struct hci_dev *hdev = data;
+
+	hci_dev_lock(hdev);
+	hdev->rpa_timeout = val;
+	hci_dev_unlock(hdev);
+
+	return 0;
+}
+
+static int rpa_timeout_get(void *data, u64 *val)
+{
+	struct hci_dev *hdev = data;
+
+	hci_dev_lock(hdev);
+	*val = hdev->rpa_timeout;
+	hci_dev_unlock(hdev);
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(rpa_timeout_fops, rpa_timeout_get,
+			rpa_timeout_set, "%llu\n");
+
 static int sniff_min_interval_set(void *data, u64 val)
 {
 	struct hci_dev *hdev = data;
@@ -1614,6 +1639,8 @@ static int __hci_init(struct hci_dev *hdev)
 				    hdev, &random_address_fops);
 		debugfs_create_file("static_address", 0444, hdev->debugfs,
 				    hdev, &static_address_fops);
+		debugfs_create_file("rpa_timeout", 0644, hdev->debugfs,
+				    hdev, &rpa_timeout_fops);
 
 		/* For controllers with a public address, provide a debug
 		 * option to force the usage of the configured static
-- 
1.8.5.3

Re: [PATCH 14/15] Bluetooth: Add debugfs entry for RPA regeneration timeout

[Marcel Holtmann]

Hi Johan,

> This patch adds a rpa_timeout debugfs entry which can be used to set the
> RPA regeneration timeout to something else than the default 15 minutes.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/hci_core.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index eeeeda7e6b70..ecd147e51f45 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -492,6 +492,31 @@ static int idle_timeout_get(void *data, u64 *val)
> DEFINE_SIMPLE_ATTRIBUTE(idle_timeout_fops, idle_timeout_get,
> 			idle_timeout_set, "%llu\n");
> 
> +static int rpa_timeout_set(void *data, u64 val)
> +{
> +	struct hci_dev *hdev = data;
> +

if we are doing this in seconds, we might want to set some limits here. Don’t know which make sense, but something like at least 30 seconds and not more than 24 hours.

> +	hci_dev_lock(hdev);
> +	hdev->rpa_timeout = val;
> +	hci_dev_unlock(hdev);
> +
> +	return 0;
> +}

Regards

Marcel

[PATCH 15/15] Bluetooth: Add full support for updating privacy state

[johan.hedberg]

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

There are several things that need updating in addition to the
HCI_PRIVACY flag when Set Privacy is called. This includes toggling the
HCI_RPA_EXPIRED flag, setting the own_address_type variable, as well as
restarting advertising if necessary.

This patch adds these missing pieces to the set_privacy function.

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 2dd0c730f0e9..7d0ab8b8d65c 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4221,6 +4221,7 @@ static int set_privacy(struct sock *sk, struct hci_dev *hdev, void *cp_data,
 		       u16 len)
 {
 	struct mgmt_cp_set_privacy *cp = cp_data;
+	struct hci_request req;
 	bool changed;
 	int err;
 
@@ -4239,11 +4240,42 @@ static int set_privacy(struct sock *sk, struct hci_dev *hdev, void *cp_data,
 	if (cp->privacy) {
 		changed = !test_and_set_bit(HCI_PRIVACY, &hdev->dev_flags);
 		memcpy(hdev->irk, cp->irk, sizeof(hdev->irk));
+		hdev->own_addr_type = ADDR_LE_DEV_RANDOM;
+		set_bit(HCI_RPA_EXPIRED, &hdev->dev_flags);
 	} else {
 		changed = test_and_clear_bit(HCI_PRIVACY, &hdev->dev_flags);
 		memset(hdev->irk, 0, sizeof(hdev->irk));
+		cancel_delayed_work(&hdev->rpa_expired);
+		clear_bit(HCI_RPA_EXPIRED, &hdev->dev_flags);
+
+		if (!changed)
+			goto send_rsp;
+
+		if (test_bit(HCI_FORCE_STATIC_ADDR, &hdev->dev_flags) ||
+		    !bacmp(&hdev->bdaddr, BDADDR_ANY))
+			hdev->own_addr_type = ADDR_LE_DEV_RANDOM;
+		else
+			hdev->own_addr_type = ADDR_LE_DEV_PUBLIC;
 	}
 
+	if (!changed || !hdev_is_powered(hdev))
+		goto send_rsp;
+
+	hci_req_init(&req, hdev);
+
+	if (!cp->privacy && bacmp(&hdev->static_addr, BDADDR_ANY))
+		hci_req_add(&req, HCI_OP_LE_SET_RANDOM_ADDR, 6,
+			    &hdev->static_addr);
+
+	if (test_bit(HCI_ADVERTISING, &hdev->dev_flags) &&
+	    hci_conn_num(hdev, LE_LINK) == 0) {
+		disable_advertising(&req);
+		enable_advertising(&req);
+	}
+
+	hci_req_run(&req, NULL);
+
+send_rsp:
 	err = send_settings_rsp(sk, MGMT_OP_SET_PRIVACY, hdev);
 	if (err < 0)
 		goto unlock;
-- 
1.8.5.3

Re: [PATCH 15/15] Bluetooth: Add full support for updating privacy state

[Marcel Holtmann]

Hi Johan,

> There are several things that need updating in addition to the
> HCI_PRIVACY flag when Set Privacy is called. This includes toggling the
> HCI_RPA_EXPIRED flag, setting the own_address_type variable, as well as
> restarting advertising if necessary.
> 
> This patch adds these missing pieces to the set_privacy function.

none of these should be needed if we use hdev->own_address_type as identity address type. And if we ensure that we update even the static address before each command that needs it.

Regards

Marcel

Re: [PATCH 00/15] Bluetooth: LE Privacy support

[Marcel Holtmann]

Hi Johan,

> This set of patches implements support for the Set Privacy mgmt command,
> i.e. the ability to have a local IRK and use it to generate a local RPA.
> 
> I'm not expecting most of the patches to have too many points of
> discussion, except for the last one: we might want to consider adding
> some restrictions to when Set Privacy can be called in order to make the
> command implementation simpler. We could e.g. reject the command
> whenever advertising is enabled or possibly even when the adapter is
> powered on.
> 
> Johan
> 
> ----------------------------------------------------------------
> Johan Hedberg (15):
>      Bluetooth: Add helper variables to smp_distribute_keys()
>      Bluetooth: Add initial code for distributing local IRK
>      Bluetooth: Move enable/disable_advertising higher up in mgmt.c
>      Bluetooth: Add mgmt defines for privacy
>      Bluetooth: Add Privacy lag to mgmt supported/current settings
>      Bluetooth: Add skeleton for Set Privacy command
>      Bluetooth: Add SMP function for generating RPAs
>      Bluetooth: Don't write static address when privacy is enabled
>      Bluetooth: Fix hdev->own_addr_type value when privacy is set
>      Bluetooth: Add mechanism for updating the local RPA
>      Bluetooth: Update local RPA if necessary when connecting LE
>      Bluetooth: Update local RPA if necessary before enabling advertising
>      Bluetooth: Update local RPA if necessary before starting LE scan
>      Bluetooth: Add debugfs entry for RPA regeneration timeout
>      Bluetooth: Add full support for updating privacy state
> 
> include/net/bluetooth/hci.h      |   2 +
> include/net/bluetooth/hci_core.h |   9 ++
> include/net/bluetooth/mgmt.h     |   8 ++
> net/bluetooth/hci_conn.c         |   3 +
> net/bluetooth/hci_core.c         |  60 ++++++++++-
> net/bluetooth/mgmt.c             | 198 +++++++++++++++++++++++++++--------
> net/bluetooth/smp.c              |  40 +++++--
> net/bluetooth/smp.h              |   1 +
> 8 files changed, 262 insertions(+), 59 deletions(-)

patches 1-5 have been applied to bluetooth-next tree.

Regards

Marcel