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

[PATCH v2] 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         |   11 +++++++----
 net/bluetooth/hci_event.c        |    2 ++
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index c0b232c..ce7a415 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..fddd0ac 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1330,10 +1330,13 @@ 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 (!conn)
+		return 0;
+
+	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

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

[Marcel Holtmann]

Hi Vishal,

> 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         |   11 +++++++----
>  net/bluetooth/hci_event.c        |    2 ++
>  3 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index c0b232c..ce7a415 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;

I would actually rename this into an action. So something like
flush_key.
 
>  	unsigned int	sent;
>  
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 286f3fc..fddd0ac 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1330,10 +1330,13 @@ 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 (!conn)
> +		return 0;
> +
> +	if (persistent)
> +		conn->temp_link_key = false;
> +	else
> +		conn->temp_link_key = true;

So I would actually prefer a cleanup patch first that changes
hci_persistent_key() function into a bool return value.

After that it could become just like this

	conn->temp_link_key = !persistent;

>  
>  	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);
>  	}

Otherwise this looks all reasonable.

Regards

Marcel

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

[vishal agarwal]

Hi Marcel,

On Thu, Apr 5, 2012 at 9:55 PM, Marcel Holtmann <marcel@holtmann.org> wrote=
:
> Hi Vishal,
>
>> 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>
>> ---
>> =A0include/net/bluetooth/hci_core.h | =A0 =A01 +
>> =A0net/bluetooth/hci_core.c =A0 =A0 =A0 =A0 | =A0 11 +++++++----
>> =A0net/bluetooth/hci_event.c =A0 =A0 =A0 =A0| =A0 =A02 ++
>> =A03 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hc=
i_core.h
>> index c0b232c..ce7a415 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -318,6 +318,7 @@ struct hci_conn {
>>
>> =A0 =A0 =A0 __u8 =A0 =A0 =A0 =A0 =A0 =A0remote_cap;
>> =A0 =A0 =A0 __u8 =A0 =A0 =A0 =A0 =A0 =A0remote_auth;
>> + =A0 =A0 bool =A0 =A0 =A0 =A0 =A0 =A0temp_link_key;
>
> I would actually rename this into an action. So something like
> flush_key.
>
>> =A0 =A0 =A0 unsigned int =A0 =A0sent;
>>
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 286f3fc..fddd0ac 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1330,10 +1330,13 @@ int hci_add_link_key(struct hci_dev *hdev, struc=
t hci_conn *conn, int new_key,
>>
>> =A0 =A0 =A0 mgmt_new_link_key(hdev, key, persistent);
>>
>> - =A0 =A0 if (!persistent) {
>> - =A0 =A0 =A0 =A0 =A0 =A0 list_del(&key->list);
>> - =A0 =A0 =A0 =A0 =A0 =A0 kfree(key);
>> - =A0 =A0 }
>> + =A0 =A0 if (!conn)
>> + =A0 =A0 =A0 =A0 =A0 =A0 return 0;
>> +
>> + =A0 =A0 if (persistent)
>> + =A0 =A0 =A0 =A0 =A0 =A0 conn->temp_link_key =3D false;
>> + =A0 =A0 else
>> + =A0 =A0 =A0 =A0 =A0 =A0 conn->temp_link_key =3D true;
>
> So I would actually prefer a cleanup patch first that changes
> hci_persistent_key() function into a bool return value.
>
> After that it could become just like this
>
> =A0 =A0 =A0 =A0conn->temp_link_key =3D !persistent;
>

As this persistent variable is also used in function mgmt_new_link_key,
which sends this value to bluez as store_hint and bluez stores the key
on the basis of this value. So this will require 2 cleanup patches, one for
bluez and one for kernel. thats what you want?
>>
>> =A0 =A0 =A0 return 0;
>> =A0}
>> 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
>> =A0 =A0 =A0 }
>>
>> =A0 =A0 =A0 if (ev->status =3D=3D 0) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 if (conn->type =3D=3D ACL_LINK && conn->temp_l=
ink_key)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 hci_remove_link_key(hdev, &con=
n->dst);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 hci_proto_disconn_cfm(conn, ev->reason);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 hci_conn_del(conn);
>> =A0 =A0 =A0 }
>
> Otherwise this looks all reasonable.
>
> Regards
>
> Marcel
>
>
> --
> 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 =A0http://vger.kernel.org/majordomo-info.html

Thanks
Vishal

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

[Johan Hedberg]

Hi Vishal,

On Wed, Apr 11, 2012, vishal agarwal wrote:
> > So I would actually prefer a cleanup patch first that changes
> > hci_persistent_key() function into a bool return value.
> >
> > After that it could become just like this
> >
> >        conn->temp_link_key = !persistent;
> >
> 
> As this persistent variable is also used in function mgmt_new_link_key,
> which sends this value to bluez as store_hint and bluez stores the key
> on the basis of this value. So this will require 2 cleanup patches, one for
> bluez and one for kernel. thats what you want?

No, don't change the mgmt protocol or user space. Changing
mgmt_new_link_key to take bool instead of u8 as a parameter does make
sense though and you could have that in the same cleanup patch as the
hci_persistent_key change.

Johan