Hi Archie,
On Wed, May 6, 2020 at 11:38 PM Archie Pusaka <apusaka@xxxxxxxxxx> wrote:
>
> From: Archie Pusaka <apusaka@xxxxxxxxxxxx>
>
> 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.
--
Luiz Augusto von Dentz
