2015-02-26 09:39:27

by Lukasz Rymanowski

Subject: [PATCH 00/16] android/gatt: Improve robustness in Gatt Client

This set improves robustness on searching services, characteristics,
descriptors and included services.

Issue found on UPF50.

There is 4 test showing issue and after each test there is a fix for that.

Last 5 patches is a same fix for shared/gatt-helpers. I did not write special
test for it as we will test it once we make Android to use it shared code.
Anyway, this change does not brake gatt unit tests

Lukasz Rymanowski (16):
android/tester-gatt: Add robustness test for search services
attrib/gatt: Improve robustness on search primary services
android/tester-gatt: Add robustness test for get characteristic
attrib/gatt: Minor refactor in char_discovered_cb
attrib/gatt: Improve robustness when searching for characteristics
android/gatt: Check status on get characteristic callback
android/tester: Add robustness test for get descriptor
attrib/gatt: Minor refactor in desc_discovered_cb
attrib/gatt: Improve robustness when searching for descriptors
android/tester: Add robustness test for get included services
attrib/gatt: Improve robustness when searching for included services
shated/gatt-helpers: Improve robustness of search service
shared/gatt-helpers: Improve robustness of get characteristics
shared/gatt-helpers: Improve robustness of get include services
shared/gatt-helpers: Improve robustness read by type request
shared/gatt-helpers: Improve robustness of get descriptors

android/gatt.c | 8 +++
android/tester-gatt.c | 128 ++++++++++++++++++++++++++++++++++++++++++++++
attrib/gatt.c | 93 +++++++++++++++++++++++++++++----
src/shared/gatt-helpers.c | 89 +++++++++++++++++++++++++++++---
4 files changed, 302 insertions(+), 16 deletions(-)

--
1.8.4

2015-02-27 15:35:12

by Szymon Janc

Subject: Re: [PATCH 00/16] android/gatt: Improve robustness in Gatt Client

Hi Łukasz,

On Thursday 26 of February 2015 10:39:27 Lukasz Rymanowski wrote:
> This set improves robustness on searching services, characteristics,
> descriptors and included services.
>
> Issue found on UPF50.
>
> There is 4 test showing issue and after each test there is a fix for that.
>
> Last 5 patches is a same fix for shared/gatt-helpers. I did not write special
> test for it as we will test it once we make Android to use it shared code.
> Anyway, this change does not brake gatt unit tests
>
> Lukasz Rymanowski (16):
> android/tester-gatt: Add robustness test for search services
> attrib/gatt: Improve robustness on search primary services
> android/tester-gatt: Add robustness test for get characteristic
> attrib/gatt: Minor refactor in char_discovered_cb
> attrib/gatt: Improve robustness when searching for characteristics
> android/gatt: Check status on get characteristic callback
> android/tester: Add robustness test for get descriptor
> attrib/gatt: Minor refactor in desc_discovered_cb
> attrib/gatt: Improve robustness when searching for descriptors
> android/tester: Add robustness test for get included services
> attrib/gatt: Improve robustness when searching for included services
> shated/gatt-helpers: Improve robustness of search service
> shared/gatt-helpers: Improve robustness of get characteristics
> shared/gatt-helpers: Improve robustness of get include services
> shared/gatt-helpers: Improve robustness read by type request
> shared/gatt-helpers: Improve robustness of get descriptors
>
> android/gatt.c | 8 +++
> android/tester-gatt.c | 128 ++++++++++++++++++++++++++++++++++++++++++++++
> attrib/gatt.c | 93 +++++++++++++++++++++++++++++----
> src/shared/gatt-helpers.c | 89 +++++++++++++++++++++++++++++---
> 4 files changed, 302 insertions(+), 16 deletions(-)
>
Patches 1-11 are now applied, thanks.

--
Best regards,
Szymon Janc

2015-02-27 15:14:42

by Lukasz Rymanowski

Subject: Re: [PATCH 15/16] shared/gatt-helpers: Improve robustness read by type request

Hi Szymon,

On Fri, Feb 27, 2015 at 4:07 PM, Szymon Janc <[email protected]> wrote:
> Hi Łukasz,
>
> On Thursday 26 of February 2015 10:39:42 Lukasz Rymanowski wrote:
>> This patch makes sure that we do get into infinite loop when doing
>> read by type request.
>>
>> It could happen if we got bogus read by type response
>> ---
>> src/shared/gatt-helpers.c | 15 ++++++++++++++-
>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
>> index 8858e58..6e5cf30 100644
>> --- a/src/shared/gatt-helpers.c
>> +++ b/src/shared/gatt-helpers.c
>> @@ -1331,10 +1331,22 @@ static void read_by_type_cb(uint8_t opcode, const void *pdu,
>> }
>>
>> last_handle = get_le16(pdu + length - data_length);
>> +
>> + /*
>> + * If last handle is lower from previous start handle then it is smth
>> + * wrong. Let's stop search, otherwise we might enter infinite loop.
>> + */
>> + if (last_handle < op->start_handle) {
>> + success = true;
>> + goto done;
>> + }
>
> Shouldn't this be success = false ?

True, my bad. Can you fix that or should I send v2?
>
>> +
>> + op->start_handle = last_handle + 1;
>> +
>> if (last_handle != op->end_handle) {
>> uint8_t pdu[4 + get_uuid_len(&op->uuid)];
>>
>> - put_le16(last_handle + 1, pdu);
>> + put_le16(op->start_handle, pdu);
>> put_le16(op->end_handle, pdu + 2);
>> bt_uuid_to_le(&op->uuid, pdu + 4);
>>
>> @@ -1376,6 +1388,7 @@ bool bt_gatt_read_by_type(struct bt_att *att, uint16_t start, uint16_t end,
>> op->callback = callback;
>> op->user_data = user_data;
>> op->destroy = destroy;
>> + op->start_handle = start;
>> op->end_handle = end;
>> op->uuid = *uuid;
>>
>>
>
> --
> Best regards,
> Szymon Janc
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2015-02-27 15:07:01

by Szymon Janc

Subject: Re: [PATCH 15/16] shared/gatt-helpers: Improve robustness read by type request

Hi Łukasz,

On Thursday 26 of February 2015 10:39:42 Lukasz Rymanowski wrote:
> This patch makes sure that we do get into infinite loop when doing
> read by type request.
>
> It could happen if we got bogus read by type response
> ---
> src/shared/gatt-helpers.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
> index 8858e58..6e5cf30 100644
> --- a/src/shared/gatt-helpers.c
> +++ b/src/shared/gatt-helpers.c
> @@ -1331,10 +1331,22 @@ static void read_by_type_cb(uint8_t opcode, const void *pdu,
> }
>
> last_handle = get_le16(pdu + length - data_length);
> +
> + /*
> + * If last handle is lower from previous start handle then it is smth
> + * wrong. Let's stop search, otherwise we might enter infinite loop.
> + */
> + if (last_handle < op->start_handle) {
> + success = true;
> + goto done;
> + }

Shouldn't this be success = false ?

> +
> + op->start_handle = last_handle + 1;
> +
> if (last_handle != op->end_handle) {
> uint8_t pdu[4 + get_uuid_len(&op->uuid)];
>
> - put_le16(last_handle + 1, pdu);
> + put_le16(op->start_handle, pdu);
> put_le16(op->end_handle, pdu + 2);
> bt_uuid_to_le(&op->uuid, pdu + 4);
>
> @@ -1376,6 +1388,7 @@ bool bt_gatt_read_by_type(struct bt_att *att, uint16_t start, uint16_t end,
> op->callback = callback;
> op->user_data = user_data;
> op->destroy = destroy;
> + op->start_handle = start;
> op->end_handle = end;
> op->uuid = *uuid;
>
>

--
Best regards,
Szymon Janc

2015-02-26 09:39:41

by Lukasz Rymanowski

Subject: [PATCH 14/16] shared/gatt-helpers: Improve robustness of get include services

This patch makes sure that we do get into infinite loop when doing
search for included services.

It could happen if we got bogus read by type response
---
src/shared/gatt-helpers.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index cd0ffe2..8858e58 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -1094,10 +1094,21 @@ static void discover_included_cb(uint8_t opcode, const void *pdu,
}

last_handle = get_le16(pdu + length - data_length);
+
+ /*
+ * If last handle is lower from previous start handle then it is smth
+ * wrong. Let's stop search, otherwise we might enter infinite loop.
+ */
+ if (last_handle < op->start_handle) {
+ success = false;
+ goto failed;
+ }
+
+ op->start_handle = last_handle + 1;
if (last_handle != op->end_handle) {
uint8_t pdu[6];

- put_le16(last_handle + 1, pdu);
+ put_le16(op->start_handle, pdu);
put_le16(op->end_handle, pdu + 2);
put_le16(GATT_INCLUDE_UUID, pdu + 4);

@@ -1140,6 +1151,7 @@ bool bt_gatt_discover_included_services(struct bt_att *att,
op->callback = callback;
op->user_data = user_data;
op->destroy = destroy;
+ op->start_handle = start;
op->end_handle = end;

put_le16(start, pdu);
--
1.8.4

2015-02-26 09:39:42

by Lukasz Rymanowski

Subject: [PATCH 15/16] shared/gatt-helpers: Improve robustness read by type request

This patch makes sure that we do get into infinite loop when doing
read by type request.

It could happen if we got bogus read by type response
---
src/shared/gatt-helpers.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index 8858e58..6e5cf30 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -1331,10 +1331,22 @@ static void read_by_type_cb(uint8_t opcode, const void *pdu,
}

last_handle = get_le16(pdu + length - data_length);
+
+ /*
+ * If last handle is lower from previous start handle then it is smth
+ * wrong. Let's stop search, otherwise we might enter infinite loop.
+ */
+ if (last_handle < op->start_handle) {
+ success = true;
+ goto done;
+ }
+
+ op->start_handle = last_handle + 1;
+
if (last_handle != op->end_handle) {
uint8_t pdu[4 + get_uuid_len(&op->uuid)];

- put_le16(last_handle + 1, pdu);
+ put_le16(op->start_handle, pdu);
put_le16(op->end_handle, pdu + 2);
bt_uuid_to_le(&op->uuid, pdu + 4);

@@ -1376,6 +1388,7 @@ bool bt_gatt_read_by_type(struct bt_att *att, uint16_t start, uint16_t end,
op->callback = callback;
op->user_data = user_data;
op->destroy = destroy;
+ op->start_handle = start;
op->end_handle = end;
op->uuid = *uuid;

--
1.8.4

2015-02-26 09:39:43

by Lukasz Rymanowski

Subject: [PATCH 16/16] shared/gatt-helpers: Improve robustness of get descriptors

This patch makes sure that we do get into infinite loop when doing
get descriptors operation.

It could happen if we got bogus find information response
---
src/shared/gatt-helpers.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index 6e5cf30..39ba636 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -1460,10 +1460,22 @@ static void discover_descs_cb(uint8_t opcode, const void *pdu,
}

last_handle = get_le16(pdu + length - data_length);
+
+ /*
+ * If last handle is lower from previous start handle then it is smth
+ * wrong. Let's stop search, otherwise we might enter infinite loop.
+ */
+ if (last_handle < op->start_handle) {
+ success = false;
+ goto done;
+ }
+
+ op->start_handle = last_handle + 1;
+
if (last_handle != op->end_handle) {
uint8_t pdu[4];

- put_le16(last_handle + 1, pdu);
+ put_le16(op->start_handle, pdu);
put_le16(op->end_handle, pdu + 2);

op->id = bt_att_send(op->att, BT_ATT_OP_FIND_INFO_REQ,
@@ -1505,6 +1517,7 @@ bool bt_gatt_discover_descriptors(struct bt_att *att,
op->callback = callback;
op->user_data = user_data;
op->destroy = destroy;
+ op->start_handle = start;
op->end_handle = end;

put_le16(start, pdu);
--
1.8.4

2015-02-26 09:39:39

by Lukasz Rymanowski

Subject: [PATCH 12/16] shated/gatt-helpers: Improve robustness of search service

This patch makes sure that we do get into infinite loop when doing
search service request

It could happen if we got bogus read by group or find by type response
---
src/shared/gatt-helpers.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index 2d07a83..1a60f31 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -212,6 +212,7 @@ static bool convert_uuid_le(const uint8_t *src, size_t len, uint8_t dst[16])
struct discovery_op {
struct bt_att *att;
unsigned int id;
+ uint16_t start_handle;
uint16_t end_handle;
int ref_count;
bt_uuid_t uuid;
@@ -663,10 +664,22 @@ static void read_by_grp_type_cb(uint8_t opcode, const void *pdu,
}

last_end = get_le16(pdu + length - data_length + 2);
+
+ /*
+ * If last handle is lower from previous start handle then it is smth
+ * wrong. Let's stop search, otherwise we might enter infinite loop.
+ */
+ if (last_end < op->start_handle) {
+ success = false;
+ goto done;
+ }
+
+ op->start_handle = last_end + 1;
+
if (last_end < op->end_handle) {
uint8_t pdu[6];

- put_le16(last_end + 1, pdu);
+ put_le16(op->start_handle, pdu);
put_le16(op->end_handle, pdu + 2);
put_le16(op->service_type, pdu + 4);

@@ -738,10 +751,22 @@ static void find_by_type_val_cb(uint8_t opcode, const void *pdu,
* last_end is end handle of last data set
*/
last_end = get_le16(pdu + length - 2);
+
+ /*
+ * If last handle is lower from previous start handle then it is smth
+ * wrong. Let's stop search, otherwise we might enter infinite loop.
+ */
+ if (last_end < op->start_handle) {
+ success = false;
+ goto done;
+ }
+
+ op->start_handle = last_end + 1;
+
if (last_end < op->end_handle) {
uint8_t pdu[6 + get_uuid_len(&op->uuid)];

- put_le16(last_end + 1, pdu);
+ put_le16(op->start_handle, pdu);
put_le16(op->end_handle, pdu + 2);
put_le16(op->service_type, pdu + 4);
bt_uuid_to_le(&op->uuid, pdu + 6);
@@ -782,6 +807,7 @@ static bool discover_services(struct bt_att *att, bt_uuid_t *uuid,
return false;

op->att = att;
+ op->start_handle = start;
op->end_handle = end;
op->callback = callback;
op->user_data = user_data;
--
1.8.4

2015-02-26 09:39:40

by Lukasz Rymanowski

Subject: [PATCH 13/16] shared/gatt-helpers: Improve robustness of get characteristics

This patch makes sure that we do get into infinite loop when doing
search for characteristics

It could happen if we got bogus read by type response
---
src/shared/gatt-helpers.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index 1a60f31..cd0ffe2 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -1203,10 +1203,22 @@ static void discover_chrcs_cb(uint8_t opcode, const void *pdu,
goto done;
}
last_handle = get_le16(pdu + length - data_length);
+
+ /*
+ * If last handle is lower from previous start handle then it is smth
+ * wrong. Let's stop search, otherwise we might enter infinite loop.
+ */
+ if (last_handle < op->start_handle) {
+ success = false;
+ goto done;
+ }
+
+ op->start_handle = last_handle + 1;
+
if (last_handle != op->end_handle) {
uint8_t pdu[6];

- put_le16(last_handle + 1, pdu);
+ put_le16(op->start_handle, pdu);
put_le16(op->end_handle, pdu + 2);
put_le16(GATT_CHARAC_UUID, pdu + 4);

@@ -1249,6 +1261,7 @@ bool bt_gatt_discover_characteristics(struct bt_att *att,
op->callback = callback;
op->user_data = user_data;
op->destroy = destroy;
+ op->start_handle = start;
op->end_handle = end;

put_le16(start, pdu);
--
1.8.4

2015-02-26 09:39:38

by Lukasz Rymanowski

Subject: [PATCH 11/16] attrib/gatt: Improve robustness when searching for included services

Without this patch it is possible to enter infinite loop when
searching included services on remote device. This patch fixes that.

Issue happens when remote device replies with ending handle which is
lower than start handle we use for search
---
attrib/gatt.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/attrib/gatt.c b/attrib/gatt.c
index bc2f92a..ea5c1cf 100644
--- a/attrib/gatt.c
+++ b/attrib/gatt.c
@@ -55,6 +55,7 @@ struct included_discovery {
unsigned int id;
int refs;
int err;
+ uint16_t start_handle;
uint16_t end_handle;
GSList *includes;
gatt_cb_t cb;
@@ -549,8 +550,19 @@ static void find_included_cb(uint8_t status, const uint8_t *pdu, uint16_t len,

att_data_list_free(list);

+ /*
+ * If last handle is lower from previous start handle then it is smth
+ * wrong. Let's stop search, otherwise we might enter infinite loop.
+ */
+ if (last_handle < isd->start_handle) {
+ isd->err = ATT_ECODE_UNLIKELY;
+ goto done;
+ }
+
+ isd->start_handle = last_handle + 1;
+
if (last_handle < isd->end_handle)
- find_included(isd, last_handle + 1);
+ find_included(isd, isd->start_handle);

done:
if (isd->err == 0)
@@ -564,6 +576,7 @@ unsigned int gatt_find_included(GAttrib *attrib, uint16_t start, uint16_t end,

isd = g_new0(struct included_discovery, 1);
isd->attrib = g_attrib_ref(attrib);
+ isd->start_handle = start;
isd->end_handle = end;
isd->cb = func;
isd->user_data = user_data;
--
1.8.4

2015-02-26 09:39:36

by Lukasz Rymanowski

Subject: [PATCH 09/16] attrib/gatt: Improve robustness when searching for descriptors

Without this patch it is possible to enter infinite loop when
searching descriptors on remote device. This patch fixes that.

Issue happens when remote device replies with ending handle which is
lower than start handle we use for search.
---
attrib/gatt.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/attrib/gatt.c b/attrib/gatt.c
index df4e3a7..bc2f92a 100644
--- a/attrib/gatt.c
+++ b/attrib/gatt.c
@@ -83,6 +83,7 @@ struct discover_desc {
GAttrib *attrib;
unsigned int id;
bt_uuid_t *uuid;
+ uint16_t start;
uint16_t end;
GSList *descriptors;
gatt_cb_t cb;
@@ -1072,6 +1073,17 @@ static void desc_discovered_cb(guint8 status, const guint8 *ipdu,

att_data_list_free(list);

+ /*
+ * If last handle is lower from previous start handle then it is smth
+ * wrong. Let's stop search, otherwise we might enter infinite loop.
+ */
+ if (last < dd->start) {
+ err = ATT_ECODE_UNLIKELY;
+ goto done;
+ }
+
+ dd->start = last + 1;
+
if (last < dd->end && !uuid_found) {
guint16 oplen;
size_t buflen;
@@ -1079,7 +1091,7 @@ static void desc_discovered_cb(guint8 status, const guint8 *ipdu,

buf = g_attrib_get_buffer(dd->attrib, &buflen);

- oplen = enc_find_info_req(last + 1, dd->end, buf, buflen);
+ oplen = enc_find_info_req(dd->start, dd->end, buf, buflen);
if (oplen == 0)
return;

@@ -1114,6 +1126,7 @@ guint gatt_discover_desc(GAttrib *attrib, uint16_t start, uint16_t end,
dd->attrib = g_attrib_ref(attrib);
dd->cb = func;
dd->user_data = user_data;
+ dd->start = start;
dd->end = end;
dd->uuid = g_memdup(uuid, sizeof(bt_uuid_t));

--
1.8.4

2015-02-26 09:39:37

by Lukasz Rymanowski

Subject: [PATCH 10/16] android/tester: Add robustness test for get included services

---
android/tester-gatt.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)

diff --git a/android/tester-gatt.c b/android/tester-gatt.c
index 10c09d2..b8b088b 100644
--- a/android/tester-gatt.c
+++ b/android/tester-gatt.c
@@ -950,6 +950,13 @@ static struct iovec get_descriptor_3[] = {
end_pdu
};

+static struct iovec get_included_0[] = {
+ SEARCH_SERVICE_SINGLE_SUCCESS_PDUS,
+ raw_pdu(0x08, 0x01, 0x00, 0x10, 0x00, 0x02, 0x28),
+ raw_pdu(0x09, 0x08, 0x00, 0x00, 0x15, 0x00, 0x19, 0x00, 0xff, 0xfe),
+ end_pdu
+};
+
static struct iovec get_included_1[] = {
SEARCH_SERVICE_SINGLE_SUCCESS_PDUS,
raw_pdu(0x08, 0x01, 0x00, 0x10, 0x00, 0x02, 0x28),
@@ -2378,6 +2385,31 @@ static struct test_case test_cases[] = {
ACTION_SUCCESS(bluetooth_disable_action, NULL),
CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_OFF),
),
+ TEST_CASE_BREDRLE("Gatt Client - Get Included Services - Incorrect rsp",
+ ACTION_SUCCESS(init_pdus, get_included_0),
+ ACTION_SUCCESS(bluetooth_enable_action, NULL),
+ CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_ON),
+ ACTION_SUCCESS(emu_setup_powered_remote_action, NULL),
+ ACTION_SUCCESS(emu_set_ssp_mode_action, NULL),
+ ACTION_SUCCESS(emu_set_connect_cb_action, gatt_conn_cb),
+ ACTION_SUCCESS(gatt_client_register_action, &app1_uuid),
+ CALLBACK_STATUS(CB_GATTC_REGISTER_CLIENT, BT_STATUS_SUCCESS),
+ ACTION_SUCCESS(gatt_client_start_scan_action, NULL),
+ CLLBACK_GATTC_SCAN_RES(prop_emu_remotes_default_set, 1, TRUE),
+ ACTION_SUCCESS(gatt_client_stop_scan_action, NULL),
+ ACTION_SUCCESS(gatt_client_connect_action, &app1_conn_req),
+ CALLBACK_GATTC_CONNECT(GATT_STATUS_SUCCESS,
+ prop_emu_remotes_default_set,
+ CONN1_ID, APP1_ID),
+ ACTION_SUCCESS(gatt_client_search_services, &search_services_1),
+ CALLBACK_GATTC_SEARCH_COMPLETE(GATT_STATUS_SUCCESS, CONN1_ID),
+ ACTION_SUCCESS(gatt_client_get_included_action,
+ &get_incl_data_1),
+ CALLBACK_GATTC_GET_INCLUDED(GATT_STATUS_FAILURE, CONN1_ID,
+ &service_1, NULL),
+ ACTION_SUCCESS(bluetooth_disable_action, NULL),
+ CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_OFF),
+ ),
TEST_CASE_BREDRLE("Gatt Client - Get Included Service - 16 UUID",
ACTION_SUCCESS(init_pdus, get_included_1),
ACTION_SUCCESS(bluetooth_enable_action, NULL),
--
1.8.4

2015-02-26 09:39:35

by Lukasz Rymanowski

Subject: [PATCH 08/16] attrib/gatt: Minor refactor in desc_discovered_cb

This is needed for next patch
---
attrib/gatt.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/attrib/gatt.c b/attrib/gatt.c
index 2749c46..df4e3a7 100644
--- a/attrib/gatt.c
+++ b/attrib/gatt.c
@@ -1008,12 +1008,17 @@ static void desc_discovered_cb(guint8 status, const guint8 *ipdu,
{
struct discover_desc *dd = user_data;
struct att_data_list *list;
- unsigned int i, err = ATT_ECODE_ATTR_NOT_FOUND;
+ unsigned int i, err = 0;
guint8 format;
uint16_t last = 0xffff;
uint8_t type;
gboolean uuid_found = FALSE;

+ if (status == ATT_ECODE_ATTR_NOT_FOUND) {
+ err = dd->descriptors ? 0 : status;
+ goto done;
+ }
+
if (status) {
err = status;
goto done;
@@ -1086,7 +1091,6 @@ static void desc_discovered_cb(guint8 status, const guint8 *ipdu,
}

done:
- err = (dd->descriptors ? 0 : err);
dd->cb(err, dd->descriptors, dd->user_data);
}

--
1.8.4

2015-02-26 09:39:34

by Lukasz Rymanowski

Subject: [PATCH 07/16] android/tester: Add robustness test for get descriptor

---
android/tester-gatt.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)

diff --git a/android/tester-gatt.c b/android/tester-gatt.c
index 1abfe26..10c09d2 100644
--- a/android/tester-gatt.c
+++ b/android/tester-gatt.c
@@ -914,6 +914,14 @@ static struct iovec get_characteristic_2[] = {
end_pdu
};

+static struct iovec get_descriptor_0[] = {
+ SEARCH_SERVICE_SINGLE_SUCCESS_PDUS,
+ READ_BY_TYPE_SINGLE_CHARACTERISTIC_PDUS,
+ raw_pdu(0x04, 0x01, 0x00, 0x10, 0x00),
+ raw_pdu(0x05, 0x01, 0x00, 0x00, 0x00, 0x29),
+ end_pdu
+};
+
static struct iovec get_descriptor_1[] = {
SEARCH_SERVICE_SINGLE_SUCCESS_PDUS,
READ_BY_TYPE_SINGLE_CHARACTERISTIC_PDUS,
@@ -2247,6 +2255,35 @@ static struct test_case test_cases[] = {
ACTION_SUCCESS(bluetooth_disable_action, NULL),
CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_OFF),
),
+ TEST_CASE_BREDRLE("Gatt Client - Get Descriptor - Incorrect rsp",
+ ACTION_SUCCESS(init_pdus, get_descriptor_0),
+ ACTION_SUCCESS(bluetooth_enable_action, NULL),
+ CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_ON),
+ ACTION_SUCCESS(emu_setup_powered_remote_action, NULL),
+ ACTION_SUCCESS(emu_set_ssp_mode_action, NULL),
+ ACTION_SUCCESS(emu_set_connect_cb_action, gatt_conn_cb),
+ ACTION_SUCCESS(gatt_client_register_action, &app1_uuid),
+ CALLBACK_STATUS(CB_GATTC_REGISTER_CLIENT, BT_STATUS_SUCCESS),
+ ACTION_SUCCESS(gatt_client_start_scan_action, NULL),
+ CLLBACK_GATTC_SCAN_RES(prop_emu_remotes_default_set, 1, TRUE),
+ ACTION_SUCCESS(gatt_client_stop_scan_action, NULL),
+ ACTION_SUCCESS(gatt_client_connect_action, &app1_conn_req),
+ CALLBACK_GATTC_CONNECT(GATT_STATUS_SUCCESS,
+ prop_emu_remotes_default_set,
+ CONN1_ID, APP1_ID),
+ ACTION_SUCCESS(gatt_client_search_services, &search_services_1),
+ CALLBACK_GATTC_SEARCH_COMPLETE(GATT_STATUS_SUCCESS, CONN1_ID),
+ ACTION_SUCCESS(gatt_client_get_characteristic_action,
+ &get_char_data_1),
+ CALLBACK_GATTC_GET_CHARACTERISTIC_CB(GATT_STATUS_SUCCESS,
+ CONN1_ID, &service_1, &characteristic_1, 4),
+ ACTION_SUCCESS(gatt_client_get_descriptor_action,
+ &get_desc_data_1),
+ CALLBACK_GATTC_GET_DESCRIPTOR(GATT_STATUS_FAILURE, CONN1_ID,
+ &service_1, &characteristic_1, NULL),
+ ACTION_SUCCESS(bluetooth_disable_action, NULL),
+ CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_OFF),
+ ),
TEST_CASE_BREDRLE("Gatt Client - Get Descriptor - Single",
ACTION_SUCCESS(init_pdus, get_descriptor_1),
ACTION_SUCCESS(bluetooth_enable_action, NULL),
--
1.8.4

2015-02-26 09:39:32

by Lukasz Rymanowski

Subject: [PATCH 05/16] attrib/gatt: Improve robustness when searching for characteristics

Without this patch it is possible to enter infinite loop when searching
characteristics on remote device.
This patch fixes that.

Issue happens when remote device replies with ending handle which is
lower than start handle we use for search.

Found on robustness session on UPF50
---
attrib/gatt.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/attrib/gatt.c b/attrib/gatt.c
index 4bffd64..2749c46 100644
--- a/attrib/gatt.c
+++ b/attrib/gatt.c
@@ -72,6 +72,7 @@ struct discover_char {
unsigned int id;
bt_uuid_t *uuid;
uint16_t end;
+ uint16_t start;
GSList *characteristics;
gatt_cb_t cb;
void *user_data;
@@ -629,7 +630,18 @@ static void char_discovered_cb(guint8 status, const guint8 *ipdu, guint16 iplen,

att_data_list_free(list);

- if (last != 0 && (last + 1 < dc->end)) {
+ /*
+ * If last handle is lower from previous start handle then it is smth
+ * wrong. Let's stop search, otherwise we might enter infinite loop.
+ */
+ if (last < dc->start) {
+ err = ATT_ECODE_UNLIKELY;
+ goto done;
+ }
+
+ dc->start = last + 1;
+
+ if (last != 0 && (dc->start < dc->end)) {
bt_uuid_t uuid;
guint16 oplen;
size_t buflen;
@@ -639,7 +651,7 @@ static void char_discovered_cb(guint8 status, const guint8 *ipdu, guint16 iplen,

bt_uuid16_create(&uuid, GATT_CHARAC_UUID);

- oplen = enc_read_by_type_req(last + 1, dc->end, &uuid, buf,
+ oplen = enc_read_by_type_req(dc->start, dc->end, &uuid, buf,
buflen);

if (oplen == 0)
@@ -680,6 +692,7 @@ guint gatt_discover_char(GAttrib *attrib, uint16_t start, uint16_t end,
dc->cb = func;
dc->user_data = user_data;
dc->end = end;
+ dc->start = start;
dc->uuid = g_memdup(uuid, sizeof(bt_uuid_t));

dc->id = g_attrib_send(attrib, 0, buf, plen, char_discovered_cb,
--
1.8.4

2015-02-26 09:39:33

by Lukasz Rymanowski

Subject: [PATCH 06/16] android/gatt: Check status on get characteristic callback

---
android/gatt.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/android/gatt.c b/android/gatt.c
index 53b1983..0296788 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -2725,12 +2725,20 @@ static void discover_char_cb(uint8_t status, GSList *characteristics,
struct discover_char_data *data = user_data;
struct service *srvc = data->service;

+ if (status) {
+ error("gatt: Failed to get characteristics: %s",
+ att_ecode2str(status));
+ convert_send_client_char_notify(NULL, data->conn_id, srvc);
+ goto done;
+ }
+
if (queue_isempty(srvc->chars))
cache_all_srvc_chars(srvc, characteristics);

convert_send_client_char_notify(queue_peek_head(srvc->chars),
data->conn_id, srvc);

+done:
free(data);
}

--
1.8.4

2015-02-26 09:39:31

by Lukasz Rymanowski

Subject: [PATCH 04/16] attrib/gatt: Minor refactor in char_discovered_cb

This will be needed by next patch
---
attrib/gatt.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/attrib/gatt.c b/attrib/gatt.c
index b8d766c..4bffd64 100644
--- a/attrib/gatt.c
+++ b/attrib/gatt.c
@@ -574,10 +574,16 @@ static void char_discovered_cb(guint8 status, const guint8 *ipdu, guint16 iplen,
{
struct discover_char *dc = user_data;
struct att_data_list *list;
- unsigned int i, err = ATT_ECODE_ATTR_NOT_FOUND;
+ unsigned int i, err = 0;
uint16_t last = 0;
uint8_t type;

+ /* We have all the characteristic now, lets send it up */
+ if (status == ATT_ECODE_ATTR_NOT_FOUND) {
+ err = dc->characteristics ? 0 : status;
+ goto done;
+ }
+
if (status) {
err = status;
goto done;
@@ -647,7 +653,6 @@ static void char_discovered_cb(guint8 status, const guint8 *ipdu, guint16 iplen,
}

done:
- err = (dc->characteristics ? 0 : err);
dc->cb(err, dc->characteristics, dc->user_data);
}

--
1.8.4

2015-02-26 09:39:30

by Lukasz Rymanowski

Subject: [PATCH 03/16] android/tester-gatt: Add robustness test for get characteristic

This test makes sure that BlueZ correctly handles incorrect response
from remote device on get characteristic.
---
android/tester-gatt.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)

diff --git a/android/tester-gatt.c b/android/tester-gatt.c
index 6c58bc7..1abfe26 100644
--- a/android/tester-gatt.c
+++ b/android/tester-gatt.c
@@ -907,6 +907,13 @@ static struct iovec get_characteristic_1[] = {
end_pdu
};

+static struct iovec get_characteristic_2[] = {
+ SEARCH_SERVICE_SINGLE_SUCCESS_PDUS,
+ raw_pdu(0x08, 0x01, 0x00, 0x10, 0x00, 0x03, 0x28),
+ raw_pdu(0x09, 0x07, 0x00, 0x00, 0x04, 0x00, 0x00, 0x19, 0x00),
+ end_pdu
+};
+
static struct iovec get_descriptor_1[] = {
SEARCH_SERVICE_SINGLE_SUCCESS_PDUS,
READ_BY_TYPE_SINGLE_CHARACTERISTIC_PDUS,
@@ -2189,6 +2196,31 @@ static struct test_case test_cases[] = {
ACTION_SUCCESS(bluetooth_disable_action, NULL),
CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_OFF),
),
+ TEST_CASE_BREDRLE("Gatt Client - Get Characteristic - Incorrect rsp",
+ ACTION_SUCCESS(init_pdus, get_characteristic_2),
+ ACTION_SUCCESS(bluetooth_enable_action, NULL),
+ CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_ON),
+ ACTION_SUCCESS(emu_setup_powered_remote_action, NULL),
+ ACTION_SUCCESS(emu_set_ssp_mode_action, NULL),
+ ACTION_SUCCESS(emu_set_connect_cb_action, gatt_conn_cb),
+ ACTION_SUCCESS(gatt_client_register_action, &app1_uuid),
+ CALLBACK_STATUS(CB_GATTC_REGISTER_CLIENT, BT_STATUS_SUCCESS),
+ ACTION_SUCCESS(gatt_client_start_scan_action, NULL),
+ CLLBACK_GATTC_SCAN_RES(prop_emu_remotes_default_set, 1, TRUE),
+ ACTION_SUCCESS(gatt_client_stop_scan_action, NULL),
+ ACTION_SUCCESS(gatt_client_connect_action, &app1_conn_req),
+ CALLBACK_GATTC_CONNECT(GATT_STATUS_SUCCESS,
+ prop_emu_remotes_default_set,
+ CONN1_ID, APP1_ID),
+ ACTION_SUCCESS(gatt_client_search_services, &search_services_1),
+ CALLBACK_GATTC_SEARCH_COMPLETE(GATT_STATUS_SUCCESS, CONN1_ID),
+ ACTION_SUCCESS(gatt_client_get_characteristic_action,
+ &get_char_data_1),
+ CALLBACK_GATTC_GET_CHARACTERISTIC_CB(GATT_STATUS_FAILURE,
+ CONN1_ID, &service_1, NULL, 0),
+ ACTION_SUCCESS(bluetooth_disable_action, NULL),
+ CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_OFF),
+ ),
TEST_CASE_BREDRLE("Gatt Client - Get Characteristic - None",
ACTION_SUCCESS(init_pdus, get_characteristic_1),
ACTION_SUCCESS(bluetooth_enable_action, NULL),
--
1.8.4

2015-02-26 09:39:29

by Lukasz Rymanowski

Subject: [PATCH 02/16] attrib/gatt: Improve robustness on search primary services

Without this it is possible to put BlueZ into infinite loop when
doing primary service search on remote device.

Issue happens when remote device replies with ending handle which is
lower than start handle we use for search.

This patch fixes that

Found on robustness session on UPF50
---
attrib/gatt.c | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/attrib/gatt.c b/attrib/gatt.c
index aafd3f7..b8d766c 100644
--- a/attrib/gatt.c
+++ b/attrib/gatt.c
@@ -43,6 +43,7 @@ struct discover_primary {
GAttrib *attrib;
unsigned int id;
bt_uuid_t uuid;
+ uint16_t start;
GSList *primaries;
gatt_cb_t cb;
void *user_data;
@@ -255,8 +256,19 @@ static void primary_by_uuid_cb(guint8 status, const guint8 *ipdu,
if (range->end == 0xffff)
goto done;

+ /*
+ * If last handle is lower from previous start handle then it is smth
+ * wrong. Let's stop search, otherwise we might enter infinite loop.
+ */
+ if (range->end < dp->start) {
+ err = ATT_ECODE_UNLIKELY;
+ goto done;
+ }
+
+ dp->start = range->end + 1;
+
buf = g_attrib_get_buffer(dp->attrib, &buflen);
- oplen = encode_discover_primary(range->end + 1, 0xffff, &dp->uuid,
+ oplen = encode_discover_primary(dp->start, 0xffff, &dp->uuid,
buf, buflen);

if (oplen == 0)
@@ -325,12 +337,24 @@ static void primary_all_cb(guint8 status, const guint8 *ipdu, guint16 iplen,
att_data_list_free(list);
err = 0;

+ /*
+ * If last handle is lower from previous start handle then it is smth
+ * wrong. Let's stop search, otherwise we might enter infinite loop.
+ */
+ if (end < dp->start) {
+ err = ATT_ECODE_UNLIKELY;
+ goto done;
+ }
+
+ dp->start = end + 1;
+
if (end != 0xffff) {
size_t buflen;
uint8_t *buf = g_attrib_get_buffer(dp->attrib, &buflen);
- guint16 oplen = encode_discover_primary(end + 1, 0xffff, NULL,
+ guint16 oplen = encode_discover_primary(dp->start, 0xffff, NULL,
buf, buflen);

+
g_attrib_send(dp->attrib, dp->id, buf, oplen, primary_all_cb,
discover_primary_ref(dp),
discover_primary_unref);
@@ -362,6 +386,7 @@ guint gatt_discover_primary(GAttrib *attrib, bt_uuid_t *uuid, gatt_cb_t func,
dp->attrib = g_attrib_ref(attrib);
dp->cb = func;
dp->user_data = user_data;
+ dp->start = 0x0001;

if (uuid) {
dp->uuid = *uuid;
--
1.8.4

2015-02-26 09:39:28

by Lukasz Rymanowski

Subject: [PATCH 01/16] android/tester-gatt: Add robustness test for search services

This test verifies if BlueZ do not start infinitive service search
when remote device response with incorrect end handle in Read By
Group response
---
android/tester-gatt.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)

diff --git a/android/tester-gatt.c b/android/tester-gatt.c
index eb0ca1e..6c58bc7 100644
--- a/android/tester-gatt.c
+++ b/android/tester-gatt.c
@@ -895,6 +895,12 @@ static struct iovec search_service_3[] = {
end_pdu
};

+static struct iovec search_service_4[] = {
+ raw_pdu(0x10, 0x01, 0x00, 0xff, 0xff, 0x00, 0x28),
+ raw_pdu(0x11, 0x06, 0x01, 0x00, 0x00, 0x00, 0x00, 0x18),
+ end_pdu
+};
+
static struct iovec get_characteristic_1[] = {
SEARCH_SERVICE_SINGLE_SUCCESS_PDUS,
READ_BY_TYPE_SINGLE_CHARACTERISTIC_PDUS,
@@ -2137,6 +2143,27 @@ static struct test_case test_cases[] = {
ACTION_SUCCESS(bluetooth_disable_action, NULL),
CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_OFF),
),
+ TEST_CASE_BREDRLE("Gatt Client - Search Service - Incorrect rsp",
+ ACTION_SUCCESS(init_pdus, search_service_4),
+ ACTION_SUCCESS(bluetooth_enable_action, NULL),
+ CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_ON),
+ ACTION_SUCCESS(emu_setup_powered_remote_action, NULL),
+ ACTION_SUCCESS(emu_set_ssp_mode_action, NULL),
+ ACTION_SUCCESS(emu_set_connect_cb_action, gatt_conn_cb),
+ ACTION_SUCCESS(gatt_client_register_action, &app1_uuid),
+ CALLBACK_STATUS(CB_GATTC_REGISTER_CLIENT, BT_STATUS_SUCCESS),
+ ACTION_SUCCESS(gatt_client_start_scan_action, NULL),
+ CLLBACK_GATTC_SCAN_RES(prop_emu_remotes_default_set, 1, TRUE),
+ ACTION_SUCCESS(gatt_client_stop_scan_action, NULL),
+ ACTION_SUCCESS(gatt_client_connect_action, &app1_conn_req),
+ CALLBACK_GATTC_CONNECT(GATT_STATUS_SUCCESS,
+ prop_emu_remotes_default_set,
+ CONN1_ID, APP1_ID),
+ ACTION_SUCCESS(gatt_client_search_services, &search_services_1),
+ CALLBACK_GATTC_SEARCH_COMPLETE(GATT_STATUS_SUCCESS, CONN1_ID),
+ ACTION_SUCCESS(bluetooth_disable_action, NULL),
+ CALLBACK_STATE(CB_BT_ADAPTER_STATE_CHANGED, BT_STATE_OFF),
+ ),
TEST_CASE_BREDRLE("Gatt Client - Get Characteristic - Single",
ACTION_SUCCESS(init_pdus, get_characteristic_1),
ACTION_SUCCESS(bluetooth_enable_action, NULL),
--
1.8.4