[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 the SMP code I've come accross
while working with it.

Johan

----------------------------------------------------------------
Johan Hedberg (6):
      Bluetooth: Fix missing NULL check for smp_chan_create() return value
      Bluetooth: Remove unnecessary SMP STK define
      Bluetooth: Remove HCI prefix from SMP LTK defines
      Bluetooth: Remove unused LTK authentication defines
      Bluetooth: Convert SMP flags into an enum
      Bluetooth: Add flexible buffer byte order swapping function

 include/net/bluetooth/hci.h |  9 ---------
 net/bluetooth/hci_core.c    |  2 +-
 net/bluetooth/hci_event.c   |  3 ++-
 net/bluetooth/mgmt.c        |  6 +++---
 net/bluetooth/smp.c         | 41 +++++++++++++++++++----------------------
 net/bluetooth/smp.h         |  7 +++++++
 6 files changed, 32 insertions(+), 36 deletions(-)

[PATCH 1/6] Bluetooth: Fix missing NULL check for smp_chan_create() return value

[johan.hedberg]

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

The smp_chan_create function may return NULL, e.g. in the case of memory
allocation failure, so we always need to check for this.

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

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index f2829a7932e2..e779e5298ebf 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -856,6 +856,8 @@ static u8 smp_cmd_security_req(struct l2cap_conn *conn, struct sk_buff *skb)
 		return 0;
 
 	smp = smp_chan_create(conn);
+	if (!smp)
+		return SMP_UNSPECIFIED;
 
 	skb_pull(skb, sizeof(*rp));
 
-- 
1.9.3

[PATCH 2/6] Bluetooth: Remove unnecessary SMP STK define

[johan.hedberg]

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

We never store the "master" type of STKs since we request encryption
directly with them so we only need one STK type (the one that's
looked-up on the slave side). Simply remove the unnecessary define and
rename the _SLAVE one to the shorter form.

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

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 16587dcd6a91..98f7520d2f57 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -349,7 +349,6 @@ enum {
 #define HCI_LK_AUTH_COMBINATION_P256	0x08
 /* The spec doesn't define types for SMP keys, the _MASTER suffix is implied */
 #define HCI_SMP_STK			0x80
-#define HCI_SMP_STK_SLAVE		0x81
 #define HCI_SMP_LTK			0x82
 #define HCI_SMP_LTK_SLAVE		0x83
 
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 640c54ec1bd2..752b0b262c88 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4241,7 +4241,7 @@ static void hci_le_ltk_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	 * distribute the keys. Later, security can be re-established
 	 * using a distributed LTK.
 	 */
-	if (ltk->type == HCI_SMP_STK_SLAVE) {
+	if (ltk->type == HCI_SMP_STK) {
 		list_del(&ltk->list);
 		kfree(ltk);
 	}
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index e779e5298ebf..fe27866eee12 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -562,7 +562,7 @@ static u8 smp_random(struct smp_chan *smp)
 			auth = 0;
 
 		hci_add_ltk(hcon->hdev, &hcon->dst, hcon->dst_type,
-			    HCI_SMP_STK_SLAVE, auth, stk, smp->enc_key_size,
+			    HCI_SMP_STK, auth, stk, smp->enc_key_size,
 			    ediv, rand);
 	}
 
-- 
1.9.3

[PATCH v2 2/6] Bluetooth: Remove unnecessary SMP STK define

[johan.hedberg]

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

We never store the "master" type of STKs since we request encryption
directly with them so we only need one STK type (the one that's
looked-up on the slave side). Simply remove the unnecessary define and
rename the _SLAVE one to the shorter form.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
v2: Add clarifying comment when adding slave STK

 include/net/bluetooth/hci.h | 1 -
 net/bluetooth/hci_event.c   | 2 +-
 net/bluetooth/smp.c         | 6 +++++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 16587dcd6a91..98f7520d2f57 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -349,7 +349,6 @@ enum {
 #define HCI_LK_AUTH_COMBINATION_P256	0x08
 /* The spec doesn't define types for SMP keys, the _MASTER suffix is implied */
 #define HCI_SMP_STK			0x80
-#define HCI_SMP_STK_SLAVE		0x81
 #define HCI_SMP_LTK			0x82
 #define HCI_SMP_LTK_SLAVE		0x83
 
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 640c54ec1bd2..752b0b262c88 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4241,7 +4241,7 @@ static void hci_le_ltk_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	 * distribute the keys. Later, security can be re-established
 	 * using a distributed LTK.
 	 */
-	if (ltk->type == HCI_SMP_STK_SLAVE) {
+	if (ltk->type == HCI_SMP_STK) {
 		list_del(&ltk->list);
 		kfree(ltk);
 	}
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index e779e5298ebf..4fb9f9aa0cea 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -561,8 +561,12 @@ static u8 smp_random(struct smp_chan *smp)
 		else
 			auth = 0;
 
+		/* Even though there's no _SLAVE suffix this is the
+		 * slave STK we're adding for later lookup (the master
+		 * STK never needs to be stored).
+		 */
 		hci_add_ltk(hcon->hdev, &hcon->dst, hcon->dst_type,
-			    HCI_SMP_STK_SLAVE, auth, stk, smp->enc_key_size,
+			    HCI_SMP_STK, auth, stk, smp->enc_key_size,
 			    ediv, rand);
 	}
 
-- 
1.9.3

[PATCH 3/6] Bluetooth: Remove HCI prefix from SMP LTK defines

[johan.hedberg]

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

The LTK type has really nothing to do with HCI so it makes more sense to
have these in smp.h than hci.h. This patch moves the defines to smp.h
and removes the HCI_ prefix in the same go.

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

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 98f7520d2f57..6ec5b3bd1f67 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -347,10 +347,6 @@ enum {
 #define HCI_LK_CHANGED_COMBINATION	0x06
 #define HCI_LK_UNAUTH_COMBINATION_P256	0x07
 #define HCI_LK_AUTH_COMBINATION_P256	0x08
-/* The spec doesn't define types for SMP keys, the _MASTER suffix is implied */
-#define HCI_SMP_STK			0x80
-#define HCI_SMP_LTK			0x82
-#define HCI_SMP_LTK_SLAVE		0x83
 
 /* Long Term Key types */
 #define HCI_LTK_UNAUTH			0x00
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 0a43cce9a914..eeb124dc4c2f 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2974,7 +2974,7 @@ static bool hci_persistent_key(struct hci_dev *hdev, struct hci_conn *conn,
 
 static bool ltk_type_master(u8 type)
 {
-	if (type == HCI_SMP_STK || type == HCI_SMP_LTK)
+	if (type == SMP_STK || type == SMP_LTK)
 		return true;
 
 	return false;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 752b0b262c88..a94f9ad6402e 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -32,6 +32,7 @@
 
 #include "a2mp.h"
 #include "amp.h"
+#include "smp.h"
 
 /* Handle HCI Event packets */
 
@@ -4241,7 +4242,7 @@ static void hci_le_ltk_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	 * distribute the keys. Later, security can be re-established
 	 * using a distributed LTK.
 	 */
-	if (ltk->type == HCI_SMP_STK) {
+	if (ltk->type == SMP_STK) {
 		list_del(&ltk->list);
 		kfree(ltk);
 	}
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index af8e0a6243b7..99eb845865ef 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4550,9 +4550,9 @@ static int load_long_term_keys(struct sock *sk, struct hci_dev *hdev,
 			addr_type = ADDR_LE_DEV_RANDOM;
 
 		if (key->master)
-			type = HCI_SMP_LTK;
+			type = SMP_LTK;
 		else
-			type = HCI_SMP_LTK_SLAVE;
+			type = SMP_LTK_SLAVE;
 
 		switch (key->type) {
 		case MGMT_LTK_UNAUTHENTICATED:
@@ -5279,7 +5279,7 @@ void mgmt_new_ltk(struct hci_dev *hdev, struct smp_ltk *key, bool persistent)
 	ev.key.ediv = key->ediv;
 	ev.key.rand = key->rand;
 
-	if (key->type == HCI_SMP_LTK)
+	if (key->type == SMP_LTK)
 		ev.key.master = 1;
 
 	memcpy(ev.key.val, key->val, sizeof(key->val));
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index fe27866eee12..1d03949fe928 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -562,8 +562,7 @@ static u8 smp_random(struct smp_chan *smp)
 			auth = 0;
 
 		hci_add_ltk(hcon->hdev, &hcon->dst, hcon->dst_type,
-			    HCI_SMP_STK, auth, stk, smp->enc_key_size,
-			    ediv, rand);
+			    SMP_STK, auth, stk, smp->enc_key_size, ediv, rand);
 	}
 
 	return 0;
@@ -991,7 +990,7 @@ static int smp_cmd_master_ident(struct l2cap_conn *conn, struct sk_buff *skb)
 
 	hci_dev_lock(hdev);
 	authenticated = (hcon->sec_level == BT_SECURITY_HIGH);
-	ltk = hci_add_ltk(hdev, &hcon->dst, hcon->dst_type, HCI_SMP_LTK,
+	ltk = hci_add_ltk(hdev, &hcon->dst, hcon->dst_type, SMP_LTK,
 			  authenticated, smp->tk, smp->enc_key_size,
 			  rp->ediv, rp->rand);
 	smp->ltk = ltk;
@@ -1307,7 +1306,7 @@ int smp_distribute_keys(struct l2cap_conn *conn)
 
 		authenticated = hcon->sec_level == BT_SECURITY_HIGH;
 		ltk = hci_add_ltk(hdev, &hcon->dst, hcon->dst_type,
-				  HCI_SMP_LTK_SLAVE, authenticated, enc.ltk,
+				  SMP_LTK_SLAVE, authenticated, enc.ltk,
 				  smp->enc_key_size, ediv, rand);
 		smp->slave_ltk = ltk;
 
diff --git a/net/bluetooth/smp.h b/net/bluetooth/smp.h
index 5a8dc36460a1..796f4f45f92f 100644
--- a/net/bluetooth/smp.h
+++ b/net/bluetooth/smp.h
@@ -116,6 +116,13 @@ struct smp_cmd_security_req {
 #define SMP_MIN_ENC_KEY_SIZE		7
 #define SMP_MAX_ENC_KEY_SIZE		16
 
+/* LTK types used in internal storage (struct smp_ltk) */
+enum {
+	SMP_STK,
+	SMP_LTK,
+	SMP_LTK_SLAVE,
+};
+
 /* SMP Commands */
 bool smp_sufficient_security(struct hci_conn *hcon, u8 sec_level);
 int smp_conn_security(struct hci_conn *hcon, __u8 sec_level);
-- 
1.9.3

[PATCH 4/6] Bluetooth: Remove unused LTK authentication defines

[johan.hedberg]

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

These defines were probably put in to track authenticated vs
unauthenticated LTKs, however since the LTK struct has a separate
boolean authenticated member these were never used.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 6ec5b3bd1f67..fe584d78008d 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -348,10 +348,6 @@ enum {
 #define HCI_LK_UNAUTH_COMBINATION_P256	0x07
 #define HCI_LK_AUTH_COMBINATION_P256	0x08
 
-/* Long Term Key types */
-#define HCI_LTK_UNAUTH			0x00
-#define HCI_LTK_AUTH			0x01
-
 /* ---- HCI Error Codes ---- */
 #define HCI_ERROR_AUTH_FAILURE		0x05
 #define HCI_ERROR_MEMORY_EXCEEDED	0x07
-- 
1.9.3

[PATCH 5/6] Bluetooth: Convert SMP flags into an enum

[johan.hedberg]

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

There's no reason to have explicit values for these flags. Convert them
to an enum to be consistent with other similar flags.

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

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 1d03949fe928..513a92cb0356 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -35,11 +35,13 @@
 
 #define AUTH_REQ_MASK   0x07
 
-#define SMP_FLAG_TK_VALID	1
-#define SMP_FLAG_CFM_PENDING	2
-#define SMP_FLAG_MITM_AUTH	3
-#define SMP_FLAG_COMPLETE	4
-#define SMP_FLAG_INITIATOR	5
+enum {
+	SMP_FLAG_TK_VALID,
+	SMP_FLAG_CFM_PENDING,
+	SMP_FLAG_MITM_AUTH,
+	SMP_FLAG_COMPLETE,
+	SMP_FLAG_INITIATOR,
+};
 
 struct smp_chan {
 	struct l2cap_conn *conn;
-- 
1.9.3

[PATCH 6/6] Bluetooth: Add flexible buffer byte order swapping function

[johan.hedberg]

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

Since the SMP code needs to swap ordering of variable length buffers add
a convenience function that can be used for any length buffer.

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

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 513a92cb0356..142b6083a18a 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -64,18 +64,12 @@ struct smp_chan {
 	unsigned long	flags;
 };
 
-static inline void swap128(const u8 src[16], u8 dst[16])
+static inline void swap_buf(const u8 *src, u8 *dst, size_t len)
 {
-	int i;
-	for (i = 0; i < 16; i++)
-		dst[15 - i] = src[i];
-}
+	size_t i;
 
-static inline void swap56(const u8 src[7], u8 dst[7])
-{
-	int i;
-	for (i = 0; i < 7; i++)
-		dst[6 - i] = src[i];
+	for (i = 0; i < len; i++)
+		dst[len - 1 - i] = src[i];
 }
 
 static int smp_e(struct crypto_blkcipher *tfm, const u8 *k, u8 *r)
@@ -94,7 +88,7 @@ static int smp_e(struct crypto_blkcipher *tfm, const u8 *k, u8 *r)
 	desc.flags = 0;
 
 	/* The most significant octet of key corresponds to k[0] */
-	swap128(k, tmp);
+	swap_buf(k, tmp, 16);
 
 	err = crypto_blkcipher_setkey(tfm, tmp, 16);
 	if (err) {
@@ -103,7 +97,7 @@ static int smp_e(struct crypto_blkcipher *tfm, const u8 *k, u8 *r)
 	}
 
 	/* Most significant octet of plaintextData corresponds to data[0] */
-	swap128(r, data);
+	swap_buf(r, data, 16);
 
 	sg_init_one(&sg, data, 16);
 
@@ -112,7 +106,7 @@ static int smp_e(struct crypto_blkcipher *tfm, const u8 *k, u8 *r)
 		BT_ERR("Encrypt data error %d", err);
 
 	/* Most significant octet of encryptedData corresponds to data[0] */
-	swap128(data, r);
+	swap_buf(data, r, 16);
 
 	return err;
 }
-- 
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 the SMP code I've come accross
> while working with it.
> 
> Johan
> 
> ----------------------------------------------------------------
> Johan Hedberg (6):
>      Bluetooth: Fix missing NULL check for smp_chan_create() return value
>      Bluetooth: Remove unnecessary SMP STK define
>      Bluetooth: Remove HCI prefix from SMP LTK defines
>      Bluetooth: Remove unused LTK authentication defines
>      Bluetooth: Convert SMP flags into an enum
>      Bluetooth: Add flexible buffer byte order swapping function
> 
> include/net/bluetooth/hci.h |  9 ---------
> net/bluetooth/hci_core.c    |  2 +-
> net/bluetooth/hci_event.c   |  3 ++-
> net/bluetooth/mgmt.c        |  6 +++---
> net/bluetooth/smp.c         | 41 +++++++++++++++++++----------------------
> net/bluetooth/smp.h         |  7 +++++++
> 6 files changed, 32 insertions(+), 36 deletions(-)

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

Regards

Marcel

[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(-)

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