* [PATCH 1/7] Bluetooth: Remove unnecessary checks after canceling SMP security timer
2014-09-05 10:50 [PATCH 0/7] Bluetooth: SMP fixes/improvements johan.hedberg
@ 2014-09-05 10:51 ` johan.hedberg
2014-09-05 10:51 ` [PATCH 2/7] Bluetooth: Don't take any action in smp_resume_cb if not encrypted johan.hedberg
` (5 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: johan.hedberg @ 2014-09-05 10:51 UTC (permalink / raw)
To: linux-bluetooth
From: Johan Hedberg <johan.hedberg@intel.com>
The SMP security timer used to be able to modify the SMP context state
but now days it simply calls hci_disconnect(). It is therefore
unnecessary to have extra sanity checks for the SMP context after
canceling the timer.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/smp.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 16c181181775..b8ecc7bd3e3b 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -282,8 +282,7 @@ static void smp_send_cmd(struct l2cap_conn *conn, u8 code, u16 len, void *data)
smp = chan->data;
cancel_delayed_work_sync(&smp->security_timer);
- if (test_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags))
- schedule_delayed_work(&smp->security_timer, SMP_TIMEOUT);
+ schedule_delayed_work(&smp->security_timer, SMP_TIMEOUT);
}
static __u8 authreq_to_seclevel(__u8 authreq)
@@ -375,9 +374,6 @@ static void smp_chan_destroy(struct l2cap_conn *conn)
BUG_ON(!smp);
cancel_delayed_work_sync(&smp->security_timer);
- /* In case the timeout freed the SMP context */
- if (!chan->data)
- return;
if (work_pending(&smp->distribute_work)) {
cancel_work_sync(&smp->distribute_work);
--
1.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 2/7] Bluetooth: Don't take any action in smp_resume_cb if not encrypted
2014-09-05 10:50 [PATCH 0/7] Bluetooth: SMP fixes/improvements johan.hedberg
2014-09-05 10:51 ` [PATCH 1/7] Bluetooth: Remove unnecessary checks after canceling SMP security timer johan.hedberg
@ 2014-09-05 10:51 ` johan.hedberg
2014-09-05 11:19 ` Szymon Janc
2014-09-05 10:51 ` [PATCH 3/7] Bluetooth: Move identity address update behind a workqueue johan.hedberg
` (4 subsequent siblings)
6 siblings, 1 reply; 10+ messages in thread
From: johan.hedberg @ 2014-09-05 10:51 UTC (permalink / raw)
To: linux-bluetooth
From: Johan Hedberg <johan.hedberg@intel.com>
When smp_resume_cb is called if we're not encrypted (i.e. the callback
wasn't called because we the connection became encrypted) we shouldn't
take any action at all. This patch moves also the security_timer
cancellation behind this condition.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/smp.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index b8ecc7bd3e3b..9accb4739488 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -1533,10 +1533,12 @@ static void smp_resume_cb(struct l2cap_chan *chan)
if (!smp)
return;
+ if (!test_bit(HCI_CONN_ENCRYPT, &hcon->flags))
+ return;
+
cancel_delayed_work(&smp->security_timer);
- if (test_bit(HCI_CONN_ENCRYPT, &hcon->flags))
- queue_work(hdev->workqueue, &smp->distribute_work);
+ queue_work(hdev->workqueue, &smp->distribute_work);
}
static void smp_ready_cb(struct l2cap_chan *chan)
--
1.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/7] Bluetooth: Don't take any action in smp_resume_cb if not encrypted
2014-09-05 10:51 ` [PATCH 2/7] Bluetooth: Don't take any action in smp_resume_cb if not encrypted johan.hedberg
@ 2014-09-05 11:19 ` Szymon Janc
0 siblings, 0 replies; 10+ messages in thread
From: Szymon Janc @ 2014-09-05 11:19 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
Hi Johan,
On Friday 05 of September 2014 13:51:01 johan.hedberg@gmail.com wrote:
> From: Johan Hedberg <johan.hedberg@intel.com>
>
> When smp_resume_cb is called if we're not encrypted (i.e. the callback
> wasn't called because we the connection became encrypted) we shouldn't
There is something wrong with this comment in brackets.
> take any action at all. This patch moves also the security_timer
> cancellation behind this condition.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/smp.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
> index b8ecc7bd3e3b..9accb4739488 100644
> --- a/net/bluetooth/smp.c
> +++ b/net/bluetooth/smp.c
> @@ -1533,10 +1533,12 @@ static void smp_resume_cb(struct l2cap_chan *chan)
> if (!smp)
> return;
>
> + if (!test_bit(HCI_CONN_ENCRYPT, &hcon->flags))
> + return;
> +
> cancel_delayed_work(&smp->security_timer);
>
> - if (test_bit(HCI_CONN_ENCRYPT, &hcon->flags))
> - queue_work(hdev->workqueue, &smp->distribute_work);
> + queue_work(hdev->workqueue, &smp->distribute_work);
> }
>
> static void smp_ready_cb(struct l2cap_chan *chan)
>
--
Best regards,
Szymon Janc
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/7] Bluetooth: Move identity address update behind a workqueue
2014-09-05 10:50 [PATCH 0/7] Bluetooth: SMP fixes/improvements johan.hedberg
2014-09-05 10:51 ` [PATCH 1/7] Bluetooth: Remove unnecessary checks after canceling SMP security timer johan.hedberg
2014-09-05 10:51 ` [PATCH 2/7] Bluetooth: Don't take any action in smp_resume_cb if not encrypted johan.hedberg
@ 2014-09-05 10:51 ` johan.hedberg
2014-09-05 10:51 ` [PATCH 4/7] Bluetooth: Remove unnecessary deferred work for SMP key distribution johan.hedberg
` (3 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: johan.hedberg @ 2014-09-05 10:51 UTC (permalink / raw)
To: linux-bluetooth
From: Johan Hedberg <johan.hedberg@intel.com>
The identity address update of all channels for an l2cap_conn needs to
take the lock for each channel, i.e. it's safest to do this by a
separate workqueue callback.
Previously this was partially solved by moving the entire SMP key
distribution behind a workqueue. However, if we want SMP context locking
to be correct and safe we should always use the l2cap_chan lock when
accessing it, meaning even smp_distribute_keys needs to take that lock
which would once again create a dead lock when updating the identity
address.
The simplest way to solve this is to have l2cap_conn manage the deferred
work which is what this patch does. A subsequent patch will remove the
now unnecessary SMP key distribution work struct.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
include/net/bluetooth/l2cap.h | 3 ++-
net/bluetooth/l2cap_core.c | 10 ++++++++--
net/bluetooth/smp.c | 2 +-
3 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index be25eddea615..ead99f032f7a 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -633,6 +633,8 @@ struct l2cap_conn {
struct sk_buff_head pending_rx;
struct work_struct pending_rx_work;
+ struct work_struct id_addr_update_work;
+
__u8 disc_reason;
struct l2cap_chan *smp;
@@ -937,7 +939,6 @@ int l2cap_ertm_init(struct l2cap_chan *chan);
void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan);
void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan);
void l2cap_chan_del(struct l2cap_chan *chan, int err);
-void l2cap_conn_update_id_addr(struct hci_conn *hcon);
void l2cap_send_conn_req(struct l2cap_chan *chan);
void l2cap_move_start(struct l2cap_chan *chan);
void l2cap_logical_cfm(struct l2cap_chan *chan, struct hci_chan *hchan,
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index ab405f0e53cb..b71430caab4a 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -631,9 +631,11 @@ void l2cap_chan_del(struct l2cap_chan *chan, int err)
}
EXPORT_SYMBOL_GPL(l2cap_chan_del);
-void l2cap_conn_update_id_addr(struct hci_conn *hcon)
+static void l2cap_conn_update_id_addr(struct work_struct *work)
{
- struct l2cap_conn *conn = hcon->l2cap_data;
+ struct l2cap_conn *conn = container_of(work, struct l2cap_conn,
+ id_addr_update_work);
+ struct hci_conn *hcon = conn->hcon;
struct l2cap_chan *chan;
mutex_lock(&conn->chan_lock);
@@ -1635,6 +1637,9 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
if (work_pending(&conn->pending_rx_work))
cancel_work_sync(&conn->pending_rx_work);
+ if (work_pending(&conn->id_addr_update_work))
+ cancel_work_sync(&conn->id_addr_update_work);
+
l2cap_unregister_all_users(conn);
/* Force the connection to be immediately dropped */
@@ -6927,6 +6932,7 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon)
skb_queue_head_init(&conn->pending_rx);
INIT_WORK(&conn->pending_rx_work, process_pending_rx);
+ INIT_WORK(&conn->id_addr_update_work, l2cap_conn_update_id_addr);
conn->disc_reason = HCI_ERROR_REMOTE_USER_TERM;
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 9accb4739488..795c603bed30 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -654,7 +654,7 @@ static void smp_notify_keys(struct l2cap_conn *conn)
*/
bacpy(&hcon->dst, &smp->remote_irk->bdaddr);
hcon->dst_type = smp->remote_irk->addr_type;
- l2cap_conn_update_id_addr(hcon);
+ queue_work(hdev->workqueue, &conn->id_addr_update_work);
/* When receiving an indentity resolving key for
* a remote device that does not use a resolvable
--
1.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 4/7] Bluetooth: Remove unnecessary deferred work for SMP key distribution
2014-09-05 10:50 [PATCH 0/7] Bluetooth: SMP fixes/improvements johan.hedberg
` (2 preceding siblings ...)
2014-09-05 10:51 ` [PATCH 3/7] Bluetooth: Move identity address update behind a workqueue johan.hedberg
@ 2014-09-05 10:51 ` johan.hedberg
2014-09-05 10:51 ` [PATCH 5/7] Bluetooth: Fix locking of the SMP context johan.hedberg
` (2 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: johan.hedberg @ 2014-09-05 10:51 UTC (permalink / raw)
To: linux-bluetooth
From: Johan Hedberg <johan.hedberg@intel.com>
Now that the identity address update happens through its own deferred
work there's no need to have smp_distribute_keys anymore behind a second
deferred work. This patch removes this extra construction and makes the
code do direct calls to smp_distribute_keys() again.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/smp.c | 21 +++++----------------
1 file changed, 5 insertions(+), 16 deletions(-)
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 795c603bed30..0b4403f3dce1 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -46,7 +46,6 @@ enum {
struct smp_chan {
struct l2cap_conn *conn;
struct delayed_work security_timer;
- struct work_struct distribute_work;
u8 preq[7]; /* SMP Pairing Request */
u8 prsp[7]; /* SMP Pairing Response */
@@ -375,12 +374,6 @@ static void smp_chan_destroy(struct l2cap_conn *conn)
cancel_delayed_work_sync(&smp->security_timer);
- if (work_pending(&smp->distribute_work)) {
- cancel_work_sync(&smp->distribute_work);
- if (!chan->data)
- return;
- }
-
complete = test_bit(SMP_FLAG_COMPLETE, &smp->flags);
mgmt_smp_complete(conn->hcon, complete);
@@ -703,10 +696,8 @@ static void smp_notify_keys(struct l2cap_conn *conn)
}
}
-static void smp_distribute_keys(struct work_struct *work)
+static void smp_distribute_keys(struct smp_chan *smp)
{
- struct smp_chan *smp = container_of(work, struct smp_chan,
- distribute_work);
struct smp_cmd_pairing *req, *rsp;
struct l2cap_conn *conn = smp->conn;
struct hci_conn *hcon = conn->hcon;
@@ -850,7 +841,6 @@ static struct smp_chan *smp_chan_create(struct l2cap_conn *conn)
smp->conn = conn;
chan->data = smp;
- INIT_WORK(&smp->distribute_work, smp_distribute_keys);
INIT_DELAYED_WORK(&smp->security_timer, smp_timeout);
hci_conn_hold(conn->hcon);
@@ -1290,7 +1280,7 @@ static int smp_cmd_master_ident(struct l2cap_conn *conn, struct sk_buff *skb)
rp->ediv, rp->rand);
smp->ltk = ltk;
if (!(smp->remote_key_dist & SMP_DIST_ID_KEY))
- queue_work(hdev->workqueue, &smp->distribute_work);
+ smp_distribute_keys(smp);
hci_dev_unlock(hdev);
return 0;
@@ -1368,7 +1358,7 @@ static int smp_cmd_ident_addr_info(struct l2cap_conn *conn,
smp->id_addr_type, smp->irk, &rpa);
distribute:
- queue_work(hdev->workqueue, &smp->distribute_work);
+ smp_distribute_keys(smp);
hci_dev_unlock(hcon->hdev);
@@ -1404,7 +1394,7 @@ static int smp_cmd_sign_info(struct l2cap_conn *conn, struct sk_buff *skb)
memcpy(csrk->val, rp->csrk, sizeof(csrk->val));
}
smp->csrk = csrk;
- queue_work(hdev->workqueue, &smp->distribute_work);
+ smp_distribute_keys(smp);
hci_dev_unlock(hdev);
return 0;
@@ -1526,7 +1516,6 @@ static void smp_resume_cb(struct l2cap_chan *chan)
struct smp_chan *smp = chan->data;
struct l2cap_conn *conn = chan->conn;
struct hci_conn *hcon = conn->hcon;
- struct hci_dev *hdev = hcon->hdev;
BT_DBG("chan %p", chan);
@@ -1538,7 +1527,7 @@ static void smp_resume_cb(struct l2cap_chan *chan)
cancel_delayed_work(&smp->security_timer);
- queue_work(hdev->workqueue, &smp->distribute_work);
+ smp_distribute_keys(smp);
}
static void smp_ready_cb(struct l2cap_chan *chan)
--
1.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 5/7] Bluetooth: Fix locking of the SMP context
2014-09-05 10:50 [PATCH 0/7] Bluetooth: SMP fixes/improvements johan.hedberg
` (3 preceding siblings ...)
2014-09-05 10:51 ` [PATCH 4/7] Bluetooth: Remove unnecessary deferred work for SMP key distribution johan.hedberg
@ 2014-09-05 10:51 ` johan.hedberg
2014-09-05 11:20 ` Szymon Janc
2014-09-05 10:51 ` [PATCH 6/7] Bluetooth: Fix calling smp_distribute_keys() when still waiting for keys johan.hedberg
2014-09-05 10:51 ` [PATCH 7/7] Bluetooth: Add strict checks for allowed SMP PDUs johan.hedberg
6 siblings, 1 reply; 10+ messages in thread
From: johan.hedberg @ 2014-09-05 10:51 UTC (permalink / raw)
To: linux-bluetooth
From: Johan Hedberg <johan.hedberg@intel.com>
Before the move the l2cap_chan the SMP context (smp_chan) didn't have
any kind of proper locking. The best there existed was the
HCI_CONN_LE_SMP_PEND flag which was used to enable mutual exclusion for
potential multiple creators of the SMP context.
Now that SMP has been converted to use the l2cap_chan infrastructure and
since the SMP context is directly mapped to a corresponding l2cap_chan
we get the SMP context locking essentially for free through the
l2cap_chan lock. For all callbacks that l2cap_core.c makes for each
channel implementation (smp.c in the case of SMP) the l2cap_chan lock is
held through l2cap_chan_lock(chan).
Since the calls from l2cap_core.c to smp.c are covered the only missing
piece to have the locking implemented properly is to ensure that the
lock is held for any other call path that may access the SMP context.
This means user responses through mgmt.c, requests to elevate the
security of a connection through hci_conn.c, as well as any deferred
work through workqueues.
This patch adds the necessary locking to all these other code paths that
try to access the SMP context. Since mutual exclusion for the l2cap_chan
access is now covered from all directions the patch also removes
unnecessary HCI_CONN_LE_SMP_PEND flag (once we've acquired the chan lock
we can simply check whether chan->smp is set to know if there's an SMP
context).
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
include/net/bluetooth/hci_core.h | 1 -
net/bluetooth/smp.c | 73 +++++++++++++++++++++++-----------------
2 files changed, 43 insertions(+), 31 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 2b6e04d37593..045d9133d180 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -539,7 +539,6 @@ enum {
HCI_CONN_RSWITCH_PEND,
HCI_CONN_MODE_CHANGE_PEND,
HCI_CONN_SCO_SETUP_PEND,
- HCI_CONN_LE_SMP_PEND,
HCI_CONN_MGMT_CONNECTED,
HCI_CONN_SSP_ENABLED,
HCI_CONN_SC_ENABLED,
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 0b4403f3dce1..7bdc3166db97 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -409,7 +409,6 @@ static void smp_failure(struct l2cap_conn *conn, u8 reason)
{
struct hci_conn *hcon = conn->hcon;
struct l2cap_chan *chan = conn->smp;
- struct smp_chan *smp;
if (reason)
smp_send_cmd(conn, SMP_CMD_PAIRING_FAIL, sizeof(reason),
@@ -419,12 +418,7 @@ static void smp_failure(struct l2cap_conn *conn, u8 reason)
mgmt_auth_failed(hcon->hdev, &hcon->dst, hcon->type, hcon->dst_type,
HCI_ERROR_AUTH_FAILURE);
- if (!chan->data)
- return;
-
- smp = chan->data;
-
- if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags))
+ if (chan->data)
smp_chan_destroy(conn);
}
@@ -706,9 +700,6 @@ static void smp_distribute_keys(struct smp_chan *smp)
BT_DBG("conn %p", conn);
- if (!test_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags))
- return;
-
rsp = (void *) &smp->prsp[1];
/* The responder sends its keys first */
@@ -801,7 +792,6 @@ static void smp_distribute_keys(struct smp_chan *smp)
if ((smp->remote_key_dist & 0x07))
return;
- clear_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags);
set_bit(SMP_FLAG_COMPLETE, &smp->flags);
smp_notify_keys(conn);
@@ -825,16 +815,13 @@ static struct smp_chan *smp_chan_create(struct l2cap_conn *conn)
struct smp_chan *smp;
smp = kzalloc(sizeof(*smp), GFP_ATOMIC);
- if (!smp) {
- clear_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags);
+ 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);
- clear_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags);
return NULL;
}
@@ -854,16 +841,23 @@ int smp_user_confirm_reply(struct hci_conn *hcon, u16 mgmt_op, __le32 passkey)
struct l2cap_chan *chan;
struct smp_chan *smp;
u32 value;
+ int err;
BT_DBG("");
- if (!conn || !test_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags))
+ if (!conn)
return -ENOTCONN;
chan = conn->smp;
if (!chan)
return -ENOTCONN;
+ l2cap_chan_lock(chan);
+ if (!chan->data) {
+ err = -ENOTCONN;
+ goto unlock;
+ }
+
smp = chan->data;
switch (mgmt_op) {
@@ -879,12 +873,16 @@ int smp_user_confirm_reply(struct hci_conn *hcon, u16 mgmt_op, __le32 passkey)
case MGMT_OP_USER_PASSKEY_NEG_REPLY:
case MGMT_OP_USER_CONFIRM_NEG_REPLY:
smp_failure(conn, SMP_PASSKEY_ENTRY_FAILED);
- return 0;
+ err = 0;
+ goto unlock;
default:
smp_failure(conn, SMP_PASSKEY_ENTRY_FAILED);
- return -EOPNOTSUPP;
+ err = -EOPNOTSUPP;
+ goto unlock;
}
+ err = 0;
+
/* If it is our turn to send Pairing Confirm, do so now */
if (test_bit(SMP_FLAG_CFM_PENDING, &smp->flags)) {
u8 rsp = smp_confirm(smp);
@@ -892,12 +890,15 @@ int smp_user_confirm_reply(struct hci_conn *hcon, u16 mgmt_op, __le32 passkey)
smp_failure(conn, rsp);
}
- return 0;
+unlock:
+ l2cap_chan_unlock(chan);
+ return err;
}
static u8 smp_cmd_pairing_req(struct l2cap_conn *conn, struct sk_buff *skb)
{
struct smp_cmd_pairing rsp, *req = (void *) skb->data;
+ struct l2cap_chan *chan = conn->smp;
struct hci_dev *hdev = conn->hcon->hdev;
struct smp_chan *smp;
u8 key_size, auth, sec_level;
@@ -911,12 +912,10 @@ static u8 smp_cmd_pairing_req(struct l2cap_conn *conn, struct sk_buff *skb)
if (conn->hcon->role != HCI_ROLE_SLAVE)
return SMP_CMD_NOTSUPP;
- if (!test_and_set_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags)) {
+ if (!chan->data)
smp = smp_chan_create(conn);
- } else {
- struct l2cap_chan *chan = conn->smp;
+ else
smp = chan->data;
- }
if (!smp)
return SMP_UNSPECIFIED;
@@ -1122,6 +1121,7 @@ static u8 smp_cmd_security_req(struct l2cap_conn *conn, struct sk_buff *skb)
struct smp_cmd_security_req *rp = (void *) skb->data;
struct smp_cmd_pairing cp;
struct hci_conn *hcon = conn->hcon;
+ struct l2cap_chan *chan = conn->smp;
struct smp_chan *smp;
u8 sec_level;
@@ -1143,7 +1143,8 @@ static u8 smp_cmd_security_req(struct l2cap_conn *conn, struct sk_buff *skb)
if (smp_ltk_encrypt(conn, hcon->pending_sec_level))
return 0;
- if (test_and_set_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags))
+ /* If SMP is already in progress ignore this request */
+ if (chan->data)
return 0;
smp = smp_chan_create(conn);
@@ -1170,8 +1171,10 @@ static u8 smp_cmd_security_req(struct l2cap_conn *conn, struct sk_buff *skb)
int smp_conn_security(struct hci_conn *hcon, __u8 sec_level)
{
struct l2cap_conn *conn = hcon->l2cap_data;
+ struct l2cap_chan *chan = conn->smp;
struct smp_chan *smp;
__u8 authreq;
+ int ret;
BT_DBG("conn %p hcon %p level 0x%2.2x", conn, hcon, sec_level);
@@ -1192,12 +1195,19 @@ int smp_conn_security(struct hci_conn *hcon, __u8 sec_level)
if (smp_ltk_encrypt(conn, hcon->pending_sec_level))
return 0;
- if (test_and_set_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags))
- return 0;
+ l2cap_chan_lock(chan);
+
+ /* If SMP is already in progress ignore this request */
+ if (chan->data) {
+ ret = 0;
+ goto unlock;
+ }
smp = smp_chan_create(conn);
- if (!smp)
- return 1;
+ if (!smp) {
+ ret = 1;
+ goto unlock;
+ }
authreq = seclevel_to_authreq(sec_level);
@@ -1223,8 +1233,11 @@ int smp_conn_security(struct hci_conn *hcon, __u8 sec_level)
}
set_bit(SMP_FLAG_INITIATOR, &smp->flags);
+ ret = 0;
- return 0;
+unlock:
+ l2cap_chan_unlock(conn->smp);
+ return ret;
}
static int smp_cmd_encrypt_info(struct l2cap_conn *conn, struct sk_buff *skb)
@@ -1504,7 +1517,7 @@ static void smp_teardown_cb(struct l2cap_chan *chan, int err)
BT_DBG("chan %p", chan);
- if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags))
+ if (chan->data)
smp_chan_destroy(conn);
conn->smp = NULL;
--
1.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 5/7] Bluetooth: Fix locking of the SMP context
2014-09-05 10:51 ` [PATCH 5/7] Bluetooth: Fix locking of the SMP context johan.hedberg
@ 2014-09-05 11:20 ` Szymon Janc
0 siblings, 0 replies; 10+ messages in thread
From: Szymon Janc @ 2014-09-05 11:20 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
Hi Johan,
On Friday 05 of September 2014 13:51:04 johan.hedberg@gmail.com wrote:
> From: Johan Hedberg <johan.hedberg@intel.com>
>
> Before the move the l2cap_chan the SMP context (smp_chan) didn't have
> any kind of proper locking. The best there existed was the
> HCI_CONN_LE_SMP_PEND flag which was used to enable mutual exclusion for
> potential multiple creators of the SMP context.
>
> Now that SMP has been converted to use the l2cap_chan infrastructure and
> since the SMP context is directly mapped to a corresponding l2cap_chan
> we get the SMP context locking essentially for free through the
> l2cap_chan lock. For all callbacks that l2cap_core.c makes for each
> channel implementation (smp.c in the case of SMP) the l2cap_chan lock is
> held through l2cap_chan_lock(chan).
>
> Since the calls from l2cap_core.c to smp.c are covered the only missing
> piece to have the locking implemented properly is to ensure that the
> lock is held for any other call path that may access the SMP context.
> This means user responses through mgmt.c, requests to elevate the
> security of a connection through hci_conn.c, as well as any deferred
> work through workqueues.
>
> This patch adds the necessary locking to all these other code paths that
> try to access the SMP context. Since mutual exclusion for the l2cap_chan
> access is now covered from all directions the patch also removes
> unnecessary HCI_CONN_LE_SMP_PEND flag (once we've acquired the chan lock
> we can simply check whether chan->smp is set to know if there's an SMP
> context).
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/hci_core.h | 1 -
> net/bluetooth/smp.c | 73 +++++++++++++++++++++++-----------------
> 2 files changed, 43 insertions(+), 31 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 2b6e04d37593..045d9133d180 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -539,7 +539,6 @@ enum {
> HCI_CONN_RSWITCH_PEND,
> HCI_CONN_MODE_CHANGE_PEND,
> HCI_CONN_SCO_SETUP_PEND,
> - HCI_CONN_LE_SMP_PEND,
> HCI_CONN_MGMT_CONNECTED,
> HCI_CONN_SSP_ENABLED,
> HCI_CONN_SC_ENABLED,
> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
> index 0b4403f3dce1..7bdc3166db97 100644
> --- a/net/bluetooth/smp.c
> +++ b/net/bluetooth/smp.c
> @@ -409,7 +409,6 @@ static void smp_failure(struct l2cap_conn *conn, u8 reason)
> {
> struct hci_conn *hcon = conn->hcon;
> struct l2cap_chan *chan = conn->smp;
> - struct smp_chan *smp;
>
> if (reason)
> smp_send_cmd(conn, SMP_CMD_PAIRING_FAIL, sizeof(reason),
> @@ -419,12 +418,7 @@ static void smp_failure(struct l2cap_conn *conn, u8 reason)
> mgmt_auth_failed(hcon->hdev, &hcon->dst, hcon->type, hcon->dst_type,
> HCI_ERROR_AUTH_FAILURE);
>
> - if (!chan->data)
> - return;
> -
> - smp = chan->data;
> -
> - if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags))
> + if (chan->data)
> smp_chan_destroy(conn);
> }
>
> @@ -706,9 +700,6 @@ static void smp_distribute_keys(struct smp_chan *smp)
>
> BT_DBG("conn %p", conn);
>
> - if (!test_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags))
> - return;
> -
> rsp = (void *) &smp->prsp[1];
>
> /* The responder sends its keys first */
> @@ -801,7 +792,6 @@ static void smp_distribute_keys(struct smp_chan *smp)
> if ((smp->remote_key_dist & 0x07))
> return;
>
> - clear_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags);
> set_bit(SMP_FLAG_COMPLETE, &smp->flags);
> smp_notify_keys(conn);
>
> @@ -825,16 +815,13 @@ static struct smp_chan *smp_chan_create(struct l2cap_conn *conn)
> struct smp_chan *smp;
>
> smp = kzalloc(sizeof(*smp), GFP_ATOMIC);
> - if (!smp) {
> - clear_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags);
> + 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);
> - clear_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags);
> return NULL;
> }
>
> @@ -854,16 +841,23 @@ int smp_user_confirm_reply(struct hci_conn *hcon, u16 mgmt_op, __le32 passkey)
> struct l2cap_chan *chan;
> struct smp_chan *smp;
> u32 value;
> + int err;
>
> BT_DBG("");
>
> - if (!conn || !test_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags))
> + if (!conn)
> return -ENOTCONN;
>
> chan = conn->smp;
> if (!chan)
> return -ENOTCONN;
>
> + l2cap_chan_lock(chan);
> + if (!chan->data) {
> + err = -ENOTCONN;
> + goto unlock;
> + }
> +
> smp = chan->data;
>
> switch (mgmt_op) {
> @@ -879,12 +873,16 @@ int smp_user_confirm_reply(struct hci_conn *hcon, u16 mgmt_op, __le32 passkey)
> case MGMT_OP_USER_PASSKEY_NEG_REPLY:
> case MGMT_OP_USER_CONFIRM_NEG_REPLY:
> smp_failure(conn, SMP_PASSKEY_ENTRY_FAILED);
> - return 0;
> + err = 0;
> + goto unlock;
> default:
> smp_failure(conn, SMP_PASSKEY_ENTRY_FAILED);
> - return -EOPNOTSUPP;
> + err = -EOPNOTSUPP;
> + goto unlock;
> }
>
> + err = 0;
> +
> /* If it is our turn to send Pairing Confirm, do so now */
> if (test_bit(SMP_FLAG_CFM_PENDING, &smp->flags)) {
> u8 rsp = smp_confirm(smp);
> @@ -892,12 +890,15 @@ int smp_user_confirm_reply(struct hci_conn *hcon, u16 mgmt_op, __le32 passkey)
> smp_failure(conn, rsp);
> }
>
> - return 0;
> +unlock:
> + l2cap_chan_unlock(chan);
> + return err;
> }
>
> static u8 smp_cmd_pairing_req(struct l2cap_conn *conn, struct sk_buff *skb)
> {
> struct smp_cmd_pairing rsp, *req = (void *) skb->data;
> + struct l2cap_chan *chan = conn->smp;
> struct hci_dev *hdev = conn->hcon->hdev;
> struct smp_chan *smp;
> u8 key_size, auth, sec_level;
> @@ -911,12 +912,10 @@ static u8 smp_cmd_pairing_req(struct l2cap_conn *conn, struct sk_buff *skb)
> if (conn->hcon->role != HCI_ROLE_SLAVE)
> return SMP_CMD_NOTSUPP;
>
> - if (!test_and_set_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags)) {
> + if (!chan->data)
> smp = smp_chan_create(conn);
> - } else {
> - struct l2cap_chan *chan = conn->smp;
> + else
> smp = chan->data;
> - }
>
> if (!smp)
> return SMP_UNSPECIFIED;
> @@ -1122,6 +1121,7 @@ static u8 smp_cmd_security_req(struct l2cap_conn *conn, struct sk_buff *skb)
> struct smp_cmd_security_req *rp = (void *) skb->data;
> struct smp_cmd_pairing cp;
> struct hci_conn *hcon = conn->hcon;
> + struct l2cap_chan *chan = conn->smp;
> struct smp_chan *smp;
> u8 sec_level;
>
> @@ -1143,7 +1143,8 @@ static u8 smp_cmd_security_req(struct l2cap_conn *conn, struct sk_buff *skb)
> if (smp_ltk_encrypt(conn, hcon->pending_sec_level))
> return 0;
>
> - if (test_and_set_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags))
> + /* If SMP is already in progress ignore this request */
> + if (chan->data)
> return 0;
>
> smp = smp_chan_create(conn);
> @@ -1170,8 +1171,10 @@ static u8 smp_cmd_security_req(struct l2cap_conn *conn, struct sk_buff *skb)
> int smp_conn_security(struct hci_conn *hcon, __u8 sec_level)
> {
> struct l2cap_conn *conn = hcon->l2cap_data;
> + struct l2cap_chan *chan = conn->smp;
> struct smp_chan *smp;
> __u8 authreq;
> + int ret;
>
> BT_DBG("conn %p hcon %p level 0x%2.2x", conn, hcon, sec_level);
>
> @@ -1192,12 +1195,19 @@ int smp_conn_security(struct hci_conn *hcon, __u8 sec_level)
> if (smp_ltk_encrypt(conn, hcon->pending_sec_level))
> return 0;
>
> - if (test_and_set_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags))
> - return 0;
> + l2cap_chan_lock(chan);
> +
> + /* If SMP is already in progress ignore this request */
> + if (chan->data) {
> + ret = 0;
> + goto unlock;
> + }
>
> smp = smp_chan_create(conn);
> - if (!smp)
> - return 1;
> + if (!smp) {
> + ret = 1;
> + goto unlock;
> + }
>
> authreq = seclevel_to_authreq(sec_level);
>
> @@ -1223,8 +1233,11 @@ int smp_conn_security(struct hci_conn *hcon, __u8 sec_level)
> }
>
> set_bit(SMP_FLAG_INITIATOR, &smp->flags);
> + ret = 0;
>
> - return 0;
> +unlock:
> + l2cap_chan_unlock(conn->smp);
I know that this is same data but using chan variable instead of conn->smp will make code
easier to follow since chan was used for locking.
> + return ret;
> }
>
> static int smp_cmd_encrypt_info(struct l2cap_conn *conn, struct sk_buff *skb)
> @@ -1504,7 +1517,7 @@ static void smp_teardown_cb(struct l2cap_chan *chan, int err)
>
> BT_DBG("chan %p", chan);
>
> - if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags))
> + if (chan->data)
> smp_chan_destroy(conn);
>
> conn->smp = NULL;
>
--
Best regards,
Szymon Janc
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 6/7] Bluetooth: Fix calling smp_distribute_keys() when still waiting for keys
2014-09-05 10:50 [PATCH 0/7] Bluetooth: SMP fixes/improvements johan.hedberg
` (4 preceding siblings ...)
2014-09-05 10:51 ` [PATCH 5/7] Bluetooth: Fix locking of the SMP context johan.hedberg
@ 2014-09-05 10:51 ` johan.hedberg
2014-09-05 10:51 ` [PATCH 7/7] Bluetooth: Add strict checks for allowed SMP PDUs johan.hedberg
6 siblings, 0 replies; 10+ messages in thread
From: johan.hedberg @ 2014-09-05 10:51 UTC (permalink / raw)
To: linux-bluetooth
From: Johan Hedberg <johan.hedberg@intel.com>
When we're in the process of receiving keys in phase 3 of SMP we keep
track of which keys are still expected in the smp->remote_key_dist
variable. If we still have some key bits set we need to continue waiting
for more PDUs and not needlessly call smp_distribute_keys(). This patch
fixes two such cases in the smp_cmd_master_ident() and
smp_cmd_ident_addr_info() handler functions.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/smp.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 7bdc3166db97..5562431b8406 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -1292,7 +1292,7 @@ static int smp_cmd_master_ident(struct l2cap_conn *conn, struct sk_buff *skb)
authenticated, smp->tk, smp->enc_key_size,
rp->ediv, rp->rand);
smp->ltk = ltk;
- if (!(smp->remote_key_dist & SMP_DIST_ID_KEY))
+ if (!(smp->remote_key_dist & 0x07))
smp_distribute_keys(smp);
hci_dev_unlock(hdev);
@@ -1371,7 +1371,8 @@ static int smp_cmd_ident_addr_info(struct l2cap_conn *conn,
smp->id_addr_type, smp->irk, &rpa);
distribute:
- smp_distribute_keys(smp);
+ if (!(smp->remote_key_dist & 0x07))
+ smp_distribute_keys(smp);
hci_dev_unlock(hcon->hdev);
--
1.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 7/7] Bluetooth: Add strict checks for allowed SMP PDUs
2014-09-05 10:50 [PATCH 0/7] Bluetooth: SMP fixes/improvements johan.hedberg
` (5 preceding siblings ...)
2014-09-05 10:51 ` [PATCH 6/7] Bluetooth: Fix calling smp_distribute_keys() when still waiting for keys johan.hedberg
@ 2014-09-05 10:51 ` johan.hedberg
6 siblings, 0 replies; 10+ messages in thread
From: johan.hedberg @ 2014-09-05 10:51 UTC (permalink / raw)
To: linux-bluetooth
From: Johan Hedberg <johan.hedberg@intel.com>
SMP defines quite clearly when certain PDUs are to be expected/allowed
and when not, but doesn't have any explicit request/response definition.
So far the code has relied on each PDU handler to behave correctly if
receiving PDUs at an unexpected moment, however this requires many
different checks and is prone to errors.
This patch introduces a generic way to keep track of allowed PDUs and
thereby reduces the responsibility & load on individual command
handlers. The tracking is implemented using a simple bit-mask where each
opcode maps to its own bit. If the bit is set the corresponding PDU is
allow and if the bit is not set the PDU is not allowed.
As a simple example, when we send the Pairing Request we'd set the bit
for Pairing Response, and when we receive the Pairing Response we'd
clear the bit for Pairing Response.
Since the disallowed PDU rejection is now done in a single central place
we need to be a bit careful of which action makes most sense to all
cases. Previously some, such as Security Request, have been simply
ignored whereas others have caused an explicit disconnect.
The only PDU rejection action that keeps good interoperability and can
be used for all the applicable use cases is to drop the data. This may
raise some concerns of us now being more lenient for misbehaving (and
potentially malicious) devices, but the policy of simply dropping data
has been a successful one for many years e.g. in L2CAP (where this is
the *only* policy for such cases - we never request disconnection in
l2cap_core.c because of bad data). Furthermore, we cannot prevent
connected devices from creating the SMP context (through a Security or
Pairing Request), and once the context exists looking up the
corresponding bit for the received opcode and deciding to reject it is
essentially an equally lightweight operation as the kind of rejection
that l2cap_core.c already successfully does.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/smp.c | 122 ++++++++++++++++++++++++++++++++++++++--------------
net/bluetooth/smp.h | 2 +
2 files changed, 92 insertions(+), 32 deletions(-)
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 5562431b8406..0e223be38cac 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -46,6 +46,7 @@ enum {
struct smp_chan {
struct l2cap_conn *conn;
struct delayed_work security_timer;
+ unsigned long allow_cmd; /* Bitmask of allowed commands */
u8 preq[7]; /* SMP Pairing Request */
u8 prsp[7]; /* SMP Pairing Response */
@@ -68,6 +69,24 @@ struct smp_chan {
struct crypto_blkcipher *tfm_aes;
};
+static bool smp_cmd_allowed(struct smp_chan *smp, u8 code)
+{
+ if (code > SMP_CMD_MAX)
+ return false;
+
+ return test_bit(code, &smp->allow_cmd);
+}
+
+static void smp_allow_cmd(struct smp_chan *smp, u8 code)
+{
+ set_bit(code, &smp->allow_cmd);
+}
+
+static void smp_disallow_cmd(struct smp_chan *smp, u8 code)
+{
+ clear_bit(code, &smp->allow_cmd);
+}
+
static inline void swap_buf(const u8 *src, u8 *dst, size_t len)
{
size_t i;
@@ -552,6 +571,11 @@ static u8 smp_confirm(struct smp_chan *smp)
smp_send_cmd(smp->conn, SMP_CMD_PAIRING_CONFIRM, sizeof(cp), &cp);
+ if (conn->hcon->out)
+ smp_allow_cmd(smp, SMP_CMD_PAIRING_CONFIRM);
+ else
+ smp_allow_cmd(smp, SMP_CMD_PAIRING_RANDOM);
+
return 0;
}
@@ -690,6 +714,20 @@ static void smp_notify_keys(struct l2cap_conn *conn)
}
}
+static void smp_allow_key_dist(struct smp_chan *smp)
+{
+ /* Allow the first expected phase 3 PDU. The rest of the PDUs
+ * will be allowed in each PDU handler to ensure we receive
+ * them in the correct order.
+ */
+ if (smp->remote_key_dist & SMP_DIST_ENC_KEY)
+ smp_allow_cmd(smp, SMP_CMD_ENCRYPT_INFO);
+ else if (smp->remote_key_dist & SMP_DIST_ID_KEY)
+ smp_allow_cmd(smp, SMP_CMD_IDENT_INFO);
+ else if (smp->remote_key_dist & SMP_DIST_SIGN)
+ smp_allow_cmd(smp, SMP_CMD_SIGN_INFO);
+}
+
static void smp_distribute_keys(struct smp_chan *smp)
{
struct smp_cmd_pairing *req, *rsp;
@@ -703,8 +741,10 @@ static void smp_distribute_keys(struct smp_chan *smp)
rsp = (void *) &smp->prsp[1];
/* The responder sends its keys first */
- if (hcon->out && (smp->remote_key_dist & 0x07))
+ if (hcon->out && (smp->remote_key_dist & 0x07)) {
+ smp_allow_key_dist(smp);
return;
+ }
req = (void *) &smp->preq[1];
@@ -789,8 +829,10 @@ static void smp_distribute_keys(struct smp_chan *smp)
}
/* If there are still keys to be received wait for them */
- if ((smp->remote_key_dist & 0x07))
+ if ((smp->remote_key_dist & 0x07)) {
+ smp_allow_key_dist(smp);
return;
+ }
set_bit(SMP_FLAG_COMPLETE, &smp->flags);
smp_notify_keys(conn);
@@ -828,6 +870,8 @@ static struct smp_chan *smp_chan_create(struct l2cap_conn *conn)
smp->conn = conn;
chan->data = smp;
+ smp_allow_cmd(smp, SMP_CMD_PAIRING_FAIL);
+
INIT_DELAYED_WORK(&smp->security_timer, smp_timeout);
hci_conn_hold(conn->hcon);
@@ -924,6 +968,8 @@ static u8 smp_cmd_pairing_req(struct l2cap_conn *conn, struct sk_buff *skb)
(req->auth_req & SMP_AUTH_BONDING))
return SMP_PAIRING_NOTSUPP;
+ smp_disallow_cmd(smp, SMP_CMD_PAIRING_REQ);
+
smp->preq[0] = SMP_CMD_PAIRING_REQ;
memcpy(&smp->preq[1], req, sizeof(*req));
skb_pull(skb, sizeof(*req));
@@ -957,6 +1003,7 @@ static u8 smp_cmd_pairing_req(struct l2cap_conn *conn, struct sk_buff *skb)
memcpy(&smp->prsp[1], &rsp, sizeof(rsp));
smp_send_cmd(conn, SMP_CMD_PAIRING_RSP, sizeof(rsp), &rsp);
+ smp_allow_cmd(smp, SMP_CMD_PAIRING_CONFIRM);
/* Request setup of TK */
ret = tk_request(conn, 0, auth, rsp.io_capability, req->io_capability);
@@ -982,6 +1029,8 @@ static u8 smp_cmd_pairing_rsp(struct l2cap_conn *conn, struct sk_buff *skb)
if (conn->hcon->role != HCI_ROLE_MASTER)
return SMP_CMD_NOTSUPP;
+ smp_disallow_cmd(smp, SMP_CMD_PAIRING_RSP);
+
skb_pull(skb, sizeof(*rsp));
req = (void *) &smp->preq[1];
@@ -1039,13 +1088,19 @@ static u8 smp_cmd_pairing_confirm(struct l2cap_conn *conn, struct sk_buff *skb)
if (skb->len < sizeof(smp->pcnf))
return SMP_INVALID_PARAMS;
+ smp_disallow_cmd(smp, SMP_CMD_PAIRING_CONFIRM);
+
memcpy(smp->pcnf, skb->data, sizeof(smp->pcnf));
skb_pull(skb, sizeof(smp->pcnf));
- if (conn->hcon->out)
+ if (conn->hcon->out) {
smp_send_cmd(conn, SMP_CMD_PAIRING_RANDOM, sizeof(smp->prnd),
smp->prnd);
- else if (test_bit(SMP_FLAG_TK_VALID, &smp->flags))
+ smp_allow_cmd(smp, SMP_CMD_PAIRING_RANDOM);
+ return 0;
+ }
+
+ if (test_bit(SMP_FLAG_TK_VALID, &smp->flags))
return smp_confirm(smp);
else
set_bit(SMP_FLAG_CFM_PENDING, &smp->flags);
@@ -1063,6 +1118,8 @@ static u8 smp_cmd_pairing_random(struct l2cap_conn *conn, struct sk_buff *skb)
if (skb->len < sizeof(smp->rrnd))
return SMP_INVALID_PARAMS;
+ smp_disallow_cmd(smp, SMP_CMD_PAIRING_RANDOM);
+
memcpy(smp->rrnd, skb->data, sizeof(smp->rrnd));
skb_pull(skb, sizeof(smp->rrnd));
@@ -1164,6 +1221,7 @@ static u8 smp_cmd_security_req(struct l2cap_conn *conn, struct sk_buff *skb)
memcpy(&smp->preq[1], &cp, sizeof(cp));
smp_send_cmd(conn, SMP_CMD_PAIRING_REQ, sizeof(cp), &cp);
+ smp_allow_cmd(smp, SMP_CMD_PAIRING_RSP);
return 0;
}
@@ -1226,10 +1284,12 @@ int smp_conn_security(struct hci_conn *hcon, __u8 sec_level)
memcpy(&smp->preq[1], &cp, sizeof(cp));
smp_send_cmd(conn, SMP_CMD_PAIRING_REQ, sizeof(cp), &cp);
+ smp_allow_cmd(smp, SMP_CMD_PAIRING_RSP);
} else {
struct smp_cmd_security_req cp;
cp.auth_req = authreq;
smp_send_cmd(conn, SMP_CMD_SECURITY_REQ, sizeof(cp), &cp);
+ smp_allow_cmd(smp, SMP_CMD_PAIRING_REQ);
}
set_bit(SMP_FLAG_INITIATOR, &smp->flags);
@@ -1251,9 +1311,8 @@ static int smp_cmd_encrypt_info(struct l2cap_conn *conn, struct sk_buff *skb)
if (skb->len < sizeof(*rp))
return SMP_INVALID_PARAMS;
- /* Ignore this PDU if it wasn't requested */
- if (!(smp->remote_key_dist & SMP_DIST_ENC_KEY))
- return 0;
+ smp_disallow_cmd(smp, SMP_CMD_ENCRYPT_INFO);
+ smp_allow_cmd(smp, SMP_CMD_MASTER_IDENT);
skb_pull(skb, sizeof(*rp));
@@ -1277,13 +1336,13 @@ static int smp_cmd_master_ident(struct l2cap_conn *conn, struct sk_buff *skb)
if (skb->len < sizeof(*rp))
return SMP_INVALID_PARAMS;
- /* Ignore this PDU if it wasn't requested */
- if (!(smp->remote_key_dist & SMP_DIST_ENC_KEY))
- return 0;
-
/* Mark the information as received */
smp->remote_key_dist &= ~SMP_DIST_ENC_KEY;
+ smp_disallow_cmd(smp, SMP_CMD_MASTER_IDENT);
+ if (smp->remote_key_dist & SMP_DIST_ID_KEY)
+ smp_allow_cmd(smp, SMP_CMD_IDENT_INFO);
+
skb_pull(skb, sizeof(*rp));
hci_dev_lock(hdev);
@@ -1310,9 +1369,8 @@ static int smp_cmd_ident_info(struct l2cap_conn *conn, struct sk_buff *skb)
if (skb->len < sizeof(*info))
return SMP_INVALID_PARAMS;
- /* Ignore this PDU if it wasn't requested */
- if (!(smp->remote_key_dist & SMP_DIST_ID_KEY))
- return 0;
+ smp_disallow_cmd(smp, SMP_CMD_IDENT_INFO);
+ smp_allow_cmd(smp, SMP_CMD_IDENT_ADDR_INFO);
skb_pull(skb, sizeof(*info));
@@ -1328,7 +1386,6 @@ static int smp_cmd_ident_addr_info(struct l2cap_conn *conn,
struct l2cap_chan *chan = conn->smp;
struct smp_chan *smp = chan->data;
struct hci_conn *hcon = conn->hcon;
- struct hci_dev *hdev = hcon->hdev;
bdaddr_t rpa;
BT_DBG("");
@@ -1336,13 +1393,13 @@ static int smp_cmd_ident_addr_info(struct l2cap_conn *conn,
if (skb->len < sizeof(*info))
return SMP_INVALID_PARAMS;
- /* Ignore this PDU if it wasn't requested */
- if (!(smp->remote_key_dist & SMP_DIST_ID_KEY))
- return 0;
-
/* Mark the information as received */
smp->remote_key_dist &= ~SMP_DIST_ID_KEY;
+ smp_disallow_cmd(smp, SMP_CMD_IDENT_ADDR_INFO);
+ if (smp->remote_key_dist & SMP_DIST_SIGN)
+ smp_allow_cmd(smp, SMP_CMD_SIGN_INFO);
+
skb_pull(skb, sizeof(*info));
hci_dev_lock(hcon->hdev);
@@ -1392,13 +1449,11 @@ static int smp_cmd_sign_info(struct l2cap_conn *conn, struct sk_buff *skb)
if (skb->len < sizeof(*rp))
return SMP_INVALID_PARAMS;
- /* Ignore this PDU if it wasn't requested */
- if (!(smp->remote_key_dist & SMP_DIST_SIGN))
- return 0;
-
/* Mark the information as received */
smp->remote_key_dist &= ~SMP_DIST_SIGN;
+ smp_disallow_cmd(smp, SMP_CMD_SIGN_INFO);
+
skb_pull(skb, sizeof(*rp));
hci_dev_lock(hdev);
@@ -1418,6 +1473,7 @@ static int smp_sig_channel(struct l2cap_chan *chan, struct sk_buff *skb)
{
struct l2cap_conn *conn = chan->conn;
struct hci_conn *hcon = conn->hcon;
+ struct smp_chan *smp;
__u8 code, reason;
int err = 0;
@@ -1437,16 +1493,18 @@ static int smp_sig_channel(struct l2cap_chan *chan, struct sk_buff *skb)
code = skb->data[0];
skb_pull(skb, sizeof(code));
- /*
- * The SMP context must be initialized for all other PDUs except
- * pairing and security requests. If we get any other PDU when
- * not initialized simply disconnect (done if this function
- * returns an error).
+ smp = chan->data;
+
+ /* If we have an existing context only allow explicitly allowed
+ * commands. If we don't have a context the only allowed
+ * commands are pairing request and security request.
*/
- if (code != SMP_CMD_PAIRING_REQ && code != SMP_CMD_SECURITY_REQ &&
- !test_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags)) {
- BT_ERR("Unexpected SMP command 0x%02x. Disconnecting.", code);
- err = -EOPNOTSUPP;
+ if ((smp && !smp_cmd_allowed(smp, code)) ||
+ (!smp && code != SMP_CMD_PAIRING_REQ &&
+ code != SMP_CMD_SECURITY_REQ)) {
+ BT_ERR("%s unexpected SMP command 0x%02x from %pMR",
+ hcon->hdev->name, code, &hcon->dst);
+ reason = 0;
goto done;
}
diff --git a/net/bluetooth/smp.h b/net/bluetooth/smp.h
index cf1094617c69..5240537efde3 100644
--- a/net/bluetooth/smp.h
+++ b/net/bluetooth/smp.h
@@ -102,6 +102,8 @@ struct smp_cmd_security_req {
__u8 auth_req;
} __packed;
+#define SMP_CMD_MAX 0x0b
+
#define SMP_PASSKEY_ENTRY_FAILED 0x01
#define SMP_OOB_NOT_AVAIL 0x02
#define SMP_AUTH_REQUIREMENTS 0x03
--
1.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread