[PATCH v2 1/2] Bluetooth: Refactor PIN code rejection to use user_pairing_resp()

[PATCH v2 1/2] Bluetooth: Refactor PIN code rejection to use user_pairing_resp()

[Jaganath Kanakkassery]

Reuse user_pairing_resp() to send PIN code negative reply.

Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com>
---
 net/bluetooth/mgmt.c |  155 +++++++++++++++++++++++---------------------------
 1 files changed, 72 insertions(+), 83 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index e83368f..5a442b9 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1718,89 +1718,6 @@ static int send_pin_code_neg_reply(struct sock *sk, struct hci_dev *hdev,
 	return err;
 }
 
-static int pin_code_reply(struct sock *sk, struct hci_dev *hdev, void *data,
-			  u16 len)
-{
-	struct hci_conn *conn;
-	struct mgmt_cp_pin_code_reply *cp = data;
-	struct hci_cp_pin_code_reply reply;
-	struct pending_cmd *cmd;
-	int err;
-
-	BT_DBG("");
-
-	hci_dev_lock(hdev);
-
-	if (!hdev_is_powered(hdev)) {
-		err = cmd_status(sk, hdev->id, MGMT_OP_PIN_CODE_REPLY,
-				 MGMT_STATUS_NOT_POWERED);
-		goto failed;
-	}
-
-	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &cp->addr.bdaddr);
-	if (!conn) {
-		err = cmd_status(sk, hdev->id, MGMT_OP_PIN_CODE_REPLY,
-				 MGMT_STATUS_NOT_CONNECTED);
-		goto failed;
-	}
-
-	if (conn->pending_sec_level == BT_SECURITY_HIGH && cp->pin_len != 16) {
-		struct mgmt_cp_pin_code_neg_reply ncp;
-
-		memcpy(&ncp.addr, &cp->addr, sizeof(ncp.addr));
-
-		BT_ERR("PIN code is not 16 bytes long");
-
-		err = send_pin_code_neg_reply(sk, hdev, &ncp);
-		if (err >= 0)
-			err = cmd_status(sk, hdev->id, MGMT_OP_PIN_CODE_REPLY,
-					 MGMT_STATUS_INVALID_PARAMS);
-
-		goto failed;
-	}
-
-	cmd = mgmt_pending_add(sk, MGMT_OP_PIN_CODE_REPLY, hdev, data, len);
-	if (!cmd) {
-		err = -ENOMEM;
-		goto failed;
-	}
-
-	bacpy(&reply.bdaddr, &cp->addr.bdaddr);
-	reply.pin_len = cp->pin_len;
-	memcpy(reply.pin_code, cp->pin_code, sizeof(reply.pin_code));
-
-	err = hci_send_cmd(hdev, HCI_OP_PIN_CODE_REPLY, sizeof(reply), &reply);
-	if (err < 0)
-		mgmt_pending_remove(cmd);
-
-failed:
-	hci_dev_unlock(hdev);
-	return err;
-}
-
-static int pin_code_neg_reply(struct sock *sk, struct hci_dev *hdev,
-			      void *data, u16 len)
-{
-	struct mgmt_cp_pin_code_neg_reply *cp = data;
-	int err;
-
-	BT_DBG("");
-
-	hci_dev_lock(hdev);
-
-	if (!hdev_is_powered(hdev)) {
-		err = cmd_status(sk, hdev->id, MGMT_OP_PIN_CODE_NEG_REPLY,
-				 MGMT_STATUS_NOT_POWERED);
-		goto failed;
-	}
-
-	err = send_pin_code_neg_reply(sk, hdev, cp);
-
-failed:
-	hci_dev_unlock(hdev);
-	return err;
-}
-
 static int set_io_capability(struct sock *sk, struct hci_dev *hdev, void *data,
 			     u16 len)
 {
@@ -2083,6 +2000,78 @@ done:
 	return err;
 }
 
+static int pin_code_reply(struct sock *sk, struct hci_dev *hdev, void *data,
+			  u16 len)
+{
+	struct hci_conn *conn;
+	struct mgmt_cp_pin_code_reply *cp = data;
+	struct hci_cp_pin_code_reply reply;
+	struct pending_cmd *cmd;
+	int err;
+
+	BT_DBG("");
+
+	hci_dev_lock(hdev);
+
+	if (!hdev_is_powered(hdev)) {
+		err = cmd_status(sk, hdev->id, MGMT_OP_PIN_CODE_REPLY,
+				 MGMT_STATUS_NOT_POWERED);
+		goto failed;
+	}
+
+	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &cp->addr.bdaddr);
+	if (!conn) {
+		err = cmd_status(sk, hdev->id, MGMT_OP_PIN_CODE_REPLY,
+				 MGMT_STATUS_NOT_CONNECTED);
+		goto failed;
+	}
+
+	if (conn->pending_sec_level == BT_SECURITY_HIGH && cp->pin_len != 16) {
+		struct mgmt_cp_pin_code_neg_reply ncp;
+
+		memcpy(&ncp.addr, &cp->addr, sizeof(ncp.addr));
+
+		BT_ERR("PIN code is not 16 bytes long");
+
+		err = send_pin_code_neg_reply(sk, hdev, &ncp);
+		if (err >= 0)
+			err = cmd_status(sk, hdev->id, MGMT_OP_PIN_CODE_REPLY,
+					 MGMT_STATUS_INVALID_PARAMS);
+
+		goto failed;
+	}
+
+	cmd = mgmt_pending_add(sk, MGMT_OP_PIN_CODE_REPLY, hdev, data, len);
+	if (!cmd) {
+		err = -ENOMEM;
+		goto failed;
+	}
+
+	bacpy(&reply.bdaddr, &cp->addr.bdaddr);
+	reply.pin_len = cp->pin_len;
+	memcpy(reply.pin_code, cp->pin_code, sizeof(reply.pin_code));
+
+	err = hci_send_cmd(hdev, HCI_OP_PIN_CODE_REPLY, sizeof(reply), &reply);
+	if (err < 0)
+		mgmt_pending_remove(cmd);
+
+failed:
+	hci_dev_unlock(hdev);
+	return err;
+}
+
+static int pin_code_neg_reply(struct sock *sk, struct hci_dev *hdev,
+			      void *data, u16 len)
+{
+	struct mgmt_cp_pin_code_neg_reply *cp = data;
+
+	BT_DBG("");
+
+	return user_pairing_resp(sk, hdev, &cp->addr.bdaddr, cp->addr.type,
+				MGMT_OP_PIN_CODE_NEG_REPLY,
+				HCI_OP_PIN_CODE_NEG_REPLY, 0);
+}
+
 static int user_confirm_reply(struct sock *sk, struct hci_dev *hdev, void *data,
 			      u16 len)
 {
-- 
1.7.1

[PATCH v2 2/2] Bluetooth: Override status if local user rejects pairing

[Jaganath Kanakkassery]

In case if pairing is failed, user cannot differentiate from the status
whether failure is caused by local rejection or remote rejection
because authentication failure is coming in both the cases.

This patch overrides the status from controller with "Rejected"
in case of local rejection.

This patch will be useful if user can take some action based on local
or remote rejection

Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com>
---
 include/net/bluetooth/hci_core.h |    4 +++-
 net/bluetooth/hci_event.c        |    4 ++--
 net/bluetooth/mgmt.c             |   18 ++++++++++++++++--
 net/bluetooth/smp.c              |    2 +-
 4 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 475b8c0..bbb295f 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -312,6 +312,8 @@ struct hci_conn {
 	__u16		disc_timeout;
 	unsigned long	flags;
 
+	bool		auth_rejected;
+
 	__u8		remote_cap;
 	__u8		remote_auth;
 	bool		flush_key;
@@ -1036,7 +1038,7 @@ int mgmt_user_passkey_reply_complete(struct hci_dev *hdev, bdaddr_t *bdaddr,
 int mgmt_user_passkey_neg_reply_complete(struct hci_dev *hdev, bdaddr_t *bdaddr,
 					 u8 link_type, u8 addr_type, u8 status);
 int mgmt_auth_failed(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
-		     u8 addr_type, u8 status);
+		     u8 addr_type, u8 status, bool auth_rejected);
 int mgmt_auth_enable_complete(struct hci_dev *hdev, u8 status);
 int mgmt_ssp_enable_complete(struct hci_dev *hdev, u8 enable, u8 status);
 int mgmt_set_class_of_dev_complete(struct hci_dev *hdev, u8 *dev_class,
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 1ba929c..d5b4655 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1948,7 +1948,7 @@ static void hci_auth_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		}
 	} else {
 		mgmt_auth_failed(hdev, &conn->dst, conn->type, conn->dst_type,
-				 ev->status);
+				 ev->status, conn->auth_rejected);
 	}
 
 	clear_bit(HCI_CONN_AUTH_PEND, &conn->flags);
@@ -3281,7 +3281,7 @@ static void hci_simple_pair_complete_evt(struct hci_dev *hdev,
 	 * the mgmt_auth_failed event */
 	if (!test_bit(HCI_CONN_AUTH_PEND, &conn->flags) && ev->status != 0)
 		mgmt_auth_failed(hdev, &conn->dst, conn->type, conn->dst_type,
-				 ev->status);
+				 ev->status, conn->auth_rejected);
 
 	hci_conn_put(conn);
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 5a442b9..4fc3379 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1764,6 +1764,10 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
 	bacpy(&rp.addr.bdaddr, &conn->dst);
 	rp.addr.type = link_to_bdaddr(conn->type, conn->dst_type);
 
+	/* Override status if local device rejected pairing */
+	if (conn->auth_rejected == true)
+		status = MGMT_STATUS_REJECTED;
+
 	cmd_complete(cmd->sk, cmd->index, MGMT_OP_PAIR_DEVICE, status,
 		     &rp, sizeof(rp));
 
@@ -1962,6 +1966,11 @@ static int user_pairing_resp(struct sock *sk, struct hci_dev *hdev,
 		goto done;
 	}
 
+	if (hci_op == HCI_OP_USER_CONFIRM_NEG_REPLY ||
+				hci_op == HCI_OP_USER_PASSKEY_NEG_REPLY ||
+				hci_op == HCI_OP_PIN_CODE_NEG_REPLY)
+		conn->auth_rejected = true;
+
 	if (type == BDADDR_LE_PUBLIC || type == BDADDR_LE_RANDOM) {
 		/* Continue with pairing via SMP */
 		err = smp_user_confirm_reply(conn, mgmt_op, passkey);
@@ -3260,13 +3269,18 @@ int mgmt_user_passkey_neg_reply_complete(struct hci_dev *hdev, bdaddr_t *bdaddr,
 }
 
 int mgmt_auth_failed(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
-		     u8 addr_type, u8 status)
+		     u8 addr_type, u8 status, bool auth_rejected)
 {
 	struct mgmt_ev_auth_failed ev;
 
 	bacpy(&ev.addr.bdaddr, bdaddr);
 	ev.addr.type = link_to_bdaddr(link_type, addr_type);
-	ev.status = mgmt_status(status);
+
+	/* Override status if local device rejected pairing */
+	if (auth_rejected == true)
+		ev.status = MGMT_STATUS_REJECTED;
+	else
+		ev.status = mgmt_status(status);
 
 	return mgmt_event(MGMT_EV_AUTH_FAILED, hdev, &ev, sizeof(ev), NULL);
 }
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 16ef0dc..3c04313 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -265,7 +265,7 @@ static void smp_failure(struct l2cap_conn *conn, u8 reason, u8 send)
 
 	clear_bit(HCI_CONN_ENCRYPT_PEND, &conn->hcon->flags);
 	mgmt_auth_failed(conn->hcon->hdev, conn->dst, hcon->type,
-			 hcon->dst_type, reason);
+			 hcon->dst_type, reason, hcon->auth_rejected);
 
 	if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags)) {
 		cancel_delayed_work_sync(&conn->security_timer);
-- 
1.7.1

Re: [PATCH v2 1/2] Bluetooth: Refactor PIN code rejection to use user_pairing_resp()

[Johan Hedberg]

Hi Jaganath,

On Fri, Jul 06, 2012, Jaganath Kanakkassery wrote:
> Reuse user_pairing_resp() to send PIN code negative reply.
> 
> Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com>
> ---
>  net/bluetooth/mgmt.c |  155 +++++++++++++++++++++++---------------------------
>  1 files changed, 72 insertions(+), 83 deletions(-)

Would it be possible for you to do this patch without moving around the
functions? It'd make it easier to see that only the relevant bits are
changed.

Johan

Re: [PATCH v2 2/2] Bluetooth: Override status if local user rejects pairing

[Johan Hedberg]

Hi Jaganath,

On Fri, Jul 06, 2012, Jaganath Kanakkassery wrote:
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 5a442b9..4fc3379 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -1764,6 +1764,10 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
>  	bacpy(&rp.addr.bdaddr, &conn->dst);
>  	rp.addr.type = link_to_bdaddr(conn->type, conn->dst_type);
>  
> +	/* Override status if local device rejected pairing */
> +	if (conn->auth_rejected == true)
> +		status = MGMT_STATUS_REJECTED;

I think simply "if (conn->auth_rejected)" should be fine (no "== true").
And what if status == 0 and this is a repairing over the same hci_conn
which was previously rejected? Seems like you'd give a false negative in
that case. Maybe the check should be "if (status && conn->auth_rejected)".

> +	/* Override status if local device rejected pairing */
> +	if (auth_rejected == true)

Same thing again with the comparison. The stuff inside () of an
if-statement should be a valid boolean, and if the standard bool type by
itself can't be considered that then I don't know what can.

The thing that's worrying me is that there's nowhere where you clear
conn->auth_rejected. If a re-authentication is attempted with the same
hci_conn the code would be doing wrong things. I'm not completely sure
where this clearing should occur since we're potentially sending two
mgmt events through two different code paths (pairing_complete and
mgmt_auth_failed) so clearing in either one might be risky in that it
causes the second function to do the wrong thing.

Johan

Re: [PATCH v2 1/2] Bluetooth: Refactor PIN code rejection to use user_pairing_resp()

[Jaganath]

Hi Johan,

--------------------------------------------------
From: "Johan Hedberg" <johan.hedberg@gmail.com>
Sent: Friday, July 06, 2012 2:48 PM
To: "Jaganath Kanakkassery" <jaganath.k@samsung.com>
Cc: <linux-bluetooth@vger.kernel.org>
Subject: Re: [PATCH v2 1/2] Bluetooth: Refactor PIN code rejection to use 
user_pairing_resp()

> Hi Jaganath,
>
> On Fri, Jul 06, 2012, Jaganath Kanakkassery wrote:
>> Reuse user_pairing_resp() to send PIN code negative reply.
>>
>> Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com>
>> ---
>>  net/bluetooth/mgmt.c |  155 
>> +++++++++++++++++++++++---------------------------
>>  1 files changed, 72 insertions(+), 83 deletions(-)
>
> Would it be possible for you to do this patch without moving around the
> functions? It'd make it easier to see that only the relevant bits are
> changed.

Ok, I will the revert movement of the function pin_code_reply() and raise 
v3.

Thanks,
Jaganath 

Re: [PATCH v2 2/2] Bluetooth: Override status if local user rejects pairing

[Jaganath]

Hi Johan,

--------------------------------------------------
From: "Johan Hedberg" <johan.hedberg@gmail.com>
Sent: Friday, July 06, 2012 2:55 PM
To: "Jaganath Kanakkassery" <jaganath.k@samsung.com>
Cc: <linux-bluetooth@vger.kernel.org>
Subject: Re: [PATCH v2 2/2] Bluetooth: Override status if local user rejects 
pairing

> Hi Jaganath,
>
> On Fri, Jul 06, 2012, Jaganath Kanakkassery wrote:
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index 5a442b9..4fc3379 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -1764,6 +1764,10 @@ static void pairing_complete(struct pending_cmd 
>> *cmd, u8 status)
>>  bacpy(&rp.addr.bdaddr, &conn->dst);
>>  rp.addr.type = link_to_bdaddr(conn->type, conn->dst_type);
>>
>> + /* Override status if local device rejected pairing */
>> + if (conn->auth_rejected == true)
>> + status = MGMT_STATUS_REJECTED;
>
> I think simply "if (conn->auth_rejected)" should be fine (no "== true").
> And what if status == 0 and this is a repairing over the same hci_conn
> which was previously rejected? Seems like you'd give a false negative in
> that case. Maybe the check should be "if (status && conn->auth_rejected)".
>> + /* Override status if local device rejected pairing */
>> + if (auth_rejected == true)
>
> Same thing again with the comparison. The stuff inside () of an
> if-statement should be a valid boolean, and if the standard bool type by
> itself can't be considered that then I don't know what can.
>
> The thing that's worrying me is that there's nowhere where you clear
> conn->auth_rejected. If a re-authentication is attempted with the same
> hci_conn the code would be doing wrong things. I'm not completely sure
> where this clearing should occur since we're potentially sending two
> mgmt events through two different code paths (pairing_complete and
> mgmt_auth_failed) so clearing in either one might be risky in that it
> causes the second function to do the wrong thing.

I think we can change setting auth_rejected to true only if the pairing is
local initiated and reset it in pairing_complete (). Since 
mgmt_auth_failed()
will be called before pairing_complete () it will be fine I think. Please 
let me
know your view on that.

Thanks,
Jaganath 

Re: [PATCH v2 2/2] Bluetooth: Override status if local user rejects pairing

[Johan Hedberg]

Hi Jaganath,

On Sat, Jul 07, 2012, Jaganath wrote:
> I think we can change setting auth_rejected to true only if the
> pairing is local initiated and reset it in pairing_complete (). Since
> mgmt_auth_failed() will be called before pairing_complete () it will
> be fine I think.  Please let me know your view on that.

I think this should be consistent regardless of which side triggered the
pairing, i.e. use STATUS_REJECTED always when our agent rejects it.

Johan