[PATCH v2 0/5] Improve robustness in Gatt Client

[PATCH v2 0/5] Improve robustness in Gatt Client

[Szymon Janc]

Hi,

this is fixed and rebased version of Lukasz's patches from few weeks back.

>From original cover letter:
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


BR
Szymon

Lukasz Rymanowski (5):
  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

 src/shared/gatt-helpers.c | 89 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 83 insertions(+), 6 deletions(-)

-- 
1.9.3

[PATCH v2 1/5] shated/gatt-helpers: Improve robustness of search service

[Szymon Janc]

From: Lukasz Rymanowski <lukasz.rymanowski@tieto.com>

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 2d6088e..bbe1de9 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 bt_gatt_request {
 	struct bt_att *att;
 	unsigned int id;
+	uint16_t start_handle;
 	uint16_t end_handle;
 	int ref_count;
 	bt_uuid_t uuid;
@@ -690,10 +691,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);
 
@@ -765,10 +778,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);
@@ -810,6 +835,7 @@ static struct bt_gatt_request *discover_services(struct bt_att *att,
 		return NULL;
 
 	op->att = att;
+	op->start_handle = start;
 	op->end_handle = end;
 	op->callback = callback;
 	op->user_data = user_data;
-- 
1.9.3

[PATCH v2 2/5] shared/gatt-helpers: Improve robustness of get characteristics

[Szymon Janc]

From: Lukasz Rymanowski <lukasz.rymanowski@tieto.com>

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 bbe1de9..4e4c7eb 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -1235,10 +1235,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);
 
@@ -1281,6 +1293,7 @@ struct bt_gatt_request *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.9.3

[PATCH v2 3/5] shared/gatt-helpers: Improve robustness of get include services

[Szymon Janc]

From: Lukasz Rymanowski <lukasz.rymanowski@tieto.com>

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 4e4c7eb..78aca7d 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -1125,10 +1125,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);
 
@@ -1171,6 +1182,7 @@ struct bt_gatt_request *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.9.3

[PATCH v2 4/5] shared/gatt-helpers: Improve robustness read by type request

[Szymon Janc]

From: Lukasz Rymanowski <lukasz.rymanowski@tieto.com>

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 78aca7d..87a2be7 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -1364,10 +1364,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 = false;
+		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);
 
@@ -1409,6 +1421,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.9.3

[PATCH v2 5/5] shared/gatt-helpers: Improve robustness of get descriptors

[Szymon Janc]

From: Lukasz Rymanowski <lukasz.rymanowski@tieto.com>

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 87a2be7..a782265 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -1494,10 +1494,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,
@@ -1539,6 +1551,7 @@ struct bt_gatt_request *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.9.3

Re: [PATCH v2 0/5] Improve robustness in Gatt Client

[Luiz Augusto von Dentz]

Hi Szymon,

On Thu, Mar 19, 2015 at 11:56 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
> Hi,
>
> this is fixed and rebased version of Lukasz's patches from few weeks back.
>
> From original cover letter:
> 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
>
>
> BR
> Szymon
>
> Lukasz Rymanowski (5):
>   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
>
>  src/shared/gatt-helpers.c | 89 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 83 insertions(+), 6 deletions(-)
>
> --
> 1.9.3

Pushed, thanks.


-- 
Luiz Augusto von Dentz