* [PATCH 0/2] Add SMP MITM hooks into MGMTOPS @ 2011-11-09 19:13 Brian Gix 2011-11-09 19:13 ` [PATCH 1/2] Bluetooth: Add SMP support to user_confirm_reply Brian Gix 2011-11-09 19:13 ` [PATCH 2/2] Bluetooth: Add Passkey for SSP & SMP pairing Brian Gix 0 siblings, 2 replies; 7+ messages in thread From: Brian Gix @ 2011-11-09 19:13 UTC (permalink / raw) To: linux-bluetooth 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 bgix@codeaurora.org Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] Bluetooth: Add SMP support to user_confirm_reply 2011-11-09 19:13 [PATCH 0/2] Add SMP MITM hooks into MGMTOPS Brian Gix @ 2011-11-09 19:13 ` Brian Gix 2011-11-09 22:26 ` Marcel Holtmann 2011-11-09 19:13 ` [PATCH 2/2] Bluetooth: Add Passkey for SSP & SMP pairing Brian Gix 1 sibling, 1 reply; 7+ messages in thread From: Brian Gix @ 2011-11-09 19:13 UTC (permalink / raw) To: linux-bluetooth; +Cc: Brian Gix to enable User Confirmation during LE-SMP pairing. Signed-off-by: Brian Gix <bgix@codeaurora.org> --- 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 bgix@codeaurora.org Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] Bluetooth: Add SMP support to user_confirm_reply 2011-11-09 19:13 ` [PATCH 1/2] Bluetooth: Add SMP support to user_confirm_reply Brian Gix @ 2011-11-09 22:26 ` Marcel Holtmann 2011-11-09 22:34 ` Brian Gix 0 siblings, 1 reply; 7+ messages in thread From: Marcel Holtmann @ 2011-11-09 22:26 UTC (permalink / raw) To: Brian Gix; +Cc: linux-bluetooth Hi Brian, > to enable User Confirmation during LE-SMP pairing. > > Signed-off-by: Brian Gix <bgix@codeaurora.org> > --- > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] Bluetooth: Add SMP support to user_confirm_reply 2011-11-09 22:26 ` Marcel Holtmann @ 2011-11-09 22:34 ` Brian Gix 2011-11-09 22:44 ` Marcel Holtmann 0 siblings, 1 reply; 7+ messages in thread From: Brian Gix @ 2011-11-09 22:34 UTC (permalink / raw) To: Marcel Holtmann; +Cc: linux-bluetooth 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 bgix@codeaurora.org Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] Bluetooth: Add SMP support to user_confirm_reply 2011-11-09 22:34 ` Brian Gix @ 2011-11-09 22:44 ` Marcel Holtmann 0 siblings, 0 replies; 7+ messages in thread From: Marcel Holtmann @ 2011-11-09 22:44 UTC (permalink / raw) To: Brian Gix; +Cc: linux-bluetooth 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] Bluetooth: Add Passkey for SSP & SMP pairing 2011-11-09 19:13 [PATCH 0/2] Add SMP MITM hooks into MGMTOPS Brian Gix 2011-11-09 19:13 ` [PATCH 1/2] Bluetooth: Add SMP support to user_confirm_reply Brian Gix @ 2011-11-09 19:13 ` Brian Gix 2011-11-09 22:33 ` Marcel Holtmann 1 sibling, 1 reply; 7+ messages in thread From: Brian Gix @ 2011-11-09 19:13 UTC (permalink / raw) To: linux-bluetooth; +Cc: Brian Gix Some Man-In-The-Middle (MITM) protection schemes require User Passkey Entry. Signed-off-by: Brian Gix <bgix@codeaurora.org> --- 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 bgix@codeaurora.org Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Bluetooth: Add Passkey for SSP & SMP pairing 2011-11-09 19:13 ` [PATCH 2/2] Bluetooth: Add Passkey for SSP & SMP pairing Brian Gix @ 2011-11-09 22:33 ` Marcel Holtmann 0 siblings, 0 replies; 7+ messages in thread From: Marcel Holtmann @ 2011-11-09 22:33 UTC (permalink / raw) To: Brian Gix; +Cc: linux-bluetooth Hi Brian, > Some Man-In-The-Middle (MITM) protection schemes require > User Passkey Entry. > > Signed-off-by: Brian Gix <bgix@codeaurora.org> > --- > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-11-09 22:44 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-11-09 19:13 [PATCH 0/2] Add SMP MITM hooks into MGMTOPS Brian Gix 2011-11-09 19:13 ` [PATCH 1/2] Bluetooth: Add SMP support to user_confirm_reply Brian Gix 2011-11-09 22:26 ` Marcel Holtmann 2011-11-09 22:34 ` Brian Gix 2011-11-09 22:44 ` Marcel Holtmann 2011-11-09 19:13 ` [PATCH 2/2] Bluetooth: Add Passkey for SSP & SMP pairing Brian Gix 2011-11-09 22:33 ` Marcel Holtmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).