2012-01-18 23:13:29

by Vinicius Costa Gomes

[permalink] [raw]
Subject: [PATCH BlueZ v2 1/4] Add support for creating devices from the stored LTK's

---
src/adapter.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 files changed, 106 insertions(+), 3 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index a75a0c4..6e8552b 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -1797,6 +1797,23 @@ struct adapter_keys {
GSList *keys;
};

+static int str2buf(const char *str, uint8_t *buf, size_t blen)
+{
+ int i, dlen;
+
+ if (str == NULL)
+ return -EINVAL;
+
+ memset(buf, 0, blen);
+
+ dlen = MIN((strlen(str) / 2), blen);
+
+ for (i = 0; i < dlen; i++)
+ sscanf(str + (i * 2), "%02hhX", &buf[i]);
+
+ return 0;
+}
+
static struct link_key_info *get_key_info(const char *addr, const char *value)
{
struct link_key_info *info;
@@ -1832,6 +1849,43 @@ static struct link_key_info *get_key_info(const char *addr, const char *value)
return info;
}

+static struct smp_ltk_info *get_ltk_info(const char *addr, const char *value)
+{
+ struct smp_ltk_info *ltk;
+ char *ptr;
+ int i, ret, total;
+
+ total = strlen(value);
+ if (total < 60) {
+ error("Unexpectedly short (%zu) LTK", strlen(value));
+ return NULL;
+ }
+
+ ltk = g_new0(struct smp_ltk_info, 1);
+
+ str2ba(addr, &ltk->bdaddr);
+
+ str2buf(value, ltk->val, sizeof(ltk->val));
+
+ ptr = (char *) value + 2 * sizeof(ltk->val) + 1;
+ total -= 2 * sizeof(ltk->val) + 1;
+
+ ret = sscanf(ptr, " %hhd %hhd %hhd %hhd %hd %n",
+ (uint8_t *) &ltk->addr_type,
+ &ltk->authenticated, &ltk->master,
+ &ltk->enc_size, &ltk->ediv, &i);
+ if (ret < 2) {
+ g_free(ltk);
+ return NULL;
+ }
+ ptr += i;
+ total -= i;
+
+ str2buf(ptr, ltk->rand, sizeof(ltk->rand));
+
+ return ltk;
+}
+
static void create_stored_device_from_linkkeys(char *key, char *value,
void *user_data)
{
@@ -1855,6 +1909,37 @@ static void create_stored_device_from_linkkeys(char *key, char *value,
}
}

