diff mbox series

[Bluez,v2] shared/gatt-server: Fix read multiple charc values

Message ID 20200507143805.Bluez.v2.1.I45da219fa28f72db2e11778c15cee5aad446b1c4@changeid (mailing list archive)
State Accepted
Delegated to: Luiz Von Dentz
Headers show
Series [Bluez,v2] shared/gatt-server: Fix read multiple charc values | expand

Commit Message

Archie Pusaka May 7, 2020, 6:38 a.m. UTC 
From: Archie Pusaka <apusaka@chromium.org>

According to bluetooth spec Ver 5.2, Vol 3, Part G, 4.8.4, An
ATT_ERROR_RSP PDU shall be sent by the server in response to the
ATT_READ_MULTIPLE_RSP PDU if insufficient authentication,
insufficient authorization, insufficient encryption key size, or
insufficient encryption is used by the client, or if a read operation
is not permitted on any of the Characteristic Values.

Currently if the size of the response grows larger than the MTU size,
BlueZ does an early return and not check the permission for the rest
of the characteristics. This patch forces BlueZ to check for possible
errors even though we already reach MTU size.

This patch also moves the read permission check for read multiple
characteristics so it is done before actually retrieving the
characteristics.
---

Changes in v2:
- Fix error underflowing unsigned int

 src/shared/gatt-server.c | 88 ++++++++++++++++++++--------------------
 1 file changed, 45 insertions(+), 43 deletions(-)

Comments

Luiz Augusto von Dentz May 11, 2020, 4:16 p.m. UTC | #1 
Hi Archie,

