[PATCH 0/4] Bluetooth: SMP LTK fixes

[PATCH 0/4] Bluetooth: SMP LTK fixes

[johan.hedberg]

Hi,

Now that my LTK key distribution patch is already applied I
unfortunately uncovered several bugs due to it. The main issue is that
the kernel code was not accounting for the possibility of there being
two LTKs for the same remote device in the hdev->long_term_keys list.

There are still fixes needed on the user space side which I haven't
completely sorted out yet (to pushable form - it's more or less working
already through). Right now the storage format doesn't allow storing two
LTKs for the same device.

----------------------------------------------------------------
Johan Hedberg (4):
      Bluetooth: Fix long_term_keys debugfs output
      Bluetooth: Make LTK key type check more readable
      Bluetooth: Remove unnecessary LTK type check from hci_add_ltk
      Bluetooth: Fix differentiating stored master vs slave LTK types

 include/net/bluetooth/hci_core.h |  5 +++--
 net/bluetooth/hci_core.c         | 31 +++++++++++++++++++++----------
 net/bluetooth/hci_event.c        |  2 +-
 net/bluetooth/smp.c              |  3 ++-
 4 files changed, 27 insertions(+), 14 deletions(-)

[PATCH 1/4] Bluetooth: Fix long_term_keys debugfs output

[johan.hedberg]

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

The code was previously iterating the wrong list (and what's worse
casting entries to a type which they were not) and also missing a proper
line terminator when printing each entry. The code now also prints the
LTK type in hex for easier comparison with the kernel-defined values.

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

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 369d30750417..8094a41c9a26 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -620,9 +620,9 @@ static int long_term_keys_show(struct seq_file *f, void *ptr)
 	struct list_head *p, *n;
 
 	hci_dev_lock(hdev);