+static void create_stored_device_from_ltks(char *key, char *value,
+ void *user_data)
+{
+ struct adapter_keys *keys = user_data;
+ struct btd_adapter *adapter = keys->adapter;
+ struct btd_device *device;
+ struct smp_ltk_info *info;
+ char srcaddr[18];
+ bdaddr_t src;
+
+ info = get_ltk_info(key, value);
+ if (info)
+ keys->keys = g_slist_append(keys->keys, info);
+
+ if (g_slist_find_custom(adapter->devices, key,
+ (GCompareFunc) device_address_cmp))
+ return;
+
+ adapter_get_address(adapter, &src);
+ ba2str(&src, srcaddr);
+
+ if (g_strcmp0(srcaddr, key) == 0)
+ return;
+
+ device = device_create(connection, adapter, key, info->addr_type);
+ if (device) {
+ device_set_temporary(device, FALSE);
+ adapter->devices = g_slist_append(adapter->devices, device);
+ }
+}
+
static void create_stored_device_from_blocked(char *key, char *value,
void *user_data)
{
@@ -1942,6 +2027,13 @@ static void create_stored_device_from_primary(char *key, char *value,
g_slist_free(uuids);
}

+static void smp_key_free(void *data)
+{
+ struct smp_ltk_info *info = data;
+
+ g_free(info);
+}
+
static void load_devices(struct btd_adapter *adapter)
{
char filename[PATH_MAX + 1];
@@ -1964,11 +2056,22 @@ static void load_devices(struct btd_adapter *adapter)

err = adapter_ops->load_keys(adapter->dev_id, keys.keys,
main_opts.debug_keys);
- if (err < 0) {
+ if (err < 0)
error("Unable to load keys to adapter_ops: %s (%d)",
strerror(-err), -err);
- g_slist_free_full(keys.keys, g_free);
- }
+
+ g_slist_free_full(keys.keys, g_free);
+ keys.keys = NULL;
+
+ create_name(filename, PATH_MAX, STORAGEDIR, srcaddr, "longtermkeys");
+ textfile_foreach(filename, create_stored_device_from_ltks, &keys);
+
+ err = adapter_ops->load_ltks(adapter->dev_id, keys.keys);
+ if (err < 0)
+ error("Unable to load keys to adapter_ops: %s (%d)",
+ strerror(-err), -err);
+ g_slist_free_full(keys.keys, smp_key_free);
+ keys.keys = NULL;

create_name(filename, PATH_MAX, STORAGEDIR, srcaddr, "blocked");
textfile_foreach(filename, create_stored_device_from_blocked, adapter);
--
1.7.8.1



2012-01-24 13:48:46

by Vinicius Costa Gomes

[permalink] [raw]
Subject: Re: [PATCH BlueZ] Fix memory leak when loading keys

Hi Johan,

On 15:39 Tue 24 Jan, Johan Hedberg wrote:
> Hi Vinicius,
>
> On Tue, Jan 24, 2012, Vinicius Costa Gomes wrote:
> > > In the future, could you please try to adopt the commit summary naming
> > > convention we've started using where there's a short prefix to highlight
> > > the part of the code the patch relates to. In this this case it should
> > > have been "hciops: ...".
> >
> > Noted. Would you want me to resend my last series?
>
> If you want to. Otherwise I'll fix it with "git commit --amend" myself.

I'll do it. Thanks.

>
> Johan

Cheers,
--
Vinicius

2012-01-24 13:39:57

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH BlueZ] Fix memory leak when loading keys

Hi Vinicius,

On Tue, Jan 24, 2012, Vinicius Costa Gomes wrote:
> > In the future, could you please try to adopt the commit summary naming
> > convention we've started using where there's a short prefix to highlight
> > the part of the code the patch relates to. In this this case it should
> > have been "hciops: ...".
>
> Noted. Would you want me to resend my last series?

If you want to. Otherwise I'll fix it with "git commit --amend" myself.

Johan

2012-01-24 13:34:02

by Vinicius Costa Gomes

[permalink] [raw]
Subject: Re: [PATCH BlueZ] Fix memory leak when loading keys

Hi Johan,

On 15:02 Tue 24 Jan, Johan Hedberg wrote:
> Hi Vinicius,
>
> On Mon, Jan 23, 2012, Vinicius Costa Gomes wrote:
> > If we need a copy of those keys we should copy them.
> > ---
> > plugins/hciops.c | 12 +++++++++++-
> > 1 files changed, 11 insertions(+), 1 deletions(-)
>
> Applied. Thanks.
>
> In the future, could you please try to adopt the commit summary naming
> convention we've started using where there's a short prefix to highlight
> the part of the code the patch relates to. In this this case it should
> have been "hciops: ...".

Noted. Would you want me to resend my last series?

>
> Johan

Cheers,
--
Vinicius

2012-01-24 13:02:40

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH BlueZ] Fix memory leak when loading keys

Hi Vinicius,

On Mon, Jan 23, 2012, Vinicius Costa Gomes wrote:
> If we need a copy of those keys we should copy them.
> ---
> plugins/hciops.c | 12 +++++++++++-
> 1 files changed, 11 insertions(+), 1 deletions(-)

Applied. Thanks.

In the future, could you please try to adopt the commit summary naming
convention we've started using where there's a short prefix to highlight
the part of the code the patch relates to. In this this case it should
have been "hciops: ...".

Johan

2012-01-24 11:46:30

by Anderson Lizardo

[permalink] [raw]
Subject: Re: [PATCH BlueZ] Fix memory leak when loading keys

Hi Johan,