On Wed, May 6, 2020 at 11:38 PM Archie Pusaka <apusaka@google.com> wrote:
>
> From: Archie Pusaka <apusaka@chromium.org>
>
> According to bluetooth spec Ver 5.2, Vol 3, Part G, 4.8.4, An
> ATT_ERROR_RSP PDU shall be sent by the server in response to the
> ATT_READ_MULTIPLE_RSP PDU if insufficient authentication,
> insufficient authorization, insufficient encryption key size, or
> insufficient encryption is used by the client, or if a read operation
> is not permitted on any of the Characteristic Values.
>
> Currently if the size of the response grows larger than the MTU size,
> BlueZ does an early return and not check the permission for the rest
> of the characteristics. This patch forces BlueZ to check for possible
> errors even though we already reach MTU size.
>
> This patch also moves the read permission check for read multiple
> characteristics so it is done before actually retrieving the
> characteristics.
> ---
>
> Changes in v2:
> - Fix error underflowing unsigned int
>
>  src/shared/gatt-server.c | 88 ++++++++++++++++++++--------------------
>  1 file changed, 45 insertions(+), 43 deletions(-)
>
> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> index 4e07398d2..28ac2d68d 100644
> --- a/src/shared/gatt-server.c
> +++ b/src/shared/gatt-server.c
> @@ -1057,33 +1057,23 @@ static void read_multiple_complete_cb(struct gatt_db_attribute *attr, int err,
>         uint16_t length;
>
>         if (err != 0) {
> -               bt_att_chan_send_error_rsp(data->chan, data->opcode, handle,
> -                                                                       err);
> -               read_mult_data_free(data);
> -               return;
> -       }
> -
> -       ecode = check_permissions(data->server, attr, BT_ATT_PERM_READ |
> -                                               BT_ATT_PERM_READ_AUTHEN |
> -                                               BT_ATT_PERM_READ_ENCRYPT);
> -       if (ecode) {
> -               bt_att_chan_send_error_rsp(data->chan, data->opcode, handle,
> -                                                                       ecode);
> -               read_mult_data_free(data);
> -               return;
> +               ecode = err;
> +               goto error;
>         }
>
>         length = data->opcode == BT_ATT_OP_READ_MULT_VL_REQ ?
> -                       MIN(len, data->mtu - data->length - 3) :
> +                       MIN(len, MAX(data->mtu - data->length, 3) - 3) :
>                         MIN(len, data->mtu - data->length - 1);
>
>         if (data->opcode == BT_ATT_OP_READ_MULT_VL_REQ) {
>                 /* The Length Value Tuple List may be truncated within the first
>                  * two octets of a tuple due to the size limits of the current
> -                * ATT_MTU.
> +                * ATT_MTU, but the first two octets cannot be separated.
>                  */
> -               put_le16(len, data->rsp_data + data->length);
> -               data->length += 2;
> +               if (data->mtu - data->length >= 3) {
> +                       put_le16(len, data->rsp_data + data->length);
> +                       data->length += 2;
> +               }
>         }
>
>         memcpy(data->rsp_data + data->length, value, length);
> @@ -1091,45 +1081,46 @@ static void read_multiple_complete_cb(struct gatt_db_attribute *attr, int err,
>
>         data->cur_handle++;
>
> -       len = data->opcode == BT_ATT_OP_READ_MULT_VL_REQ ?
> -               data->mtu - data->length - 3 : data->mtu - data->length - 1;
> -
> -       if (!len || (data->cur_handle == data->num_handles)) {
> +       if (data->cur_handle == data->num_handles) {
>                 bt_att_chan_send_rsp(data->chan, data->opcode + 1,
>                                                 data->rsp_data, data->length);
>                 read_mult_data_free(data);
>                 return;
>         }
>
> +       handle = data->handles[data->cur_handle];
> +
>         util_debug(data->server->debug_callback, data->server->debug_data,
>                                 "%s Req - #%zu of %zu: 0x%04x",
>                                 data->opcode == BT_ATT_OP_READ_MULT_REQ ?
>                                 "Read Multiple" :
>                                 "Read Multiple Variable Length",
>                                 data->cur_handle + 1, data->num_handles,
> -                               data->handles[data->cur_handle]);
> +                               handle);
>
> -       next_attr = gatt_db_get_attribute(data->server->db,
> -                                       data->handles[data->cur_handle]);
> +       next_attr = gatt_db_get_attribute(data->server->db, handle);
>
>         if (!next_attr) {
> -               bt_att_chan_send_error_rsp(data->chan,
> -                                       BT_ATT_OP_READ_MULT_REQ,
> -                                       data->handles[data->cur_handle],
> -                                       BT_ATT_ERROR_INVALID_HANDLE);
> -               read_mult_data_free(data);
> -               return;
> +               ecode = BT_ATT_ERROR_INVALID_HANDLE;
> +               goto error;
>         }
>
> -       if (!gatt_db_attribute_read(next_attr, 0, BT_ATT_OP_READ_MULT_REQ,
> +       ecode = check_permissions(data->server, next_attr, BT_ATT_PERM_READ |
> +                                               BT_ATT_PERM_READ_AUTHEN |
> +                                               BT_ATT_PERM_READ_ENCRYPT);
> +       if (ecode)
> +               goto error;
> +
> +       if (gatt_db_attribute_read(next_attr, 0, data->opcode,
>                                         data->server->att,
> -                                       read_multiple_complete_cb, data)) {
> -               bt_att_chan_send_error_rsp(data->chan,
> -                                               BT_ATT_OP_READ_MULT_REQ,
> -                                               data->handles[data->cur_handle],
> -                                               BT_ATT_ERROR_UNLIKELY);
> -               read_mult_data_free(data);
> -       }
> +                                       read_multiple_complete_cb, data))
> +               return;
> +
> +       ecode = BT_ATT_ERROR_UNLIKELY;
> +
> +error:
> +       bt_att_chan_send_error_rsp(data->chan, data->opcode, handle, ecode);
> +       read_mult_data_free(data);
>  }
>
>  static struct read_mult_data *read_mult_data_new(struct bt_gatt_server *server,
> @@ -1161,8 +1152,9 @@ static void read_multiple_cb(struct bt_att_chan *chan, uint8_t opcode,
>         struct bt_gatt_server *server = user_data;
>         struct gatt_db_attribute *attr;
>         struct read_mult_data *data = NULL;
> -       uint8_t ecode = BT_ATT_ERROR_UNLIKELY;
> +       uint8_t ecode;
>         size_t i = 0;
> +       uint16_t handle = 0;
>
>         if (length < 4) {
>                 ecode = BT_ATT_ERROR_INVALID_PDU;
> @@ -1176,28 +1168,38 @@ static void read_multiple_cb(struct bt_att_chan *chan, uint8_t opcode,
>         for (i = 0; i < data->num_handles; i++)
>                 data->handles[i] = get_le16(pdu + i * 2);
>
> +       handle = data->handles[0];
> +
>         util_debug(server->debug_callback, server->debug_data,
>                         "%s Req - %zu handles, 1st: 0x%04x",
>                         data->opcode == BT_ATT_OP_READ_MULT_REQ ?
>                         "Read Multiple" : "Read Multiple Variable Length",
> -                       data->num_handles, data->handles[0]);
> +                       data->num_handles, handle);
>
> -       attr = gatt_db_get_attribute(server->db, data->handles[0]);
> +       attr = gatt_db_get_attribute(server->db, handle);
>
>         if (!attr) {
>                 ecode = BT_ATT_ERROR_INVALID_HANDLE;
>                 goto error;
>         }
>
> +       ecode = check_permissions(data->server, attr, BT_ATT_PERM_READ |
> +                                               BT_ATT_PERM_READ_AUTHEN |
> +                                               BT_ATT_PERM_READ_ENCRYPT);
> +       if (ecode)
> +               goto error;
> +
>         if (gatt_db_attribute_read(attr, 0, opcode, server->att,
>                                         read_multiple_complete_cb, data))
>                 return;
>
> +       ecode = BT_ATT_ERROR_UNLIKELY;
> +
>  error:
>         if (data)
>                 read_mult_data_free(data);
>
> -       bt_att_chan_send_error_rsp(chan, opcode, 0, ecode);
> +       bt_att_chan_send_error_rsp(chan, opcode, handle, ecode);
>  }
>
>  static bool append_prep_data(struct prep_write_data *prep_data, uint16_t handle,
> --
> 2.26.2.526.g744177e7f7-goog

Applied, thanks.
diff mbox series

Patch

diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
index 4e07398d2..28ac2d68d 100644
--- a/src/shared/gatt-server.c
+++ b/src/shared/gatt-server.c
@@ -1057,33 +1057,23 @@  static void read_multiple_complete_cb(struct gatt_db_attribute *attr, int err,
 	uint16_t length;
 
 	if (err != 0) {
-		bt_att_chan_send_error_rsp(data->chan, data->opcode, handle,
-									err);
-		read_mult_data_free(data);
-		return;
-	}
-
-	ecode = check_permissions(data->server, attr, BT_ATT_PERM_READ |
-						BT_ATT_PERM_READ_AUTHEN |
-						BT_ATT_PERM_READ_ENCRYPT);
-	if (ecode) {
-		bt_att_chan_send_error_rsp(data->chan, data->opcode, handle,
-									ecode);
-		read_mult_data_free(data);
-		return;
+		ecode = err;
+		goto error;
 	}
 
 	length = data->opcode == BT_ATT_OP_READ_MULT_VL_REQ ?
-			MIN(len, data->mtu - data->length - 3) :
+			MIN(len, MAX(data->mtu - data->length, 3) - 3) :
 			MIN(len, data->mtu - data->length - 1);
 
 	if (data->opcode == BT_ATT_OP_READ_MULT_VL_REQ) {
 		/* The Length Value Tuple List may be truncated within the first
 		 * two octets of a tuple due to the size limits of the current
-		 * ATT_MTU.
+		 * ATT_MTU, but the first two octets cannot be separated.
 		 */
-		put_le16(len, data->rsp_data + data->length);
-		data->length += 2;
+		if (data->mtu - data->length >= 3) {
+			put_le16(len, data->rsp_data + data->length);
+			data->length += 2;
+		}
 	}
 
 	memcpy(data->rsp_data + data->length, value, length);
@@ -1091,45 +1081,46 @@  static void read_multiple_complete_cb(struct gatt_db_attribute *attr, int err,
 
 	data->cur_handle++;
 
-	len = data->opcode == BT_ATT_OP_READ_MULT_VL_REQ ?
-		data->mtu - data->length - 3 : data->mtu - data->length - 1;
-
-	if (!len || (data->cur_handle == data->num_handles)) {
+	if (data->cur_handle == data->num_handles) {
 		bt_att_chan_send_rsp(data->chan, data->opcode + 1,
 						data->rsp_data, data->length);
 		read_mult_data_free(data);
 		return;
 	}
 
+	handle = data->handles[data->cur_handle];
+
 	util_debug(data->server->debug_callback, data->server->debug_data,
 				"%s Req - #%zu of %zu: 0x%04x",
 				data->opcode == BT_ATT_OP_READ_MULT_REQ ?
 				"Read Multiple" :
 				"Read Multiple Variable Length",
 				data->cur_handle + 1, data->num_handles,
-				data->handles[data->cur_handle]);
+				handle);
 
-	next_attr = gatt_db_get_attribute(data->server->db,
-					data->handles[data->cur_handle]);
+	next_attr = gatt_db_get_attribute(data->server->db, handle);
 
 	if (!next_attr) {
-		bt_att_chan_send_error_rsp(data->chan,
-					BT_ATT_OP_READ_MULT_REQ,
-					data->handles[data->cur_handle],
-					BT_ATT_ERROR_INVALID_HANDLE);
-		read_mult_data_free(data);
-		return;
+		ecode = BT_ATT_ERROR_INVALID_HANDLE;
+		goto error;
 	}
 
-	if (!gatt_db_attribute_read(next_attr, 0, BT_ATT_OP_READ_MULT_REQ,
+	ecode = check_permissions(data->server, next_attr, BT_ATT_PERM_READ |
+						BT_ATT_PERM_READ_AUTHEN |
+						BT_ATT_PERM_READ_ENCRYPT);
+	if (ecode)
+		goto error;
+
+	if (gatt_db_attribute_read(next_attr, 0, data->opcode,
 					data->server->att,
-					read_multiple_complete_cb, data)) {
-		bt_att_chan_send_error_rsp(data->chan,
-						BT_ATT_OP_READ_MULT_REQ,
-						data->handles[data->cur_handle],
-						BT_ATT_ERROR_UNLIKELY);
-		read_mult_data_free(data);
-	}
+					read_multiple_complete_cb, data))
+		return;
+
+	ecode = BT_ATT_ERROR_UNLIKELY;
+
+error:
+	bt_att_chan_send_error_rsp(data->chan, data->opcode, handle, ecode);
+	read_mult_data_free(data);
 }
 
 static struct read_mult_data *read_mult_data_new(struct bt_gatt_server *server,
@@ -1161,8 +1152,9 @@  static void read_multiple_cb(struct bt_att_chan *chan, uint8_t opcode,
 	struct bt_gatt_server *server = user_data;
 	struct gatt_db_attribute *attr;
 	struct read_mult_data *data = NULL;
-	uint8_t ecode = BT_ATT_ERROR_UNLIKELY;
+	uint8_t ecode;
 	size_t i = 0;
+	uint16_t handle = 0;
 
 	if (length < 4) {
 		ecode = BT_ATT_ERROR_INVALID_PDU;
@@ -1176,28 +1168,38 @@  static void read_multiple_cb(struct bt_att_chan *chan, uint8_t opcode,
 	for (i = 0; i < data->num_handles; i++)
 		data->handles[i] = get_le16(pdu + i * 2);
 
+	handle = data->handles[0];
+
 	util_debug(server->debug_callback, server->debug_data,
 			"%s Req - %zu handles, 1st: 0x%04x",
 			data->opcode == BT_ATT_OP_READ_MULT_REQ ?
 			"Read Multiple" : "Read Multiple Variable Length",
-			data->num_handles, data->handles[0]);
+			data->num_handles, handle);
 
-	attr = gatt_db_get_attribute(server->db, data->handles[0]);
+	attr = gatt_db_get_attribute(server->db, handle);
 
 	if (!attr) {
 		ecode = BT_ATT_ERROR_INVALID_HANDLE;
 		goto error;
 	}
 
+	ecode = check_permissions(data->server, attr, BT_ATT_PERM_READ |
+						BT_ATT_PERM_READ_AUTHEN |
+						BT_ATT_PERM_READ_ENCRYPT);
+	if (ecode)
+		goto error;
+
 	if (gatt_db_attribute_read(attr, 0, opcode, server->att,
 					read_multiple_complete_cb, data))
 		return;
 
+	ecode = BT_ATT_ERROR_UNLIKELY;
+
 error:
 	if (data)
 		read_mult_data_free(data);
 
-	bt_att_chan_send_error_rsp(chan, opcode, 0, ecode);
+	bt_att_chan_send_error_rsp(chan, opcode, handle, ecode);
 }
 
 static bool append_prep_data(struct prep_write_data *prep_data, uint16_t handle,