-	list_for_each_safe(p, n, &hdev->link_keys) {
+	list_for_each_safe(p, n, &hdev->long_term_keys) {
 		struct smp_ltk *ltk = list_entry(p, struct smp_ltk, list);
-		seq_printf(f, "%pMR (type %u) %u %u %u %.4x %*phN %*phN\\n",
+		seq_printf(f, "%pMR (type %u) %u 0x%02x %u %.4x %*phN %*phN\n",
 			   &ltk->bdaddr, ltk->bdaddr_type, ltk->authenticated,
 			   ltk->type, ltk->enc_size, __le16_to_cpu(ltk->ediv),
 			   8, ltk->rand, 16, ltk->val);
-- 
1.8.5.3

[PATCH 2/4] Bluetooth: Make LTK key type check more readable

[johan.hedberg]

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

Instead of magic bitwise operations simply compare with the two possible
type values that we are interested in.

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

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 8094a41c9a26..754a59079de9 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2717,7 +2717,7 @@ int hci_add_ltk(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 addr_type, u8 type,
 	if (!new_key)
 		return 0;
 
-	if (type & HCI_SMP_LTK)
+	if (type == HCI_SMP_LTK || type == HCI_SMP_LTK_SLAVE)
 		mgmt_new_ltk(hdev, key, 1);
 
 	return 0;
-- 
1.8.5.3

[PATCH 3/4] Bluetooth: Remove unnecessary LTK type check from hci_add_ltk

[johan.hedberg]

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

All callers of hci_add_ltk pass a valid value to it. There are no places
where e.g. user space, the controller or the remote peer would be able
to cause invalid values to be passed. Therefore, just remove the
potentially confusing check from the beginning of the function.

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

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 754a59079de9..180473d965f6 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2692,9 +2692,6 @@ int hci_add_ltk(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 addr_type, u8 type,
 {
 	struct smp_ltk *key, *old_key;
 
-	if (!(type & HCI_SMP_STK) && !(type & HCI_SMP_LTK))
-		return 0;
-
 	old_key = hci_find_ltk_by_addr(hdev, bdaddr, addr_type);
 	if (old_key)
 		key = old_key;
-- 
1.8.5.3

[PATCH 4/4] Bluetooth: Fix differentiating stored master vs slave LTK types

[johan.hedberg]

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

If LTK distribution happens in both directions we will have two LTKs for
the same remote device: one which is used when we're connecting as
master and another when we're connecting as slave. When looking up LTKs
from the locally stored list we shouldn't blindly return the first match
but also consider which type of key is in question. If we do not do this
we may end up selecting an incorrect encryption key for a connection.

This patch fixes the issue by always specifying to the LTK lookup
functions whether we're looking for a master or a slave key.

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

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 8d225e4ea2ce..378e2f32cfa0 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -757,12 +757,13 @@ int hci_link_keys_clear(struct hci_dev *hdev);
 struct link_key *hci_find_link_key(struct hci_dev *hdev, bdaddr_t *bdaddr);
 int hci_add_link_key(struct hci_dev *hdev, struct hci_conn *conn, int new_key,
 		     bdaddr_t *bdaddr, u8 *val, u8 type, u8 pin_len);
-struct smp_ltk *hci_find_ltk(struct hci_dev *hdev, __le16 ediv, u8 rand[8]);
+struct smp_ltk *hci_find_ltk(struct hci_dev *hdev, __le16 ediv, u8 rand[8],
+			     bool master);
 int hci_add_ltk(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 addr_type, u8 type,
 		int new_key, u8 authenticated, u8 tk[16], u8 enc_size,
 		__le16 ediv, u8 rand[8]);
 struct smp_ltk *hci_find_ltk_by_addr(struct hci_dev *hdev, bdaddr_t *bdaddr,
-				     u8 addr_type);
+				     u8 addr_type, bool master);
 int hci_remove_ltk(struct hci_dev *hdev, bdaddr_t *bdaddr);
 int hci_smp_ltks_clear(struct hci_dev *hdev);
 int hci_remove_link_key(struct hci_dev *hdev, bdaddr_t *bdaddr);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 180473d965f6..d370b432aea6 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2605,7 +2605,16 @@ static bool hci_persistent_key(struct hci_dev *hdev, struct hci_conn *conn,
 	return false;
 }
 
-struct smp_ltk *hci_find_ltk(struct hci_dev *hdev, __le16 ediv, u8 rand[8])
+static bool ltk_type_master(u8 type)
+{
+	if (type == HCI_SMP_STK || type == HCI_SMP_LTK)
+		return true;
+
+	return false;
+}
+
+struct smp_ltk *hci_find_ltk(struct hci_dev *hdev, __le16 ediv, u8 rand[8],
+			     bool master)
 {
 	struct smp_ltk *k;
 
@@ -2614,6 +2623,9 @@ struct smp_ltk *hci_find_ltk(struct hci_dev *hdev, __le16 ediv, u8 rand[8])
 		    memcmp(rand, k->rand, sizeof(k->rand)))
 			continue;
 
+		if (ltk_type_master(k->type) != master)
+			continue;
+
 		return k;
 	}
 
@@ -2621,13 +2633,14 @@ struct smp_ltk *hci_find_ltk(struct hci_dev *hdev, __le16 ediv, u8 rand[8])
 }
 
 struct smp_ltk *hci_find_ltk_by_addr(struct hci_dev *hdev, bdaddr_t *bdaddr,
-				     u8 addr_type)
+				     u8 addr_type, bool master)
 {
 	struct smp_ltk *k;
 
 	list_for_each_entry(k, &hdev->long_term_keys, list)
 		if (addr_type == k->bdaddr_type &&
-		    bacmp(bdaddr, &k->bdaddr) == 0)
+		    bacmp(bdaddr, &k->bdaddr) == 0 &&
+		    ltk_type_master(k->type) == master)
 			return k;
 
 	return NULL;
@@ -2691,8 +2704,9 @@ int hci_add_ltk(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 addr_type, u8 type,
 		ediv, u8 rand[8])
 {
 	struct smp_ltk *key, *old_key;
+	bool master = ltk_type_master(type);
 
-	old_key = hci_find_ltk_by_addr(hdev, bdaddr, addr_type);
+	old_key = hci_find_ltk_by_addr(hdev, bdaddr, addr_type, master);
 	if (old_key)
 		key = old_key;
 	else {
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 8c44bbe19add..7bb8094a3ff2 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3650,7 +3650,7 @@ static void hci_le_ltk_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	if (conn == NULL)
 		goto not_found;
 
-	ltk = hci_find_ltk(hdev, ev->ediv, ev->random);
+	ltk = hci_find_ltk(hdev, ev->ediv, ev->random, conn->out);
 	if (ltk == NULL)
 		goto not_found;
 
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 9b1167007653..efe51ccdc615 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -699,7 +699,8 @@ static u8 smp_ltk_encrypt(struct l2cap_conn *conn, u8 sec_level)
 	struct smp_ltk *key;
 	struct hci_conn *hcon = conn->hcon;
 
-	key = hci_find_ltk_by_addr(hcon->hdev, &hcon->dst, hcon->dst_type);
+	key = hci_find_ltk_by_addr(hcon->hdev, &hcon->dst, hcon->dst_type,
+				   hcon->out);
 	if (!key)
 		return 0;
 
-- 
1.8.5.3

Re: [PATCH 0/4] Bluetooth: SMP LTK fixes

[Marcel Holtmann]

Hi Johan,

> Now that my LTK key distribution patch is already applied I
> unfortunately uncovered several bugs due to it. The main issue is that
> the kernel code was not accounting for the possibility of there being
> two LTKs for the same remote device in the hdev->long_term_keys list.
> 
> There are still fixes needed on the user space side which I haven't
> completely sorted out yet (to pushable form - it's more or less working
> already through). Right now the storage format doesn't allow storing two
> LTKs for the same device.
> 
> ----------------------------------------------------------------
> Johan Hedberg (4):
>      Bluetooth: Fix long_term_keys debugfs output
>      Bluetooth: Make LTK key type check more readable
>      Bluetooth: Remove unnecessary LTK type check from hci_add_ltk
>      Bluetooth: Fix differentiating stored master vs slave LTK types
> 
> include/net/bluetooth/hci_core.h |  5 +++--
> net/bluetooth/hci_core.c         | 31 +++++++++++++++++++++----------
> net/bluetooth/hci_event.c        |  2 +-
> net/bluetooth/smp.c              |  3 ++-
> 4 files changed, 27 insertions(+), 14 deletions(-)

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

Regards

Marcel