Return-Path: Subject: Re: [PATCH 2/2] Bluetooth: Add Passkey for SSP & SMP pairing From: Marcel Holtmann To: Brian Gix Cc: linux-bluetooth@vger.kernel.org Date: Thu, 10 Nov 2011 07:33:16 +0900 In-Reply-To: <1320866027-14202-3-git-send-email-bgix@codeaurora.org> References: <1320866027-14202-1-git-send-email-bgix@codeaurora.org> <1320866027-14202-3-git-send-email-bgix@codeaurora.org> Content-Type: text/plain; charset="UTF-8" Message-ID: <1320877999.15441.359.camel@aeonflux> Mime-Version: 1.0 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Brian, > Some Man-In-The-Middle (MITM) protection schemes require > User Passkey Entry. > > Signed-off-by: Brian Gix > --- > 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