[PATCH 1/4] android/gatt: Fix using wrong variable type

[PATCH 1/4] android/gatt: Fix using wrong variable type

[Jakub Tyszkowski]

Not all bytes were set thus the following valgrind report:
==4748== Conditional jump or move depends on uninitialised value(s)
==4748==    at 0x436493: att_handler (gatt.c:5922)
==4748==    by 0x4448ED: received_data.part.2 (gattrib.c:432)
==4748==    by 0x4E7FCE4: g_main_context_dispatch (in
/lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0)
==4748==    by 0x4E80047: ??? (in
/lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0)
==4748==    by 0x4E80309: g_main_loop_run (in
/lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0)
==4748==    by 0x4045B6: main (main.c:772)
==4748==  Uninitialised value was created by a stack allocation
==4748==    at 0x432690: get_cid.isra.5 (gatt.c:2983)
==4748==
---
 android/gatt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/android/gatt.c b/android/gatt.c
index 7cf612f..d601cda 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -2965,7 +2965,7 @@ static void read_char_cb(guint8 status, const guint8 *pdu, guint16 len,
 static int get_cid(struct gatt_device *dev)
 {
 	GIOChannel *io;
-	int cid;
+	uint16_t cid;
 
 	io = g_attrib_get_channel(dev->attrib);
 
-- 
1.9.1

[PATCH 2/4] android/gatt: Fix sign counter comparison

[Jakub Tyszkowski]

This fixes invalid sign counter comparison and fixes one issue with
TC_SEC_CSIGN_BI_03_C PTS test case.
---
 android/gatt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index d601cda..828f788 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -5935,8 +5935,8 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
 		uint8_t t[ATT_SIGNATURE_LEN];
 		uint32_t r_sign_cnt = get_le32(s);
 
-		if (r_sign_cnt <= sign_cnt) {
-			error("gatt: Invalid sign counter (%d<=%d)",
+		if (r_sign_cnt < sign_cnt) {
+			error("gatt: Invalid sign counter (%d<%d)",
 							r_sign_cnt, sign_cnt);
 			return;
 		}
-- 
1.9.1

[PATCH 3/4] android/gatt: Fix pending request data leakage

[Jakub Tyszkowski]

Fix potential memory leaks and one reported by Valgrind:
==28453== 201 (144 direct, 57 indirect) bytes in 3 blocks are definitely
lost in loss record 156 of 166
==28453==    at 0x4C2CC70: calloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==28453==    by 0x4362AD: att_handler (gatt.c:5655)
==28453==    by 0x44496D: received_data.part.2 (gattrib.c:432)
==28453==    by 0x4E7FCE4: g_main_context_dispatch (in
/lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0)
==28453==    by 0x4E80047: ??? (in
/lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0)
==28453==    by 0x4E80309: g_main_loop_run (in
/lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0)
==28453==    by 0x4045B6: main (main.c:772)
---
 android/gatt.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/android/gatt.c b/android/gatt.c
index 828f788..47dadc2 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -4350,6 +4350,7 @@ static void send_dev_complete_response(struct gatt_device *device,
 		if (val->error) {
 			queue_destroy(temp, NULL);
 			error = val->error;
+			destroy_pending_request(val);
 			goto done;
 		}
 
@@ -4363,6 +4364,9 @@ static void send_dev_complete_response(struct gatt_device *device,
 		adl = att_data_list_alloc(queue_length(temp), sizeof(uint16_t) +
 									length);
 
+		if (val)
+			destroy_pending_request(val);
+
 		val = queue_pop_head(temp);
 		while (val) {
 			uint8_t *value = adl->data[iterator++];
@@ -5637,7 +5641,8 @@ static uint8_t read_by_type(const uint8_t *cmd, uint16_t cmd_len,
 
 		data->state = REQUEST_INIT;
 		data->handle = handle;
-		queue_push_tail(device->pending_requests, data);
+		if (!queue_push_tail(device->pending_requests, data))
+			free(data);
 	}
 
 	queue_destroy(q, NULL);
-- 
1.9.1

[PATCH 4/4] android/gatt: Use proper identity address for auto connect

[Jakub Tyszkowski]

We should behave the same as whe nwe connect using active scan.
---
 android/gatt.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 47dadc2..8cc7536 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -583,8 +583,24 @@ static void device_set_state(struct gatt_device *dev, uint32_t state)
 static bool auto_connect_le(struct gatt_device *dev)
 {
 	/*  For LE devices use auto connect feature if possible */
-	if (bt_kernel_conn_control())
-		return  bt_auto_connect_add(&dev->bdaddr);
+	if (bt_kernel_conn_control()) {
+		const bdaddr_t *bdaddr;
+
+		/*
+		 * If address type is random it might be that IRK was received
+		 * and random is just for faking Android Framework. ID address
+		 * should be used for connection if present.
+		 */
+		if (dev->bdaddr_type == BDADDR_LE_RANDOM) {
+			bdaddr = bt_get_id_addr(&dev->bdaddr, NULL);
+			if (!bdaddr)
+				return -EINVAL;
+		} else {
+			bdaddr = &dev->bdaddr;
+		}
+
+		return bt_auto_connect_add(bdaddr);
+	}
 
 	/* Trigger discovery if not already started */
 	if (!scanning) {
-- 
1.9.1

Re: [PATCH 4/4] android/gatt: Use proper identity address for auto connect

[Luiz Augusto von Dentz]

Hi Jakub,

On Thu, Nov 6, 2014 at 11:30 AM, Jakub Tyszkowski
<jakub.tyszkowski@tieto.com> wrote:
> We should behave the same as whe nwe connect using active scan.
> ---
>  android/gatt.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/android/gatt.c b/android/gatt.c
> index 47dadc2..8cc7536 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -583,8 +583,24 @@ static void device_set_state(struct gatt_device *dev, uint32_t state)
>  static bool auto_connect_le(struct gatt_device *dev)
>  {
>         /*  For LE devices use auto connect feature if possible */
> -       if (bt_kernel_conn_control())
> -               return  bt_auto_connect_add(&dev->bdaddr);
> +       if (bt_kernel_conn_control()) {
> +               const bdaddr_t *bdaddr;
> +
> +               /*
> +                * If address type is random it might be that IRK was received
> +                * and random is just for faking Android Framework. ID address
> +                * should be used for connection if present.
> +                */
> +               if (dev->bdaddr_type == BDADDR_LE_RANDOM) {
> +                       bdaddr = bt_get_id_addr(&dev->bdaddr, NULL);
> +                       if (!bdaddr)
> +                               return -EINVAL;
> +               } else {
> +                       bdaddr = &dev->bdaddr;
> +               }
> +
> +               return bt_auto_connect_add(bdaddr);
> +       }
>
>         /* Trigger discovery if not already started */
>         if (!scanning) {
> --
> 1.9.1

Perhaps this would be better done inside bt_auto_connect_add since
anyway bt_get_id_addr is in bluetooth.c, actually perhaps
auto_connect_le is not really necessary since bt_auto_connect_add
should be able to do the checks done here.



-- 
Luiz Augusto von Dentz

Re: [PATCH 1/4] android/gatt: Fix using wrong variable type

[Szymon Janc]

Hi Jakub,

On Thursday 06 of November 2014 10:30:35 Jakub Tyszkowski wrote:
> Not all bytes were set thus the following valgrind report:
> ==4748== Conditional jump or move depends on uninitialised value(s)
> ==4748==    at 0x436493: att_handler (gatt.c:5922)
> ==4748==    by 0x4448ED: received_data.part.2 (gattrib.c:432)
> ==4748==    by 0x4E7FCE4: g_main_context_dispatch (in
> /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0)
> ==4748==    by 0x4E80047: ??? (in
> /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0)
> ==4748==    by 0x4E80309: g_main_loop_run (in
> /lib/x86_64-linux-gnu/libglib-2.0.so.0.4002.0)
> ==4748==    by 0x4045B6: main (main.c:772)
> ==4748==  Uninitialised value was created by a stack allocation
> ==4748==    at 0x432690: get_cid.isra.5 (gatt.c:2983)
> ==4748==
> ---
>  android/gatt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 7cf612f..d601cda 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -2965,7 +2965,7 @@ static void read_char_cb(guint8 status, const guint8 *pdu, guint16 len,
>  static int get_cid(struct gatt_device *dev)
>  {
>  	GIOChannel *io;
> -	int cid;
> +	uint16_t cid;
>  
>  	io = g_attrib_get_channel(dev->attrib);
>  

Patch 1-3 are now applied, thanks. 

-- 
Best regards, 
Szymon Janc

Re: [PATCH 4/4] android/gatt: Use proper identity address for auto connect

[Szymon Janc]

Hi Jakub, Luiz,

On Thursday 06 of November 2014 15:43:14 Luiz Augusto von Dentz wrote:
> Hi Jakub,
> 
> On Thu, Nov 6, 2014 at 11:30 AM, Jakub Tyszkowski
> 
> <jakub.tyszkowski@tieto.com> wrote:
> > We should behave the same as whe nwe connect using active scan.
> > ---
> > 
> >  android/gatt.c | 20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> > 
> > diff --git a/android/gatt.c b/android/gatt.c
> > index 47dadc2..8cc7536 100644
> > --- a/android/gatt.c
> > +++ b/android/gatt.c
> > @@ -583,8 +583,24 @@ static void device_set_state(struct gatt_device *dev,
> > uint32_t state)> 
> >  static bool auto_connect_le(struct gatt_device *dev)
> >  {
> >  
> >         /*  For LE devices use auto connect feature if possible */
> > 
> > -       if (bt_kernel_conn_control())
> > -               return  bt_auto_connect_add(&dev->bdaddr);
> > +       if (bt_kernel_conn_control()) {
> > +               const bdaddr_t *bdaddr;
> > +
> > +               /*
> > +                * If address type is random it might be that IRK was
> > received +                * and random is just for faking Android
> > Framework. ID address +                * should be used for connection if
> > present.
> > +                */
> > +               if (dev->bdaddr_type == BDADDR_LE_RANDOM) {
> > +                       bdaddr = bt_get_id_addr(&dev->bdaddr, NULL);
> > +                       if (!bdaddr)
> > +                               return -EINVAL;
> > +               } else {
> > +                       bdaddr = &dev->bdaddr;
> > +               }
> > +
> > +               return bt_auto_connect_add(bdaddr);
> > +       }
> > 
> >         /* Trigger discovery if not already started */
> >         if (!scanning) {
> > 
> > --
> > 1.9.1
> 
> Perhaps this would be better done inside bt_auto_connect_add since
> anyway bt_get_id_addr is in bluetooth.c, actually perhaps
> auto_connect_le is not really necessary since bt_auto_connect_add
> should be able to do the checks done here.

I've applied this patch so we use proper address when connecting.
Nevertheless, Jakub please send follow up patch that address Luiz comment.

Thanks.

-- 
BR
Szymon Janc