[PATCH 0/6] Bluetooth: Various SMP cleanups & fixes

[PATCH 0/6] Bluetooth: Various SMP cleanups & fixes

[johan.hedberg]

Hi,

Here's a set of cleanups and fixes for SMP. I'd consider all of them as
bluetooth-next material, but it could be debated whether the first patch
should go to bluetooth.git first (I don't have any obvious way of
exploiting it so I don't consider it very critical).

Johan

----------------------------------------------------------------
Johan Hedberg (6):
      Bluetooth: Fix missing hdev locking in smp_cmd_ident_addr_info
      Bluetooth: Add dedicated AES instance for each SMP context
      Bluetooth: Update SMP crypto functions to take the SMP context
      Bluetooth: Remove unnecessary hci_dev_unlock for smp_user_confirm_reply
      Bluetooth: Fix missing check for SMP session in smp_user_confirm_reply
      Bluetooth: Remove unnecessary hcon->smp_conn variable

 include/net/bluetooth/hci_core.h |  1 -
 net/bluetooth/mgmt.c             |  7 ----
 net/bluetooth/smp.c              | 73 ++++++++++++++++++++------------------
 3 files changed, 38 insertions(+), 43 deletions(-)

[PATCH 1/6] Bluetooth: Fix missing hdev locking in smp_cmd_ident_addr_info

[johan.hedberg]

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

The hdev lock must be held before calling into smp_distribute_keys. Also
things such as hci_add_irk() require the lock. This patch fixes the
issue by adding the necessary locking into the smp_cmd_ident_addr_info
function.

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

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 976fce2315fd..a38941593e8b 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -1076,6 +1076,8 @@ static int smp_cmd_ident_addr_info(struct l2cap_conn *conn,
 
 	skb_pull(skb, sizeof(*info));
 
+	hci_dev_lock(hcon->hdev);
+
 	/* Strictly speaking the Core Specification (4.1) allows sending
 	 * an empty address which would force us to rely on just the IRK
 	 * as "identity information". However, since such
@@ -1085,8 +1087,7 @@ static int smp_cmd_ident_addr_info(struct l2cap_conn *conn,
 	 */
 	if (!bacmp(&info->bdaddr, BDADDR_ANY)) {
 		BT_ERR("Ignoring IRK with no identity address");
-		smp_distribute_keys(conn);
-		return 0;
+		goto distribute;
 	}
 
 	bacpy(&smp->id_addr, &info->bdaddr);
@@ -1100,8 +1101,11 @@ static int smp_cmd_ident_addr_info(struct l2cap_conn *conn,
 	smp->remote_irk = hci_add_irk(conn->hcon->hdev, &smp->id_addr,
 				      smp->id_addr_type, smp->irk, &rpa);
 
+distribute:
 	smp_distribute_keys(conn);
 
+	hci_dev_unlock(hcon->hdev);
+
 	return 0;
 }
 
-- 
1.9.3

[PATCH 2/6] Bluetooth: Add dedicated AES instance for each SMP context

[johan.hedberg]

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

Many places have to be extra careful to not hold the hdev lock when
calling into the SMP code. This is because the SMP crypto functions use
the crypto handle that's part of the hci_dev struct. Giving the SMP
context its own handle helps simplifying the locking logic and removes
the risk for deadlocks.

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

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index a38941593e8b..39ca9616d2de 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -62,6 +62,8 @@ struct smp_chan {
 	struct smp_ltk	*slave_ltk;
 	struct smp_irk	*remote_irk;
 	unsigned long	flags;
+
+	struct crypto_blkcipher	*tfm_aes;
 };
 
 static inline void swap_buf(const u8 *src, u8 *dst, size_t len)
@@ -583,6 +585,13 @@ static struct smp_chan *smp_chan_create(struct l2cap_conn *conn)
 	if (!smp)
 		return NULL;
 
+	smp->tfm_aes = crypto_alloc_blkcipher("ecb(aes)", 0, CRYPTO_ALG_ASYNC);
+	if (IS_ERR(smp->tfm_aes)) {
+		BT_ERR("Unable to create ECB crypto context");
+		kfree(smp);
+		return NULL;
+	}
+
 	smp->conn = conn;
 	conn->smp_chan = smp;
 	conn->hcon->smp_conn = conn;
@@ -605,6 +614,8 @@ void smp_chan_destroy(struct l2cap_conn *conn)
 	kfree(smp->csrk);
 	kfree(smp->slave_csrk);
 
+	crypto_free_blkcipher(smp->tfm_aes);
+
 	/* If pairing failed clean up any keys we might have */
 	if (!complete) {
 		if (smp->ltk) {
-- 
1.9.3

[PATCH 3/6] Bluetooth: Update SMP crypto functions to take the SMP context

[johan.hedberg]

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

Passing the full SMP context instead of just the crypto context lets us
use the crypto handle from the context which in turn removes the need to
lock the hci_dev. Passing the SMP context instead of just the crypto
handle allows a bit more detailed logging which is helpful in
multi-adapter scenarios.

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

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 39ca9616d2de..2566a3e43bb5 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -172,13 +172,16 @@ int smp_generate_rpa(struct crypto_blkcipher *tfm, u8 irk[16], bdaddr_t *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])
+static int smp_c1(struct smp_chan *smp, 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])
 {
+	struct hci_dev *hdev = smp->conn->hcon->hdev;
 	u8 p1[16], p2[16];
 	int err;
 
+	BT_DBG("%s", hdev->name);
+
 	memset(p1, 0, 16);
 
 	/* p1 = pres || preq || _rat || _iat */
@@ -196,7 +199,7 @@ static int smp_c1(struct crypto_blkcipher *tfm, u8 k[16], u8 r[16],
 	u128_xor((u128 *) res, (u128 *) r, (u128 *) p1);
 
 	/* res = e(k, res) */
-	err = smp_e(tfm, k, res);
+	err = smp_e(smp->tfm_aes, k, res);
 	if (err) {
 		BT_ERR("Encrypt data error");
 		return err;
@@ -206,23 +209,26 @@ static int smp_c1(struct crypto_blkcipher *tfm, u8 k[16], u8 r[16],
 	u128_xor((u128 *) res, (u128 *) res, (u128 *) p2);
 
 	/* res = e(k, res) */
-	err = smp_e(tfm, k, res);
+	err = smp_e(smp->tfm_aes, k, res);
 	if (err)
 		BT_ERR("Encrypt data error");
 
 	return err;
 }
 
-static int smp_s1(struct crypto_blkcipher *tfm, u8 k[16], u8 r1[16],
-		  u8 r2[16], u8 _r[16])
+static int smp_s1(struct smp_chan *smp, u8 k[16], u8 r1[16], u8 r2[16],
+		  u8 _r[16])
 {
+	struct hci_dev *hdev = smp->conn->hcon->hdev;
 	int err;
 
+	BT_DBG("%s", hdev->name);
+
 	/* Just least significant octets from r1 and r2 are considered */
 	memcpy(_r, r2, 8);
 	memcpy(_r + 8, r1, 8);
 
-	err = smp_e(tfm, k, _r);
+	err = smp_e(smp->tfm_aes, k, _r);
 	if (err)
 		BT_ERR("Encrypt data error");
 
@@ -475,23 +481,15 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth,
 static u8 smp_confirm(struct smp_chan *smp)
 {
 	struct l2cap_conn *conn = smp->conn;
-	struct hci_dev *hdev = conn->hcon->hdev;
-	struct crypto_blkcipher *tfm = hdev->tfm_aes;
 	struct smp_cmd_pairing_confirm cp;
 	int ret;
 
 	BT_DBG("conn %p", conn);
 
-	/* Prevent mutual access to hdev->tfm_aes */
-	hci_dev_lock(hdev);
-
-	ret = smp_c1(tfm, smp->tk, smp->prnd, smp->preq, smp->prsp,
+	ret = smp_c1(smp, 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,
 		     cp.confirm_val);
-
-	hci_dev_unlock(hdev);
-
 	if (ret)
 		return SMP_UNSPECIFIED;
 
@@ -506,25 +504,17 @@ static u8 smp_random(struct smp_chan *smp)
 {
 	struct l2cap_conn *conn = smp->conn;
 	struct hci_conn *hcon = conn->hcon;
-	struct hci_dev *hdev = hcon->hdev;
-	struct crypto_blkcipher *tfm = hdev->tfm_aes;
 	u8 confirm[16];
 	int ret;
 
-	if (IS_ERR_OR_NULL(tfm))
+	if (IS_ERR_OR_NULL(smp->tfm_aes))
 		return SMP_UNSPECIFIED;
 
 	BT_DBG("conn %p %s", conn, conn->hcon->out ? "master" : "slave");
 
-	/* Prevent mutual access to hdev->tfm_aes */
-	hci_dev_lock(hdev);
-
-	ret = smp_c1(tfm, smp->tk, smp->rrnd, smp->preq, smp->prsp,
+	ret = smp_c1(smp, smp->tk, smp->rrnd, smp->preq, smp->prsp,
 		     hcon->init_addr_type, &hcon->init_addr,
 		     hcon->resp_addr_type, &hcon->resp_addr, confirm);
-
-	hci_dev_unlock(hdev);
-
 	if (ret)
 		return SMP_UNSPECIFIED;
 
@@ -538,7 +528,7 @@ static u8 smp_random(struct smp_chan *smp)
 		__le64 rand = 0;
 		__le16 ediv = 0;
 
-		smp_s1(tfm, smp->tk, smp->rrnd, smp->prnd, stk);
+		smp_s1(smp, smp->tk, smp->rrnd, smp->prnd, stk);
 
 		memset(stk + smp->enc_key_size, 0,
 		       SMP_MAX_ENC_KEY_SIZE - smp->enc_key_size);
@@ -556,7 +546,7 @@ static u8 smp_random(struct smp_chan *smp)
 		smp_send_cmd(conn, SMP_CMD_PAIRING_RANDOM, sizeof(smp->prnd),
 			     smp->prnd);
 
-		smp_s1(tfm, smp->tk, smp->prnd, smp->rrnd, stk);
+		smp_s1(smp, smp->tk, smp->prnd, smp->rrnd, stk);
 
 		memset(stk + smp->enc_key_size, 0,
 		       SMP_MAX_ENC_KEY_SIZE - smp->enc_key_size);
-- 
1.9.3

[PATCH 4/6] Bluetooth: Remove unnecessary hci_dev_unlock for smp_user_confirm_reply

[johan.hedberg]

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

Now that the SMP context has it's own crypto handle it doesn't need to
lock the hci_dev anymore for most operations. This means that it is safe
to call smp_user_confirm_reply with the lock already held.

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index f75a090cd7e4..d542f8af6a5d 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3052,14 +3052,7 @@ static int user_pairing_resp(struct sock *sk, struct hci_dev *hdev,
 	}
 
 	if (addr->type == BDADDR_LE_PUBLIC || addr->type == BDADDR_LE_RANDOM) {
-		/* Continue with pairing via SMP. The hdev lock must be
-		 * released as SMP may try to recquire it for crypto
-		 * purposes.
-		 */
-		hci_dev_unlock(hdev);
 		err = smp_user_confirm_reply(conn, mgmt_op, passkey);
-		hci_dev_lock(hdev);
-
 		if (!err)
 			err = cmd_complete(sk, hdev->id, mgmt_op,
 					   MGMT_STATUS_SUCCESS, addr,
-- 
1.9.3

[PATCH 5/6] Bluetooth: Fix missing check for SMP session in smp_user_confirm_reply

[johan.hedberg]

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

The smp_user_confirm_reply() function is called whenever user space
sends a user confirmation reply mgmt command. In case of a misbehaving
user space, or if the SMP session was removed by the time the command
comes it is important that we return an appropriate error and do not try
to access the non-existent SMP context. This patch adds the appropriate
check for the HCI_CONN_LE_SMP_PEND flag before proceeding further.

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

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 2566a3e43bb5..641ce8b69d2a 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -638,7 +638,7 @@ int smp_user_confirm_reply(struct hci_conn *hcon, u16 mgmt_op, __le32 passkey)
 
 	BT_DBG("");
 
-	if (!conn)
+	if (!conn || !test_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags))
 		return -ENOTCONN;
 
 	smp = conn->smp_chan;
-- 
1.9.3

[PATCH 6/6] Bluetooth: Remove unnecessary hcon->smp_conn variable

[johan.hedberg]

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

The smp_conn member of struct hci_conn was simply a pointer to the
l2cap_conn object. Since we already have hcon->l2cap_data that points to
the same thing there's no need to have this second variable. This patch
removes it and changes the single place that was using it to use
hcon->l2cap_data instead.

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

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index dda7f00c07c5..ca2a99807615 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -409,7 +409,6 @@ struct hci_conn {
 	struct hci_dev	*hdev;
 	void		*l2cap_data;
 	void		*sco_data;
-	void		*smp_conn;
 	struct amp_mgr	*amp_mgr;
 
 	struct hci_conn	*link;
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 641ce8b69d2a..414c5151aa46 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -584,7 +584,6 @@ static struct smp_chan *smp_chan_create(struct l2cap_conn *conn)
 
 	smp->conn = conn;
 	conn->smp_chan = smp;
-	conn->hcon->smp_conn = conn;
 
 	hci_conn_hold(conn->hcon);
 
@@ -626,13 +625,12 @@ void smp_chan_destroy(struct l2cap_conn *conn)
 
 	kfree(smp);
 	conn->smp_chan = NULL;
-	conn->hcon->smp_conn = NULL;
 	hci_conn_drop(conn->hcon);
 }
 
 int smp_user_confirm_reply(struct hci_conn *hcon, u16 mgmt_op, __le32 passkey)
 {
-	struct l2cap_conn *conn = hcon->smp_conn;
+	struct l2cap_conn *conn = hcon->l2cap_data;
 	struct smp_chan *smp;
 	u32 value;
 
-- 
1.9.3

Re: [PATCH 0/6] Bluetooth: Various SMP cleanups & fixes

[Marcel Holtmann]

Hi Johan,

> Here's a set of cleanups and fixes for SMP. I'd consider all of them as
> bluetooth-next material, but it could be debated whether the first patch
> should go to bluetooth.git first (I don't have any obvious way of
> exploiting it so I don't consider it very critical).
> 
> Johan
> 
> ----------------------------------------------------------------
> Johan Hedberg (6):
>      Bluetooth: Fix missing hdev locking in smp_cmd_ident_addr_info
>      Bluetooth: Add dedicated AES instance for each SMP context
>      Bluetooth: Update SMP crypto functions to take the SMP context
>      Bluetooth: Remove unnecessary hci_dev_unlock for smp_user_confirm_reply
>      Bluetooth: Fix missing check for SMP session in smp_user_confirm_reply
>      Bluetooth: Remove unnecessary hcon->smp_conn variable
> 
> include/net/bluetooth/hci_core.h |  1 -
> net/bluetooth/mgmt.c             |  7 ----
> net/bluetooth/smp.c              | 73 ++++++++++++++++++++------------------
> 3 files changed, 38 insertions(+), 43 deletions(-)

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

And even patch 1/6 is not something I would consider for 3.16-rcX unless we see real bugs about it. If anyone stumbles over it, we promote it to -stable once 3.16 has been released.

Regards

Marcel