On Tue, Jan 24, 2012 at 6:46 AM, Johan Hedberg <[email protected]> wrote:
> Hi Lizardo,
>
> On Tue, Jan 24, 2012, Anderson Lizardo wrote:
>> On Mon, Jan 23, 2012 at 8:21 PM, Vinicius Costa Gomes
>> <[email protected]> wrote:
>> > @@ -3570,7 +3571,16 @@ static int hciops_load_keys(int index, GSList *keys, gboolean debug_keys)
>> > ? ? ? ?if (dev->keys != NULL)
>> > ? ? ? ? ? ? ? ?return -EEXIST;
>> >
>> > - ? ? ? dev->keys = keys;
>> > + ? ? ? for (l = keys; l; l = l->next) {
>> > + ? ? ? ? ? ? ? struct link_key_info *orig, *dup;
>> > +
>> > + ? ? ? ? ? ? ? orig = l->data;
>> > +
>> > + ? ? ? ? ? ? ? dup = g_memdup(orig, sizeof(*orig));
>> > +
>> > + ? ? ? ? ? ? ? dev->keys = g_slist_prepend(dev->keys, dup);
>> > + ? ? ? }
>> > +
>>
>> No need to cleanup dev->keys first?
>
> Take a look at the first two lines of context. The function bails out if
> dev->keys is anything else than NULL.

Right. Oops :)

Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

2012-01-24 10:46:23

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH BlueZ] Fix memory leak when loading keys

Hi Lizardo,

On Tue, Jan 24, 2012, Anderson Lizardo wrote:
> On Mon, Jan 23, 2012 at 8:21 PM, Vinicius Costa Gomes
> <[email protected]> wrote:
> > @@ -3570,7 +3571,16 @@ static int hciops_load_keys(int index, GSList *keys, gboolean debug_keys)
> > ? ? ? ?if (dev->keys != NULL)
> > ? ? ? ? ? ? ? ?return -EEXIST;
> >
> > - ? ? ? dev->keys = keys;
> > + ? ? ? for (l = keys; l; l = l->next) {
> > + ? ? ? ? ? ? ? struct link_key_info *orig, *dup;
> > +
> > + ? ? ? ? ? ? ? orig = l->data;
> > +
> > + ? ? ? ? ? ? ? dup = g_memdup(orig, sizeof(*orig));
> > +
> > + ? ? ? ? ? ? ? dev->keys = g_slist_prepend(dev->keys, dup);
> > + ? ? ? }
> > +
>
> No need to cleanup dev->keys first?

Take a look at the first two lines of context. The function bails out if
dev->keys is anything else than NULL.

Johan

2012-01-24 10:40:03

by Anderson Lizardo

[permalink] [raw]
Subject: Re: [PATCH BlueZ] Fix memory leak when loading keys

Hi Vinicius,

On Mon, Jan 23, 2012 at 8:21 PM, Vinicius Costa Gomes
<[email protected]> wrote:
> @@ -3570,7 +3571,16 @@ static int hciops_load_keys(int index, GSList *keys, gboolean debug_keys)
> ? ? ? ?if (dev->keys != NULL)
> ? ? ? ? ? ? ? ?return -EEXIST;
>
> - ? ? ? dev->keys = keys;
> + ? ? ? for (l = keys; l; l = l->next) {
> + ? ? ? ? ? ? ? struct link_key_info *orig, *dup;
> +
> + ? ? ? ? ? ? ? orig = l->data;
> +
> + ? ? ? ? ? ? ? dup = g_memdup(orig, sizeof(*orig));
> +
> + ? ? ? ? ? ? ? dev->keys = g_slist_prepend(dev->keys, dup);
> + ? ? ? }
> +

No need to cleanup dev->keys first?

Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

2012-01-24 00:21:51

by Vinicius Costa Gomes

[permalink] [raw]
Subject: [PATCH BlueZ] Fix memory leak when loading keys

