This includes:
1. Addition of BR/EDR vs LE breakout in user_confirm_reply
1.1 Adds SMP placeholder
2. Addition of user_passkey_reply, with both BR/EDR (SSP) and LE (SMP) handling
2.1 Adds approriate SSP HCI opcodes
2.2 Adds appropriate MGMT opcodes
2.3 Adds SMP placeholder
--
Brian Gix
[email protected]
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi Brian,
> >> + conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,&cp->bdaddr);
> >> + if (!conn) {
> >> + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,&cp->bdaddr);
> >> + if (!conn) {
> >> + err = cmd_status(sk, index, mgmt_op, ENOTCONN);
> >> + goto done;
> >> + }
> >> +
> >> + /* Forward Confirm response to SMP */
> >> +
> >> + err = cmd_status(sk, index, mgmt_op, 0);
> >> + goto done;
> >> +
> >> }
> >
> > I am a bit lost why we are trying to find an ACL_LINK here first.
>
> We want to route the confirmation to the Secure Simple Pair HCI command
> if this is being done on an ACL link, and to the SMP pairing algorythm
> if it is being done on an LE link.
>
> While I suppose we could route all non-LE results to the related HCI
> command, it seemed cleaner to me to reject it with an ENOTCONN, rather
> than make an invalid HCI call, and depending on the baseband generating
> the error.
now I am getting it. This is not really obvious when you look at the
code for the first time. Reason is that the ENOTCONN is too nested to be
obvious. Care to add a comment about this so I do not ask dumb question
next time ;)
Regards
Marcel
Hi Marcel,
On 11/9/2011 2:26 PM, Marcel Holtmann wrote:
>> + conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,&cp->bdaddr);
>> + if (!conn) {
>> + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,&cp->bdaddr);
>> + if (!conn) {
>> + err = cmd_status(sk, index, mgmt_op, ENOTCONN);
>> + goto done;
>> + }
>> +
>> + /* Forward Confirm response to SMP */
>> +
>> + err = cmd_status(sk, index, mgmt_op, 0);
>> + goto done;
>> +
>> }
>
> I am a bit lost why we are trying to find an ACL_LINK here first.
We want to route the confirmation to the Secure Simple Pair HCI command
if this is being done on an ACL link, and to the SMP pairing algorythm
if it is being done on an LE link.
While I suppose we could route all non-LE results to the related HCI
command, it seemed cleaner to me to reject it with an ENOTCONN, rather
than make an invalid HCI call, and depending on the baseband generating
the error.
--
Brian Gix
[email protected]
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi Brian,
> Some Man-In-The-Middle (MITM) protection schemes require
> User Passkey Entry.
>
> Signed-off-by: Brian Gix <[email protected]>
> ---
> include/net/bluetooth/hci.h | 8 ++++
> include/net/bluetooth/mgmt.h | 12 ++++++
> net/bluetooth/mgmt.c | 77 +++++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 96 insertions(+), 1 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 139ce2a..ac107b5 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -453,6 +453,14 @@ struct hci_rp_user_confirm_reply {
>
> #define HCI_OP_USER_CONFIRM_NEG_REPLY 0x042d
>
> +#define HCI_OP_USER_PASSKEY_REPLY 0x042e
> +struct hci_cp_user_passkey_reply {
> + bdaddr_t bdaddr;
> + __u32 passkey;
> +} __packed;
> +
> +#define HCI_OP_USER_PASSKEY_NEG_REPLY 0x042f
> +
> #define HCI_OP_REMOTE_OOB_DATA_REPLY 0x0430
> struct hci_cp_remote_oob_data_reply {
> bdaddr_t bdaddr;
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index 3e320c9..aa56bd5 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -228,6 +228,18 @@ struct mgmt_cp_set_fast_connectable {
> __u8 enable;
> } __packed;
>
> +#define MGMT_OP_USER_PASSKEY_REPLY 0x0020
> +struct mgmt_cp_user_passkey_reply {
> + bdaddr_t bdaddr;
> + __le32 passkey;
> +} __packed;
> +
> +#define MGMT_OP_USER_PASSKEY_NEG_REPLY 0x0021
> +struct mgmt_cp_user_passkey_neg_reply {
> + bdaddr_t bdaddr;
> +} __packed;
> +
> +
No need for two empty lines here.
> #define MGMT_EV_CMD_COMPLETE 0x0001
> struct mgmt_ev_cmd_complete {
> __le16 opcode;
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 6c35f8d..c6e1ad4 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -1462,7 +1462,6 @@ static int user_confirm_reply(struct sock *sk, u16 index, unsigned char *data,
>
> err = cmd_status(sk, index, mgmt_op, 0);
> goto done;
> -
> }
And since you just introduced that all by yourself in your previous
patch, please fix it there and not here.
> cmd = mgmt_pending_add(sk, mgmt_op, hdev, data, len);
> @@ -1482,6 +1481,76 @@ done:
> return err;
> }
>
> +static int user_passkey_reply(struct sock *sk, u16 index, unsigned char *data,
> + u16 len, int success)
> +{
> + struct mgmt_cp_user_passkey_reply *cp = (void *) data;
> + u16 mgmt_op, hci_op;
> + u32 passkey = 0;
> + struct pending_cmd *cmd;
> + struct hci_dev *hdev;
> + struct hci_conn *conn;
> + int expected_len, err = 0;
> +
> + BT_DBG("");
> +
> + if (success) {
> + mgmt_op = MGMT_OP_USER_PASSKEY_REPLY;
> + hci_op = HCI_OP_USER_PASSKEY_REPLY;
> + expected_len = sizeof(*cp);
> + passkey = le32_to_cpu(cp->passkey);
> + } else {
> + mgmt_op = MGMT_OP_USER_PASSKEY_NEG_REPLY;
> + hci_op = HCI_OP_USER_PASSKEY_NEG_REPLY;
> + expected_len = sizeof(struct mgmt_cp_user_passkey_neg_reply);
> + }
> +
> + if (len != expected_len)
> + return cmd_status(sk, index, mgmt_op, EINVAL);
> +
> + hdev = hci_dev_get(index);
> + if (!hdev)
> + return cmd_status(sk, index, mgmt_op, ENODEV);
> +
> + hci_dev_lock_bh(hdev);
> +
> + if (!test_bit(HCI_UP, &hdev->flags)) {
> + err = cmd_status(sk, index, mgmt_op, ENETDOWN);
> + goto done;
> + }
> +
> + conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &cp->bdaddr);
> + if (!conn) {
> + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &cp->bdaddr);
> + if (!conn) {
> + err = cmd_status(sk, index, mgmt_op, ENOTCONN);
> + goto done;
> + }
> +
> + /* Forward Passkey response to SMP */
> +
> + err = cmd_status(sk, index, mgmt_op, 0);
> + goto done;
> + }
Same question as before, why do we have to lookup the ACL_LINK first?
> + cmd = mgmt_pending_add(sk, mgmt_op, hdev, data, len);
> + if (!cmd) {
> + err = -ENOMEM;
> + goto done;
> + }
> +
> + err = hci_send_cmd(hdev, hci_op, len, cp);
> +
> + if (err < 0)
> + mgmt_pending_remove(cmd);
> +
> +done:
> + hci_dev_unlock_bh(hdev);
> + hci_dev_put(hdev);
> +
> + return err;
> +}
> +
> static int set_local_name(struct sock *sk, u16 index, unsigned char *data,
> u16 len)
> {
> @@ -1923,6 +1992,12 @@ int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
> case MGMT_OP_USER_CONFIRM_NEG_REPLY:
> err = user_confirm_reply(sk, index, buf + sizeof(*hdr), len, 0);
> break;
> + case MGMT_OP_USER_PASSKEY_REPLY:
> + err = user_passkey_reply(sk, index, buf + sizeof(*hdr), len, 1);
> + break;
> + case MGMT_OP_USER_PASSKEY_NEG_REPLY:
> + err = user_passkey_reply(sk, index, buf + sizeof(*hdr), len, 0);
> + break;
This overloading from two different messages is actually not a really
good idea. It becomes especially bad with this
struct mgmt_cp_user_passkey_reply *cp = (void *) data;
where one of them is actually a different type, but you cast it anyway.
And I did run a git blame on this and it is mostly Johan's code and you
just follow the existing example, but I don't really like it much. This
needs to be split into two separate function (with no success parameter)
and then just a common tail as helper function.
Regards
Marcel
Hi Brian,
> to enable User Confirmation during LE-SMP pairing.
>
> Signed-off-by: Brian Gix <[email protected]>
> ---
> net/bluetooth/mgmt.c | 22 +++++++++++++++++++---
> 1 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index a6720c6..6c35f8d 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -1423,6 +1423,7 @@ static int user_confirm_reply(struct sock *sk, u16 index, unsigned char *data,
> u16 mgmt_op, hci_op;
> struct pending_cmd *cmd;
> struct hci_dev *hdev;
> + struct hci_conn *conn;
> int err;
>
> BT_DBG("");
> @@ -1446,20 +1447,35 @@ static int user_confirm_reply(struct sock *sk, u16 index, unsigned char *data,
>
> if (!test_bit(HCI_UP, &hdev->flags)) {
> err = cmd_status(sk, index, mgmt_op, ENETDOWN);
> - goto failed;
> + goto done;
> + }
> +
> + conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &cp->bdaddr);
> + if (!conn) {
> + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &cp->bdaddr);
> + if (!conn) {
> + err = cmd_status(sk, index, mgmt_op, ENOTCONN);
> + goto done;
> + }
> +
> + /* Forward Confirm response to SMP */
> +
> + err = cmd_status(sk, index, mgmt_op, 0);
> + goto done;
> +
> }
I am a bit lost why we are trying to find an ACL_LINK here first.
Regards
Marcel
to enable User Confirmation during LE-SMP pairing.
Signed-off-by: Brian Gix <[email protected]>
---
net/bluetooth/mgmt.c | 22 +++++++++++++++++++---
1 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index a6720c6..6c35f8d 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1423,6 +1423,7 @@ static int user_confirm_reply(struct sock *sk, u16 index, unsigned char *data,
u16 mgmt_op, hci_op;
struct pending_cmd *cmd;
struct hci_dev *hdev;
+ struct hci_conn *conn;
int err;
BT_DBG("");
@@ -1446,20 +1447,35 @@ static int user_confirm_reply(struct sock *sk, u16 index, unsigned char *data,
if (!test_bit(HCI_UP, &hdev->flags)) {
err = cmd_status(sk, index, mgmt_op, ENETDOWN);
- goto failed;
+ goto done;
+ }
+
+ conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &cp->bdaddr);
+ if (!conn) {
+ conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &cp->bdaddr);
+ if (!conn) {
+ err = cmd_status(sk, index, mgmt_op, ENOTCONN);
+ goto done;
+ }
+
+ /* Forward Confirm response to SMP */
+
+ err = cmd_status(sk, index, mgmt_op, 0);
+ goto done;
+
}
cmd = mgmt_pending_add(sk, mgmt_op, hdev, data, len);
if (!cmd) {
err = -ENOMEM;
- goto failed;
+ goto done;
}
err = hci_send_cmd(hdev, hci_op, sizeof(cp->bdaddr), &cp->bdaddr);
if (err < 0)
mgmt_pending_remove(cmd);
-failed:
+done:
hci_dev_unlock_bh(hdev);
hci_dev_put(hdev);
--
1.7.7.2
--
Brian Gix
[email protected]
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Some Man-In-The-Middle (MITM) protection schemes require
User Passkey Entry.
Signed-off-by: Brian Gix <[email protected]>
---
include/net/bluetooth/hci.h | 8 ++++
include/net/bluetooth/mgmt.h | 12 ++++++
net/bluetooth/mgmt.c | 77 +++++++++++++++++++++++++++++++++++++++++-
3 files changed, 96 insertions(+), 1 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 139ce2a..ac107b5 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -453,6 +453,14 @@ struct hci_rp_user_confirm_reply {
#define HCI_OP_USER_CONFIRM_NEG_REPLY 0x042d
+#define HCI_OP_USER_PASSKEY_REPLY 0x042e
+struct hci_cp_user_passkey_reply {
+ bdaddr_t bdaddr;
+ __u32 passkey;
+} __packed;
+
+#define HCI_OP_USER_PASSKEY_NEG_REPLY 0x042f
+
#define HCI_OP_REMOTE_OOB_DATA_REPLY 0x0430
struct hci_cp_remote_oob_data_reply {
bdaddr_t bdaddr;
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 3e320c9..aa56bd5 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -228,6 +228,18 @@ struct mgmt_cp_set_fast_connectable {
__u8 enable;
} __packed;
+#define MGMT_OP_USER_PASSKEY_REPLY 0x0020
+struct mgmt_cp_user_passkey_reply {
+ bdaddr_t bdaddr;
+ __le32 passkey;
+} __packed;
+
+#define MGMT_OP_USER_PASSKEY_NEG_REPLY 0x0021
+struct mgmt_cp_user_passkey_neg_reply {
+ bdaddr_t bdaddr;
+} __packed;
+
+
#define MGMT_EV_CMD_COMPLETE 0x0001
struct mgmt_ev_cmd_complete {
__le16 opcode;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 6c35f8d..c6e1ad4 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1462,7 +1462,6 @@ static int user_confirm_reply(struct sock *sk, u16 index, unsigned char *data,
err = cmd_status(sk, index, mgmt_op, 0);
goto done;
-
}
cmd = mgmt_pending_add(sk, mgmt_op, hdev, data, len);
@@ -1482,6 +1481,76 @@ done:
return err;
}
+static int user_passkey_reply(struct sock *sk, u16 index, unsigned char *data,
+ u16 len, int success)
+{
+ struct mgmt_cp_user_passkey_reply *cp = (void *) data;
+ u16 mgmt_op, hci_op;
+ u32 passkey = 0;
+ struct pending_cmd *cmd;
+ struct hci_dev *hdev;
+ struct hci_conn *conn;
+ int expected_len, err = 0;
+
+ BT_DBG("");
+
+ if (success) {
+ mgmt_op = MGMT_OP_USER_PASSKEY_REPLY;
+ hci_op = HCI_OP_USER_PASSKEY_REPLY;
+ expected_len = sizeof(*cp);
+ passkey = le32_to_cpu(cp->passkey);
+ } else {
+ mgmt_op = MGMT_OP_USER_PASSKEY_NEG_REPLY;
+ hci_op = HCI_OP_USER_PASSKEY_NEG_REPLY;
+ expected_len = sizeof(struct mgmt_cp_user_passkey_neg_reply);
+ }
+
+ if (len != expected_len)
+ return cmd_status(sk, index, mgmt_op, EINVAL);
+
+ hdev = hci_dev_get(index);
+ if (!hdev)
+ return cmd_status(sk, index, mgmt_op, ENODEV);
+
+ hci_dev_lock_bh(hdev);
+
+ if (!test_bit(HCI_UP, &hdev->flags)) {
+ err = cmd_status(sk, index, mgmt_op, ENETDOWN);
+ goto done;
+ }
+
+ conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &cp->bdaddr);
+ if (!conn) {
+ conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &cp->bdaddr);
+ if (!conn) {
+ err = cmd_status(sk, index, mgmt_op, ENOTCONN);
+ goto done;
+ }
+
+ /* Forward Passkey response to SMP */
+
+ err = cmd_status(sk, index, mgmt_op, 0);
+ goto done;
+ }
+
+ cmd = mgmt_pending_add(sk, mgmt_op, hdev, data, len);
+ if (!cmd) {
+ err = -ENOMEM;
+ goto done;
+ }
+
+ err = hci_send_cmd(hdev, hci_op, len, cp);
+
+ if (err < 0)
+ mgmt_pending_remove(cmd);
+
+done:
+ hci_dev_unlock_bh(hdev);
+ hci_dev_put(hdev);
+
+ return err;
+}
+
static int set_local_name(struct sock *sk, u16 index, unsigned char *data,
u16 len)
{
@@ -1923,6 +1992,12 @@ int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
case MGMT_OP_USER_CONFIRM_NEG_REPLY:
err = user_confirm_reply(sk, index, buf + sizeof(*hdr), len, 0);
break;
+ case MGMT_OP_USER_PASSKEY_REPLY:
+ err = user_passkey_reply(sk, index, buf + sizeof(*hdr), len, 1);
+ break;
+ case MGMT_OP_USER_PASSKEY_NEG_REPLY:
+ err = user_passkey_reply(sk, index, buf + sizeof(*hdr), len, 0);
+ break;
case MGMT_OP_SET_LOCAL_NAME:
err = set_local_name(sk, index, buf + sizeof(*hdr), len);
break;
--
1.7.7.2
--
Brian Gix
[email protected]
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum