[PATCH] Bluetooth: Temporary keys should be retained during connection

[PATCH] Bluetooth: Temporary keys should be retained during connection

[Vishal Agarwal]

If a key is non persistent then it should not be used in future
connections but it should be kept for current connection. And it
should be removed when connecion is removed.

Signed-off-by: Vishal Agarwal <vishal.agarwal@stericsson.com>
---
 include/net/bluetooth/hci_core.h |    1 +
 net/bluetooth/hci_core.c         |    8 ++++----
 net/bluetooth/mgmt.c             |    6 ++++++
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index c8d5beb..6c2d436 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -318,6 +318,7 @@ struct hci_conn {
 
 	__u8		remote_cap;
 	__u8		remote_auth;
+	bool		temp_link_key;
 
 	unsigned int	sent;
 
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 286f3fc..6542b03 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1330,10 +1330,10 @@ int hci_add_link_key(struct hci_dev *hdev, struct hci_conn *conn, int new_key,
 
 	mgmt_new_link_key(hdev, key, persistent);
 
-	if (!persistent) {
-		list_del(&key->list);
-		kfree(key);
-	}
+	if (persistent)
+		conn->temp_link_key = false;
+	else
+		conn->temp_link_key = true;
 
 	return 0;
 }
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index b4f7e32..bc6f89e 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3060,12 +3060,18 @@ int mgmt_device_disconnected(struct hci_dev *hdev, bdaddr_t *bdaddr,
 {
 	struct mgmt_addr_info ev;
 	struct sock *sk = NULL;
+	struct hci_conn *conn;
 	int err;
 
 	mgmt_pending_foreach(MGMT_OP_DISCONNECT, hdev, disconnect_rsp, &sk);
 
 	bacpy(&ev.bdaddr, bdaddr);
 	ev.type = link_to_mgmt(link_type, addr_type);
+	if (link_type == ACL_LINK) {
+		conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, bdaddr);
+		if (conn->temp_link_key)
+			hci_remove_link_key(hdev, bdaddr);
+	}
 
 	err = mgmt_event(MGMT_EV_DEVICE_DISCONNECTED, hdev, &ev, sizeof(ev),
 			 sk);
-- 
1.7.0.4

Re: [PATCH] Bluetooth: Temporary keys should be retained during connection

[Johan Hedberg]

Hi Vishal,

On Wed, Apr 04, 2012, Vishal Agarwal wrote:
> If a key is non persistent then it should not be used in future
> connections but it should be kept for current connection. And it
> should be removed when connecion is removed.
> 
> Signed-off-by: Vishal Agarwal <vishal.agarwal@stericsson.com>
> ---
>  include/net/bluetooth/hci_core.h |    1 +
>  net/bluetooth/hci_core.c         |    8 ++++----
>  net/bluetooth/mgmt.c             |    6 ++++++
>  3 files changed, 11 insertions(+), 4 deletions(-)

Firstly, did you verify that this fixes your test case? You still didn't
tell us what test case this is, btw.

> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1330,10 +1330,10 @@ int hci_add_link_key(struct hci_dev *hdev, struct hci_conn *conn, int new_key,
>  
>  	mgmt_new_link_key(hdev, key, persistent);
>  
> -	if (!persistent) {
> -		list_del(&key->list);
> -		kfree(key);
> -	}
> +	if (persistent)
> +		conn->temp_link_key = false;
> +	else
> +		conn->temp_link_key = true;
>  
>  	return 0;
>  }
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index b4f7e32..bc6f89e 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -3060,12 +3060,18 @@ int mgmt_device_disconnected(struct hci_dev *hdev, bdaddr_t *bdaddr,
>  {
>  	struct mgmt_addr_info ev;
>  	struct sock *sk = NULL;
> +	struct hci_conn *conn;
>  	int err;
>  
>  	mgmt_pending_foreach(MGMT_OP_DISCONNECT, hdev, disconnect_rsp, &sk);
>  
>  	bacpy(&ev.bdaddr, bdaddr);
>  	ev.type = link_to_mgmt(link_type, addr_type);
> +	if (link_type == ACL_LINK) {
> +		conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, bdaddr);
> +		if (conn->temp_link_key)
> +			hci_remove_link_key(hdev, bdaddr);
> +	}
>  
>  	err = mgmt_event(MGMT_EV_DEVICE_DISCONNECTED, hdev, &ev, sizeof(ev),
>  			 sk);

Since setting the flag is outside of mgmt.c I think the removal should
also be. That way you also avoid an extra call to
hci_conn_hash_lookup_ba. I.e. please put the removal in
hci_disconn_complete_evt.

I'd also still like to hear your opinion of the second option I
proposed. If you had a reference to struct link_key in hci_conn then
you'd just need to call list_del() and nothing else to remove it (i.e.
no iteration of hdev->link_keys necessary.

Johan

Re: [PATCH] Bluetooth: Temporary keys should be retained during connection

[vishal agarwal]

Hi Johan,

On Wed, Apr 4, 2012 at 5:23 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Vishal,
>
> On Wed, Apr 04, 2012, Vishal Agarwal wrote:
>> If a key is non persistent then it should not be used in future
>> connections but it should be kept for current connection. And it
>> should be removed when connecion is removed.
>>
>> Signed-off-by: Vishal Agarwal <vishal.agarwal@stericsson.com>
>> ---
>>  include/net/bluetooth/hci_core.h |    1 +
>>  net/bluetooth/hci_core.c         |    8 ++++----
>>  net/bluetooth/mgmt.c             |    6 ++++++
>>  3 files changed, 11 insertions(+), 4 deletions(-)
>
> Firstly, did you verify that this fixes your test case? You still didn't
> tell us what test case this is, btw.
>
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1330,10 +1330,10 @@ int hci_add_link_key(struct hci_dev *hdev, struct hci_conn *conn, int new_key,
>>
>>       mgmt_new_link_key(hdev, key, persistent);
>>
>> -     if (!persistent) {
>> -             list_del(&key->list);
>> -             kfree(key);
>> -     }
>> +     if (persistent)
>> +             conn->temp_link_key = false;
>> +     else
>> +             conn->temp_link_key = true;
>>
>>       return 0;
>>  }
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index b4f7e32..bc6f89e 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -3060,12 +3060,18 @@ int mgmt_device_disconnected(struct hci_dev *hdev, bdaddr_t *bdaddr,
>>  {
>>       struct mgmt_addr_info ev;
>>       struct sock *sk = NULL;
>> +     struct hci_conn *conn;
>>       int err;
>>
>>       mgmt_pending_foreach(MGMT_OP_DISCONNECT, hdev, disconnect_rsp, &sk);
>>
>>       bacpy(&ev.bdaddr, bdaddr);
>>       ev.type = link_to_mgmt(link_type, addr_type);
>> +     if (link_type == ACL_LINK) {
>> +             conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, bdaddr);
>> +             if (conn->temp_link_key)
>> +                     hci_remove_link_key(hdev, bdaddr);
>> +     }
>>
>>       err = mgmt_event(MGMT_EV_DEVICE_DISCONNECTED, hdev, &ev, sizeof(ev),
>>                        sk);
>
> Since setting the flag is outside of mgmt.c I think the removal should
> also be. That way you also avoid an extra call to
> hci_conn_hash_lookup_ba. I.e. please put the removal in
> hci_disconn_complete_evt.
>
> I'd also still like to hear your opinion of the second option I
> proposed. If you had a reference to struct link_key in hci_conn then
> you'd just need to call list_del() and nothing else to remove it (i.e.
> no iteration of hdev->link_keys necessary.
>
If I implement it this way then there will be two new variables added, one in
hci_conn to store the reference of key and other one is inside
link_key structure
to store if key is temporary or not.
or you want me to store reference of key to hci_conn only when the key
is temporary?
in this case also code might become complicated to handle cases if key
is re generated
and new key is not temporary but the older one was.
So in my opinion after the changes you suggested (moving code in
hci_disconn_complete_evt),
this is also OK. lesser and clearer code.
> Johan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thanks
Vishal

Re: [PATCH] Bluetooth: Temporary keys should be retained during connection

[Johan Hedberg]

Hi Vishal,

On Wed, Apr 04, 2012, vishal agarwal wrote:
> > Firstly, did you verify that this fixes your test case? You still
> > didn't tell us what test case this is, btw.

What about the above?

> > Since setting the flag is outside of mgmt.c I think the removal should
> > also be. That way you also avoid an extra call to
> > hci_conn_hash_lookup_ba. I.e. please put the removal in
> > hci_disconn_complete_evt.
> >
> > I'd also still like to hear your opinion of the second option I
> > proposed. If you had a reference to struct link_key in hci_conn then
> > you'd just need to call list_del() and nothing else to remove it (i.e.
> > no iteration of hdev->link_keys necessary.
> 
> If I implement it this way then there will be two new variables added,
> one in hci_conn to store the reference of key and other one is inside
> link_key structure to store if key is temporary or not.
> or you want me to store reference of key to hci_conn only when the key
> is temporary?
> in this case also code might become complicated to handle cases if key
> is re generated and new key is not temporary but the older one was.
>
> So in my opinion after the changes you suggested (moving code in
> hci_disconn_complete_evt), this is also OK. lesser and clearer code.

Ok, fair enough. The hci_disconn_complete_evt change should be enough
then.

Johan

Re: [PATCH] Bluetooth: Temporary keys should be retained during connection

[vishal agarwal]

Hi Johan,

On Wed, Apr 4, 2012 at 5:52 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Vishal,
>
> On Wed, Apr 04, 2012, vishal agarwal wrote:
>> > Firstly, did you verify that this fixes your test case? You still
>> > didn't tell us what test case this is, btw.
>
> What about the above?
>
Yes with this PTS testcase is passing now, the testcase which was failing
earlier TC_PSE_SSM_BI_02_C (test case for PBAP server)

>> > Since setting the flag is outside of mgmt.c I think the removal should
>> > also be. That way you also avoid an extra call to
>> > hci_conn_hash_lookup_ba. I.e. please put the removal in
>> > hci_disconn_complete_evt.
>> >
>> > I'd also still like to hear your opinion of the second option I
>> > proposed. If you had a reference to struct link_key in hci_conn then
>> > you'd just need to call list_del() and nothing else to remove it (i.e.
>> > no iteration of hdev->link_keys necessary.
>>
>> If I implement it this way then there will be two new variables added,
>> one in hci_conn to store the reference of key and other one is inside
>> link_key structure to store if key is temporary or not.
>> or you want me to store reference of key to hci_conn only when the key
>> is temporary?
>> in this case also code might become complicated to handle cases if key
>> is re generated and new key is not temporary but the older one was.
>>
>> So in my opinion after the changes you suggested (moving code in
>> hci_disconn_complete_evt), this is also OK. lesser and clearer code.
>
> Ok, fair enough. The hci_disconn_complete_evt change should be enough
> then.
>
> Johan

Vishal

Re: [PATCH] Bluetooth: Temporary keys should be retained during connection

[Johan Hedberg]

Hi Vishal,

On Wed, Apr 04, 2012, Vishal Agarwal wrote:
> @@ -1330,10 +1330,10 @@ int hci_add_link_key(struct hci_dev *hdev, struct hci_conn *conn, int new_key,
>  
>  	mgmt_new_link_key(hdev, key, persistent);
>  
> -	if (!persistent) {
> -		list_del(&key->list);
> -		kfree(key);
> -	}
> +	if (persistent)
> +		conn->temp_link_key = false;
> +	else
> +		conn->temp_link_key = true;

The hci_add_link_key function can be called with conn == NULL so you
need to take this into account before dereferencing it.

Johan

[PATCH] Bluetooth: Temporary keys should be retained during connection

[Vishal Agarwal]

If a key is non persistent then it should not be used in future connections
but it should be kept for current connection. And it should be removed when
connecion is removed.
Signed-off-by: Vishal Agarwal <vishal.agarwal@stericsson.com>
---
 include/net/bluetooth/hci_core.h |    1 +
 net/bluetooth/hci_core.c         |    8 ++++----
 net/bluetooth/hci_event.c        |    2 ++
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index c8d5beb..6c2d436 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -318,6 +318,7 @@ struct hci_conn {
 
 	__u8		remote_cap;
 	__u8		remote_auth;
+	bool		temp_link_key;
 
 	unsigned int	sent;
 
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 286f3fc..6542b03 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1330,10 +1330,10 @@ int hci_add_link_key(struct hci_dev *hdev, struct hci_conn *conn, int new_key,
 
 	mgmt_new_link_key(hdev, key, persistent);
 
-	if (!persistent) {
-		list_del(&key->list);
-		kfree(key);
-	}
+	if (persistent)
+		conn->temp_link_key = false;
+	else
+		conn->temp_link_key = true;
 
 	return 0;
 }
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 7325300..0b19852 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1928,6 +1928,8 @@ static inline void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff
 	}
 
 	if (ev->status == 0) {
+		if (conn->type == ACL_LINK && conn->temp_link_key)
+			hci_remove_link_key(hdev, &conn->dst);
 		hci_proto_disconn_cfm(conn, ev->reason);
 		hci_conn_del(conn);
 	}
-- 
1.7.0.4