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

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

[Vinicius Costa Gomes]

---
 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

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

[Vinicius Costa Gomes]

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

[PATCH BlueZ v2 3/4] Fix memory leak when loading keys

[Vinicius Costa Gomes]

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

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

[Vinicius Costa Gomes]

---
 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

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

[Johan Hedberg]

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

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

[Johan Hedberg]

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

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

[Vinicius Costa Gomes]

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

[PATCH BlueZ] Fix memory leak when loading keys

[Vinicius Costa Gomes]

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

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

[Anderson Lizardo]

Hi Vinicius,

On Mon, Jan 23, 2012 at 8:21 PM, Vinicius Costa Gomes
<vinicius.gomes@openbossa.org> 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

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

[Johan Hedberg]

Hi Lizardo,

On Tue, Jan 24, 2012, Anderson Lizardo wrote:
> On Mon, Jan 23, 2012 at 8:21 PM, Vinicius Costa Gomes
> <vinicius.gomes@openbossa.org> 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

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

[Anderson Lizardo]

Hi Johan,

On Tue, Jan 24, 2012 at 6:46 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Lizardo,
>
> On Tue, Jan 24, 2012, Anderson Lizardo wrote:
>> On Mon, Jan 23, 2012 at 8:21 PM, Vinicius Costa Gomes
>> <vinicius.gomes@openbossa.org> 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

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

[Johan Hedberg]

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

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

[Vinicius Costa Gomes]

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

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

[Johan Hedberg]

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

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

[Vinicius Costa Gomes]

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