Hi Johan,
On Friday 05 of September 2014 13:51:04 johan.hedberg@xxxxxxxxx wrote:
> From: Johan Hedberg <johan.hedberg@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