If we need a copy of those keys we should copy them.
---
plugins/hciops.c | 12 +++++++++++-
1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/plugins/hciops.c b/plugins/hciops.c
index d4d219c..f4af637 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -3563,6 +3563,7 @@ static int hciops_restore_powered(int index)
static int hciops_load_keys(int index, GSList *keys, gboolean debug_keys)
{
struct dev_info *dev = &devs[index];
+ GSList *l;

DBG("hci%d keys %d debug_keys %d", index, g_slist_length(keys),
debug_keys);
@@ -3570,7 +3571,16 @@ static int hciops_load_keys(int index, GSList *keys, gboolean debug_keys)
if (dev->keys != NULL)
return -EEXIST;

- dev->keys = keys;
+ for (l = keys; l; l = l->next) {
+ struct link_key_info *orig, *dup;
+
+ orig = l->data;
+
+ dup = g_memdup(orig, sizeof(*orig));
+
+ dev->keys = g_slist_prepend(dev->keys, dup);
+ }
+
dev->debug_keys = debug_keys;

return 0;
--
1.7.8.1


2012-01-23 13:29:15

by Vinicius Costa Gomes

[permalink] [raw]
Subject: Re: [PATCH BlueZ v2 3/4] Fix memory leak when loading keys

Hi Johan,

On 13:12 Mon 23 Jan, Johan Hedberg wrote:
> Hi Vinicius,
>
> On Wed, Jan 18, 2012, Vinicius Costa Gomes wrote:
> > If we need a copy of those keys we should copy them.
> > ---
> > plugins/hciops.c | 13 ++++++++++++-
> > 1 files changed, 12 insertions(+), 1 deletions(-)
>
> The two first patches have been applied but this one had some issues:
>
> > diff --git a/plugins/hciops.c b/plugins/hciops.c
> > index d4d219c..4e0729e 100644
> > --- a/plugins/hciops.c
> > +++ b/plugins/hciops.c
> > @@ -3563,6 +3563,7 @@ static int hciops_restore_powered(int index)
> > static int hciops_load_keys(int index, GSList *keys, gboolean debug_keys)
> > {
> > struct dev_info *dev = &devs[index];
> > + GSList *l, *new;
> >
> > DBG("hci%d keys %d debug_keys %d", index, g_slist_length(keys),
> > debug_keys);
> > @@ -3570,7 +3571,17 @@ static int hciops_load_keys(int index, GSList *keys, gboolean debug_keys)
> > if (dev->keys != NULL)
> > return -EEXIST;
> >
> > - dev->keys = keys;
> > + for (new = NULL, l = keys; l; l = l->next) {
> > + struct link_key_info *orig, *dup;
> > +
> > + orig = l->data;
> > +
> > + dup = g_memdup(orig, sizeof(*orig));
> > +
> > + new = g_slist_prepend(new, dup);
> > + }
> > +
> > + dev->keys = new;
> > dev->debug_keys = debug_keys;
>
>
> In general I'm always a bit on guard with usage of C++ keywords like new
> and class, but in this case instead of renaming you could just prepend
> directly to dev->keys, i.e. there's no need for this temporary variable.

Will fix. Thanks.

>
> Johan

Cheers,
--
Vinicius

2012-01-23 11:13:10

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH BlueZ v2 4/4] Remove the SMP Long Term Key when the device is removed

Hi Vinicius,

On Wed, Jan 18, 2012, Vinicius Costa Gomes wrote:
> ---
> src/device.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/src/device.c b/src/device.c
> index 16855b1..8d02c2c 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -1088,6 +1088,7 @@ static void device_remove_stored(struct btd_device *device)
> if (device_is_bonded(device)) {
> delete_entry(&src, "linkkeys", addr);
> delete_entry(&src, "aliases", addr);
> + delete_entry(&src, "longtermkeys", addr);
> device_set_bonded(device, FALSE);
> device->paired = FALSE;
> btd_adapter_remove_bonding(device->adapter, &device->bdaddr);

This patch has also been applied, so you just need to fix and resend the
third one. Thanks.

Johan

2012-01-23 11:12:05

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH BlueZ v2 3/4] Fix memory leak when loading keys

Hi Vinicius,

On Wed, Jan 18, 2012, Vinicius Costa Gomes wrote:
> If we need a copy of those keys we should copy them.
> ---
> plugins/hciops.c | 13 ++++++++++++-
> 1 files changed, 12 insertions(+), 1 deletions(-)

The two first patches have been applied but this one had some issues:

> diff --git a/plugins/hciops.c b/plugins/hciops.c
> index d4d219c..4e0729e 100644
> --- a/plugins/hciops.c
> +++ b/plugins/hciops.c
> @@ -3563,6 +3563,7 @@ static int hciops_restore_powered(int index)
> static int hciops_load_keys(int index, GSList *keys, gboolean debug_keys)
> {
> struct dev_info *dev = &devs[index];
> + GSList *l, *new;
>
> DBG("hci%d keys %d debug_keys %d", index, g_slist_length(keys),
> debug_keys);
> @@ -3570,7 +3571,17 @@ static int hciops_load_keys(int index, GSList *keys, gboolean debug_keys)
> if (dev->keys != NULL)
> return -EEXIST;
>
> - dev->keys = keys;
> + for (new = NULL, l = keys; l; l = l->next) {
> + struct link_key_info *orig, *dup;
> +
> + orig = l->data;
> +
> + dup = g_memdup(orig, sizeof(*orig));
> +
> + new = g_slist_prepend(new, dup);
> + }
> +
> + dev->keys = new;
> dev->debug_keys = debug_keys;


In general I'm always a bit on guard with usage of C++ keywords like new
and class, but in this case instead of renaming you could just prepend
directly to dev->keys, i.e. there's no need for this temporary variable.

Johan

2012-01-18 23:13:32

by Vinicius Costa Gomes

[permalink] [raw]
Subject: [PATCH BlueZ v2 4/4] Remove the SMP Long Term Key when the device is removed

---
src/device.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/src/device.c b/src/device.c
index 16855b1..8d02c2c 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1088,6 +1088,7 @@ static void device_remove_stored(struct btd_device *device)
if (device_is_bonded(device)) {
delete_entry(&src, "linkkeys", addr);
delete_entry(&src, "aliases", addr);
+ delete_entry(&src, "longtermkeys", addr);
device_set_bonded(device, FALSE);
device->paired = FALSE;
btd_adapter_remove_bonding(device->adapter, &device->bdaddr);
--
1.7.8.1


2012-01-18 23:13:31

by Vinicius Costa Gomes

[permalink] [raw]
Subject: [PATCH BlueZ v2 3/4] Fix memory leak when loading keys

If we need a copy of those keys we should copy them.
---
plugins/hciops.c | 13 ++++++++++++-
1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/plugins/hciops.c b/plugins/hciops.c
index d4d219c..4e0729e 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -3563,6 +3563,7 @@ static int hciops_restore_powered(int index)
static int hciops_load_keys(int index, GSList *keys, gboolean debug_keys)
{
struct dev_info *dev = &devs[index];
+ GSList *l, *new;

DBG("hci%d keys %d debug_keys %d", index, g_slist_length(keys),
debug_keys);
@@ -3570,7 +3571,17 @@ static int hciops_load_keys(int index, GSList *keys, gboolean debug_keys)
if (dev->keys != NULL)
return -EEXIST;

- dev->keys = keys;
+ for (new = NULL, l = keys; l; l = l->next) {
+ struct link_key_info *orig, *dup;
+
+ orig = l->data;
+
+ dup = g_memdup(orig, sizeof(*orig));
+
+ new = g_slist_prepend(new, dup);
+ }
+
+ dev->keys = new;
dev->debug_keys = debug_keys;

return 0;
--
1.7.8.1


2012-01-18 23:13:30

by Vinicius Costa Gomes

[permalink] [raw]
Subject: [PATCH BlueZ v2 2/4] Use the helper function to convert a string to a binary buffer

When loading Link Keys from storage we are able to re-use the function
that was introduced earlier for the LTK case.
---
src/adapter.c | 8 +-------
1 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 6e8552b..d5075db 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -1819,7 +1819,6 @@ static struct link_key_info *get_key_info(const char *addr, const char *value)
struct link_key_info *info;
char tmp[3];
long int l;
- int i;

if (strlen(value) < 36) {
error("Unexpectedly short (%zu) link key line", strlen(value));
@@ -1830,12 +1829,7 @@ static struct link_key_info *get_key_info(const char *addr, const char *value)

str2ba(addr, &info->bdaddr);

- memset(tmp, 0, sizeof(tmp));
-
- for (i = 0; i < 16; i++) {
- memcpy(tmp, value + (i * 2), 2);
- info->key[i] = (uint8_t) strtol(tmp, NULL, 16);
- }
+ str2buf(value, info->key, sizeof(info->key));

memcpy(tmp, value + 33, 2);
info->type = (uint8_t) strtol(tmp, NULL, 10);
--
1.7.8.1