2012-04-13 09:01:14

by Vishal Agarwal

[permalink] [raw]
Subject: [PATCH 1/2] Bluetooth: hci_persistent_key should return bool

This patch changes the return type of function hci_persistent_key
from int to bool.
Signed-off-by: Vishal Agarwal <[email protected]>
---
include/net/bluetooth/hci_core.h | 2 +-
net/bluetooth/hci_core.c | 21 +++++++++++----------
net/bluetooth/mgmt.c | 2 +-
3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index c0b232c..8a9abe2 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -983,7 +983,7 @@ int mgmt_discoverable(struct hci_dev *hdev, u8 discoverable);
int mgmt_connectable(struct hci_dev *hdev, u8 connectable);
int mgmt_write_scan_failed(struct hci_dev *hdev, u8 scan, u8 status);
int mgmt_new_link_key(struct hci_dev *hdev, struct link_key *key,
- u8 persistent);
+ bool persistent);
int mgmt_device_connected(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
u8 addr_type, u32 flags, u8 *name, u8 name_len,
u8 *dev_class);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 52c7abf..703c28d 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1219,40 +1219,40 @@ struct link_key *hci_find_link_key(struct hci_dev *hdev, bdaddr_t *bdaddr)
return NULL;
}

-static int hci_persistent_key(struct hci_dev *hdev, struct hci_conn *conn,
+static bool hci_persistent_key(struct hci_dev *hdev, struct hci_conn *conn,
u8 key_type, u8 old_key_type)
{
/* Legacy key */
if (key_type < 0x03)
- return 1;
+ return true;

/* Debug keys are insecure so don't store them persistently */
if (key_type == HCI_LK_DEBUG_COMBINATION)
- return 0;
+ return false;

/* Changed combination key and there's no previous one */
if (key_type == HCI_LK_CHANGED_COMBINATION && old_key_type == 0xff)
- return 0;
+ return false;

/* Security mode 3 case */
if (!conn)
- return 1;
+ return true;

/* Neither local nor remote side had no-bonding as requirement */
if (conn->auth_type > 0x01 && conn->remote_auth > 0x01)
- return 1;
+ return true;

/* Local side had dedicated bonding as requirement */
if (conn->auth_type == 0x02 || conn->auth_type == 0x03)
- return 1;
+ return true;

/* Remote side had dedicated bonding as requirement */
if (conn->remote_auth == 0x02 || conn->remote_auth == 0x03)
- return 1;
+ return true;

/* If none of the above criteria match, then don't store the key
* persistently */
- return 0;
+ return false;
}

struct smp_ltk *hci_find_ltk(struct hci_dev *hdev, __le16 ediv, u8 rand[8])
@@ -1289,7 +1289,8 @@ int hci_add_link_key(struct hci_dev *hdev, struct hci_conn *conn, int new_key,
bdaddr_t *bdaddr, u8 *val, u8 type, u8 pin_len)
{
struct link_key *key, *old_key;
- u8 old_key_type, persistent;
+ u8 old_key_type;
+ bool persistent;

old_key = hci_find_link_key(hdev, bdaddr);
if (old_key) {
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 4e7a01f..fe36ed9 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2954,7 +2954,7 @@ int mgmt_write_scan_failed(struct hci_dev *hdev, u8 scan, u8 status)
return 0;
}

-int mgmt_new_link_key(struct hci_dev *hdev, struct link_key *key, u8 persistent)
+int mgmt_new_link_key(struct hci_dev *hdev, struct link_key *key, bool persistent)
{
struct mgmt_ev_new_link_key ev;

--
1.7.0.4



2012-04-13 09:14:18

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH 1/2] Bluetooth: hci_persistent_key should return bool

Hi Vishal,

On Fri, Apr 13, 2012, Vishal Agarwal wrote:
> This patch changes the return type of function hci_persistent_key
> from int to bool.
> Signed-off-by: Vishal Agarwal <[email protected]>
> ---
> include/net/bluetooth/hci_core.h | 2 +-
> net/bluetooth/hci_core.c | 21 +++++++++++----------
> net/bluetooth/mgmt.c | 2 +-
> 3 files changed, 13 insertions(+), 12 deletions(-)

I'd improve the commit message a bit. First there should be an empty
line before the Signed-off-by. Secondly it's generally considered good
practice to answer the question "why?" in the commit message. I.e. you
could mention that it makes more sense/is more natural to use bool for
this variable.

Johan

2012-04-13 09:11:52

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH 2/2] Bluetooth: Temporary keys should be retained during connection

Hi Vishal,

On Fri, Apr 13, 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 <[email protected]>
> ---
> 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(-)

In general the patch looks good but I'd simplify/shorten the following
just a tiny bit:

> - if (!persistent) {
> - list_del(&key->list);
> - kfree(key);
> - }
> + if (!conn)
> + return 0;
> +
> + conn->flush_key = !persistent;
>
> return 0;

Instead you could just do:

if (conn)
conn->flush_key = !persistent;

return 0;

Johan

2012-04-13 09:01:15

by Vishal Agarwal

[permalink] [raw]
Subject: [PATCH 2/2] Bluetooth: Temporary keys should be retained during connection

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 <[email protected]>
---
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 8a9abe2..afdea95 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 flush_key;

unsigned int sent;

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 703c28d..4daee98 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1333,10 +1333,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 (!conn)
+ return 0;
+
+ conn->flush_key = !persistent;

return 0;
}
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index bb6d802..5c23548 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1930,6 +1930,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->flush_key)
+ hci_remove_link_key(hdev, &conn->dst);
hci_proto_disconn_cfm(conn, ev->reason);
hci_conn_del(conn);
}
--
1.7.0.4