[PATCH 0/3] Bluetooth: LE CoC fixes

[PATCH 0/3] Bluetooth: LE CoC fixes

[johan.hedberg]

Hi,

This patch set contains various fixes for LE CoC. The biggest functional
change is in the last patch proper reaction to the insufficient
authentication/encryption connection responses is implemented.

Johan

----------------------------------------------------------------
Johan Hedberg (3):
      Bluetooth: Fix sending incorrect LE CoC PDU in BT_CONNECT2 state
      Bluetooth: Add key preference parameter to smp_sufficient_security
      Bluetooth: Trigger SMP for the appropriate LE CoC errors

 net/bluetooth/l2cap_core.c | 39 ++++++++++++++++++++++++++++++++++++---
 net/bluetooth/smp.c        |  9 +++++----
 net/bluetooth/smp.h        |  8 +++++++-
 3 files changed, 48 insertions(+), 8 deletions(-)

[PATCH 1/3] Bluetooth: Fix sending incorrect LE CoC PDU in BT_CONNECT2 state

[johan.hedberg]

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

For LE CoC L2CAP servers we don't do security level elevation during the
BT_CONNECT2 state (instead LE CoC simply sends an immediate error
response if the security level isn't high enough). Therefore if we get a
security level change while an LE CoC channel is in the BT_CONNECT2
state we should simply do nothing.

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

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index fc15174c612c..53af3d98a533 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -7329,7 +7329,8 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 				l2cap_start_connection(chan);
 			else
 				__set_chan_timer(chan, L2CAP_DISC_TIMEOUT);
-		} else if (chan->state == BT_CONNECT2) {
+		} else if (chan->state == BT_CONNECT2 &&
+			   chan->mode != L2CAP_MODE_LE_FLOWCTL) {
 			struct l2cap_conn_rsp rsp;
 			__u16 res, stat;
 
-- 
2.1.0

[PATCH 2/3] Bluetooth: Add key preference parameter to smp_sufficient_security

[johan.hedberg]

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

So far smp_sufficient_security() has returned false if we're encrypted
with an STK but do have an LTK available. However, for the sake of LE
CoC servers we do want to let the incoming connection through even
though we're only encrypted with the STK.

This patch adds a key preference parameter to smp_sufficient_security()
with two possible values (enum used instead of bool for readability).

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/l2cap_core.c | 3 ++-
 net/bluetooth/smp.c        | 9 +++++----
 net/bluetooth/smp.h        | 8 +++++++-
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 53af3d98a533..f396d50db73e 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5388,7 +5388,8 @@ static int l2cap_le_connect_req(struct l2cap_conn *conn,
 	mutex_lock(&conn->chan_lock);
 	l2cap_chan_lock(pchan);
 
-	if (!smp_sufficient_security(conn->hcon, pchan->sec_level)) {
+	if (!smp_sufficient_security(conn->hcon, pchan->sec_level,
+				     SMP_ALLOW_STK)) {
 		result = L2CAP_CR_AUTHENTICATION;
 		chan = NULL;
 		goto response_unlock;
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 3ebf65b50881..1a14d101b213 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -1126,7 +1126,7 @@ static bool smp_ltk_encrypt(struct l2cap_conn *conn, u8 sec_level)
 	return true;
 }
 
-bool smp_sufficient_security(struct hci_conn *hcon, u8 sec_level)
+bool smp_sufficient_security(struct hci_conn *hcon, u8 sec_level, int key_pref)
 {
 	if (sec_level == BT_SECURITY_LOW)
 		return true;
@@ -1137,7 +1137,8 @@ bool smp_sufficient_security(struct hci_conn *hcon, u8 sec_level)
 	 * security. Only exception is if we don't have an LTK (e.g.
 	 * because of key distribution bits).
 	 */
-	if (test_bit(HCI_CONN_STK_ENCRYPT, &hcon->flags) &&
+	if (key_pref == SMP_USE_LTK &&
+	    test_bit(HCI_CONN_STK_ENCRYPT, &hcon->flags) &&
 	    hci_find_ltk_by_addr(hcon->hdev, &hcon->dst, hcon->dst_type,
 				 hcon->role))
 		return false;
@@ -1171,7 +1172,7 @@ static u8 smp_cmd_security_req(struct l2cap_conn *conn, struct sk_buff *skb)
 	else
 		sec_level = authreq_to_seclevel(auth);
 
-	if (smp_sufficient_security(hcon, sec_level))
+	if (smp_sufficient_security(hcon, sec_level, SMP_USE_LTK))
 		return 0;
 
 	if (sec_level > hcon->pending_sec_level)
@@ -1221,7 +1222,7 @@ int smp_conn_security(struct hci_conn *hcon, __u8 sec_level)
 	if (!test_bit(HCI_LE_ENABLED, &hcon->hdev->dev_flags))
 		return 1;
 
-	if (smp_sufficient_security(hcon, sec_level))
+	if (smp_sufficient_security(hcon, sec_level, SMP_USE_LTK))
 		return 1;
 
 	if (sec_level > hcon->pending_sec_level)
diff --git a/net/bluetooth/smp.h b/net/bluetooth/smp.h
index 86a683a8b491..8ea14c20832f 100644
--- a/net/bluetooth/smp.h
+++ b/net/bluetooth/smp.h
@@ -133,8 +133,14 @@ static inline u8 smp_ltk_sec_level(struct smp_ltk *key)
 	return BT_SECURITY_MEDIUM;
 }
 
+/* Key preferences for smp_sufficient security */
+enum {
+	SMP_ALLOW_STK,
+	SMP_USE_LTK,
+};
+
 /* SMP Commands */
-bool smp_sufficient_security(struct hci_conn *hcon, u8 sec_level);
+bool smp_sufficient_security(struct hci_conn *hcon, u8 sec_level, int key_pref);
 int smp_conn_security(struct hci_conn *hcon, __u8 sec_level);
 int smp_user_confirm_reply(struct hci_conn *conn, u16 mgmt_op, __le32 passkey);
 
-- 
2.1.0

[PATCH 3/3] Bluetooth: Trigger SMP for the appropriate LE CoC errors

[johan.hedberg]

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

The insufficient authentication/encryption errors indicate to the L2CAP
client that it should try to elevate the security level. Since there
really isn't any exception to this rule it makes sense to fully handle
it on the kernel side instead of pushing the responsibility to user
space.

This patch adds special handling of these two error codes and calls
smp_conn_security() with the elevated security level if necessary.

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

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index f396d50db73e..35dc9d52d892 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5215,9 +5215,10 @@ static int l2cap_le_connect_rsp(struct l2cap_conn *conn,
 				u8 *data)
 {
 	struct l2cap_le_conn_rsp *rsp = (struct l2cap_le_conn_rsp *) data;
+	struct hci_conn *hcon = conn->hcon;
 	u16 dcid, mtu, mps, credits, result;
 	struct l2cap_chan *chan;
-	int err;
+	int err, sec_level;
 
 	if (cmd_len < sizeof(*rsp))
 		return -EPROTO;
@@ -5256,6 +5257,36 @@ static int l2cap_le_connect_rsp(struct l2cap_conn *conn,
 		l2cap_chan_ready(chan);
 		break;
 
+	case L2CAP_CR_AUTHENTICATION:
+	case L2CAP_CR_ENCRYPTION:
+		/* If we already have MITM protection we can't do
+		 * anything.
+		 */
+		if (hcon->sec_level > BT_SECURITY_MEDIUM) {
+			l2cap_chan_del(chan, ECONNREFUSED);
+			break;
+		}
+
+		sec_level = hcon->sec_level + 1;
+		if (chan->sec_level < sec_level)
+			chan->sec_level = sec_level;
+
+		/* We'll need to send a new Connect Request */
+		clear_bit(FLAG_LE_CONN_REQ_SENT, &chan->flags);
+
+		/* smp_conn_security() will call l2cap_chan_lock() -
+		 * never for any CoC channel but only for the SMP
+		 * channel. However, this will still trigger lockdep
+		 * warnings of recursive locking, so to avoid these
+		 * unlock the channel here instead of after the call to
+		 * smp_conn_security().
+		 */
+		sec_level = chan->sec_level;
+		l2cap_chan_unlock(chan);
+
+		smp_conn_security(hcon, sec_level);
+		goto unlock;
+
 	default:
 		l2cap_chan_del(chan, ECONNREFUSED);
 		break;
-- 
2.1.0

Re: [PATCH 3/3] Bluetooth: Trigger SMP for the appropriate LE CoC errors

[Johan Hedberg]

Hi,

On Mon, Nov 10, 2014, johan.hedberg@gmail.com wrote:
> +		/* smp_conn_security() will call l2cap_chan_lock() -
> +		 * never for any CoC channel but only for the SMP
> +		 * channel. However, this will still trigger lockdep
> +		 * warnings of recursive locking, so to avoid these
> +		 * unlock the channel here instead of after the call to
> +		 * smp_conn_security().
> +		 */
> +		sec_level = chan->sec_level;
> +		l2cap_chan_unlock(chan);
> +
> +		smp_conn_security(hcon, sec_level);
> +		goto unlock;

It's might be possible to avoid this "hack" by using nested locking
annotation, so you might want to hold off with this patch for now.

I'm right now experimenting in making the rather simple change of using
mutex_lock_nested(&chan->lock, chan->chan_type) in l2cap_chan_lock() to
classify channels into separate groups based on their type. This should
allow simultaneous CoC and SMP (fixed channel) lock acquisition without
getting (false positive) warnings from lockdep.

Johan