[PATCH BlueZ v4 1/4] client: Fix reading long values

[PATCH BlueZ v4 1/4] client: Fix reading long values

[Grzegorz Kolodziejczyk]

While value has more than single MTU can carry long read procedure will
be triggered. In such cases offset need to bo considered while getting
value from storage.
---
 client/gatt.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 47 insertions(+), 2 deletions(-)

diff --git a/client/gatt.c b/client/gatt.c
index 8c818d8c1..7a6035ac1 100644
--- a/client/gatt.c
+++ b/client/gatt.c
@@ -1412,6 +1412,39 @@ static const GDBusPropertyTable chrc_properties[] = {
 	{ }
 };
 
+static int parse_offset(DBusMessageIter *iter, uint16_t *offset)
+{
+	DBusMessageIter dict;
+
+	if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_ARRAY)
+		return -EINVAL;
+
+	dbus_message_iter_recurse(iter, &dict);
+
+	while (dbus_message_iter_get_arg_type(&dict) == DBUS_TYPE_DICT_ENTRY) {
+		const char *key;
+		DBusMessageIter value, entry;
+		int var;
+
+		dbus_message_iter_recurse(&dict, &entry);
+		dbus_message_iter_get_basic(&entry, &key);
+
+		dbus_message_iter_next(&entry);
+		dbus_message_iter_recurse(&entry, &value);
+
+		var = dbus_message_iter_get_arg_type(&value);
+		if (strcasecmp(key, "offset") == 0) {
+			if (var != DBUS_TYPE_UINT16)
+				return -EINVAL;
+			dbus_message_iter_get_basic(&value, offset);
+		}
+
+		dbus_message_iter_next(&dict);
+	}
+
+	return 0;
+}
+
 static DBusMessage *read_value(DBusMessage *msg, uint8_t *value,
 						uint16_t value_len)
 {
@@ -1433,8 +1466,14 @@ static DBusMessage *chrc_read_value(DBusConnection *conn, DBusMessage *msg,
 							void *user_data)
 {
 	struct chrc *chrc = user_data;
+	DBusMessageIter iter;
+	uint16_t offset = 0;
+
+	dbus_message_iter_init(msg, &iter);
+
+	parse_offset(&iter, &offset);
 
-	return read_value(msg, chrc->value, chrc->value_len);
+	return read_value(msg, &chrc->value[offset], chrc->value_len - offset);
 }
 
 static int parse_value_arg(DBusMessageIter *iter, uint8_t **value, int *len)
@@ -1785,8 +1824,14 @@ static DBusMessage *desc_read_value(DBusConnection *conn, DBusMessage *msg,
 							void *user_data)
 {
 	struct desc *desc = user_data;
+	DBusMessageIter iter;
+	uint16_t offset = 0;
+
+	dbus_message_iter_init(msg, &iter);
+
+	parse_offset(&iter, &offset);
 
-	return read_value(msg, desc->value, desc->value_len);
+	return read_value(msg, &desc->value[offset], desc->value_len - offset);
 }
 
 static DBusMessage *desc_write_value(DBusConnection *conn, DBusMessage *msg,
-- 
2.13.6

[PATCH BlueZ v4 2/4] gatt: Add org.bluez.Error.InvalidOffset for long read procedure

[Grzegorz Kolodziejczyk]

This patch adds handling of invalid offset error for gatt database in
case if offset in read blob would be invalid.

"The Read Blob Request is repeated until the Read Blob Response’s Part
Attribute Value parameter is zero or an Error Response is sent by the server
with the Error Code set to Invalid Offset." Bluetooth Core 5.0, 4.12.2

"If the prepare Value Offset is greater than the current length of the attribute
value then all pending prepare write values shall be discarded for this client,
the queue shall be cleared and then an Error Response shall be sent with the
«Invalid Offset»." Bluetooth Core 5.0, 3.4.6.3
---
 doc/gatt-api.txt    | 1 +
 src/gatt-database.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/doc/gatt-api.txt b/doc/gatt-api.txt
index 9579381a5..f58de23c3 100644
--- a/doc/gatt-api.txt
+++ b/doc/gatt-api.txt
@@ -74,6 +74,7 @@ Methods		array{byte} ReadValue(dict options)
 					 org.bluez.Error.InProgress
 					 org.bluez.Error.NotPermitted
 					 org.bluez.Error.NotAuthorized
+					 org.bluez.Error.InvalidOffset
 					 org.bluez.Error.NotSupported
 
 		void WriteValue(array{byte} value, dict options)
diff --git a/src/gatt-database.c b/src/gatt-database.c
index 1903e08fd..ce0e03f0f 100644
--- a/src/gatt-database.c
+++ b/src/gatt-database.c
@@ -1692,6 +1692,9 @@ static uint8_t dbus_error_to_att_ecode(const char *error_name)
 	if (strcmp(error_name, "org.bluez.Error.InvalidValueLength") == 0)
 		return BT_ATT_ERROR_INVALID_ATTRIBUTE_VALUE_LEN;
 
+	if (strcmp(error_name, "org.bluez.Error.InvalidOffset") == 0)
+		return BT_ATT_ERROR_INVALID_OFFSET;
+
 	if (strcmp(error_name, "org.bluez.Error.InProgress") == 0)
 		return BT_ERROR_ALREADY_IN_PROGRESS;
 
-- 
2.13.6

[PATCH BlueZ v4 3/4] client: Update read callbacks with invalid offset error handlers

[Grzegorz Kolodziejczyk]

This patch adds invalid offset handlers to read callbacks of attributes.
---
 client/gatt.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/client/gatt.c b/client/gatt.c
index 7a6035ac1..3fa490b1a 100644
--- a/client/gatt.c
+++ b/client/gatt.c
@@ -1473,6 +1473,10 @@ static DBusMessage *chrc_read_value(DBusConnection *conn, DBusMessage *msg,
 
 	parse_offset(&iter, &offset);
 
+	if (offset > chrc->value_len)
+		return g_dbus_create_error(msg, "org.bluez.Error.InvalidOffset",
+									NULL);
+
 	return read_value(msg, &chrc->value[offset], chrc->value_len - offset);
 }
 
@@ -1831,6 +1835,10 @@ static DBusMessage *desc_read_value(DBusConnection *conn, DBusMessage *msg,
 
 	parse_offset(&iter, &offset);
 
+	if (offset > desc->value_len)
+		return g_dbus_create_error(msg, "org.bluez.Error.InvalidOffset",
+									NULL);
+
 	return read_value(msg, &desc->value[offset], desc->value_len - offset);
 }
 
-- 
2.13.6

[PATCH BlueZ v4 4/4] client: Add authorization request handling for attribute operations

[Grzegorz Kolodziejczyk]

This patch adds optional authorization request for reading, writing of
gatt database attributes.
---
 client/gatt.c | 128 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 client/main.c |  11 +++--
 2 files changed, 136 insertions(+), 3 deletions(-)

diff --git a/client/gatt.c b/client/gatt.c
index 3fa490b1a..930a64699 100644
--- a/client/gatt.c
+++ b/client/gatt.c
@@ -75,6 +75,8 @@ struct chrc {
 	uint16_t mtu;
 	struct io *write_io;
 	struct io *notify_io;
+	bool authorization_req;
+	bool authorized;
 };
 
 struct service {
@@ -91,6 +93,7 @@ static GList *characteristics;
 static GList *descriptors;
 static GList *managers;
 static GList *uuids;
+static DBusMessage *pending_message = NULL;
 
 struct pipe_io {
 	GDBusProxy *proxy;
@@ -1462,17 +1465,81 @@ static DBusMessage *read_value(DBusMessage *msg, uint8_t *value,
 	return reply;
 }
 
+struct authorize_attribute_data {
+	DBusConnection *conn;
+	void *attribute;
+	uint16_t offset;
+};
+
+static void authorize_read_response(const char *input, void *user_data)
+{
+	struct authorize_attribute_data *aad = user_data;
+	struct chrc *chrc = aad->attribute;
+	DBusMessage *reply;
+	char *err;
+
+	if (!strcmp(input, "no")) {
+		err = "org.bluez.Error.NotAuthorized";
+
+		goto error;
+	}
+
+	if (aad->offset > chrc->value_len) {
+		err = "org.bluez.Error.InvalidOffset";
+
+		goto error;
+	}
+
+	reply = read_value(pending_message, &chrc->value[aad->offset],
+						chrc->value_len - aad->offset);
+
+	chrc->authorized = true;
+
+	g_dbus_send_message(aad->conn, reply);
+
+	g_free(aad);
+
+	return;
+
+error:
+	g_dbus_send_error(aad->conn, pending_message, err, NULL);
+	g_free(aad);
+}
+
 static DBusMessage *chrc_read_value(DBusConnection *conn, DBusMessage *msg,
 							void *user_data)
 {
 	struct chrc *chrc = user_data;
 	DBusMessageIter iter;
 	uint16_t offset = 0;
+	char *str;
 
 	dbus_message_iter_init(msg, &iter);
 
 	parse_offset(&iter, &offset);
 
+	if (chrc->authorization_req && offset == 0)
+		chrc->authorized = false;
+
+	if (chrc->authorization_req && !chrc->authorized) {
+		struct authorize_attribute_data *aad;
+
+		aad = g_new0(struct authorize_attribute_data, 1);
+		aad->conn = conn;
+		aad->attribute = chrc;
+		aad->offset = offset;
+
+		str = g_strdup_printf("Authorize attribute(%s) read (yes/no):",
+								chrc->path);
+
+		bt_shell_prompt_input("gatt", str, authorize_read_response,
+									aad);
+
+		pending_message = dbus_message_ref(msg);
+
+		return NULL;
+	}
+
 	if (offset > chrc->value_len)
 		return g_dbus_create_error(msg, "org.bluez.Error.InvalidOffset",
 									NULL);
@@ -1493,14 +1560,74 @@ static int parse_value_arg(DBusMessageIter *iter, uint8_t **value, int *len)
 	return 0;
 }
 
+static void authorize_write_response(const char *input, void *user_data)
+{
+	struct authorize_attribute_data *aad = user_data;
+	struct chrc *chrc = aad->attribute;
+	DBusMessageIter iter;
+	DBusMessage *reply;
+	char *err;
+
+	dbus_message_iter_init(pending_message, &iter);
+
+	if (!strcmp(input, "no")) {
+		err = "org.bluez.Error.NotAuthorized";
+
+		goto error;
+	}
+
+	chrc->authorized = true;
+
+	if (parse_value_arg(&iter, &chrc->value, &chrc->value_len)) {
+		err = "org.bluez.Error.InvalidArguments";
+
+		goto error;
+	}
+
+	bt_shell_printf("[" COLORED_CHG "] Attribute %s written" , chrc->path);
+
+	g_dbus_emit_property_changed(aad->conn, chrc->path, CHRC_INTERFACE,
+								"Value");
+
+	reply = g_dbus_create_reply(pending_message, DBUS_TYPE_INVALID);
+	g_dbus_send_message(aad->conn, reply);
+
+	g_free(aad);
+
+	return;
+
+error:
+	g_dbus_send_error(aad->conn, pending_message, err, NULL);
+	g_free(aad);
+}
+
 static DBusMessage *chrc_write_value(DBusConnection *conn, DBusMessage *msg,
 							void *user_data)
 {
 	struct chrc *chrc = user_data;
 	DBusMessageIter iter;
+	char *str;
 
 	dbus_message_iter_init(msg, &iter);
 
+	if (chrc->authorization_req && !chrc->authorized) {
+		struct authorize_attribute_data *aad;
+
+		aad = g_new0(struct authorize_attribute_data, 1);
+		aad->conn = conn;
+		aad->attribute = chrc;
+
+		str = g_strdup_printf("Authorize attribute(%s) write (yes/no):",
+								chrc->path);
+
+		bt_shell_prompt_input("gatt", str, authorize_write_response,
+									aad);
+
+		pending_message = dbus_message_ref(msg);
+
+		return NULL;
+	}
+
 	if (parse_value_arg(&iter, &chrc->value, &chrc->value_len))
 		return g_dbus_create_error(msg,
 					"org.bluez.Error.InvalidArguments",
@@ -1763,6 +1890,7 @@ void gatt_register_chrc(DBusConnection *conn, GDBusProxy *proxy,
 	chrc->uuid = g_strdup(argv[1]);
 	chrc->path = g_strdup_printf("%s/chrc%p", service->path, chrc);
 	chrc->flags = g_strsplit(argv[2], ",", -1);
+	chrc->authorization_req = argc > 3 ? true : false;
 
 	if (g_dbus_register_interface(conn, chrc->path, CHRC_INTERFACE,
 					chrc_methods, NULL, chrc_properties,
diff --git a/client/main.c b/client/main.c
index a83010b48..f962e9a43 100644
--- a/client/main.c
+++ b/client/main.c
@@ -2011,6 +2011,11 @@ static void cmd_register_characteristic(int argc, char *argv[])
 	if (check_default_ctrl() == FALSE)
 		return bt_shell_noninteractive_quit(EXIT_FAILURE);
 
+	if (argc > 3 && strcmp(argv[3], "authz-req")) {
+		bt_shell_printf("Wrong authorization argument\n");
+		return bt_shell_noninteractive_quit(EXIT_FAILURE);
+	}
+
 	gatt_register_chrc(dbus_conn, default_ctrl->proxy, argc, argv);
 }
 
@@ -2428,9 +2433,9 @@ static const struct bt_shell_menu gatt_menu = {
 					"Register application service."  },
 	{ "unregister-service", "<UUID/object>", cmd_unregister_service,
 					"Unregister application service" },
-	{ "register-characteristic", "<UUID> <Flags=read,write,notify...>",
-					cmd_register_characteristic,
-					"Register application characteristic" },
+	{ "register-characteristic", "<UUID> <Flags=read,write,notify...> "
+				"[authz-req]", cmd_register_characteristic,
+				"Register application characteristic" },
 	{ "unregister-characteristic", "<UUID/object>",
 				cmd_unregister_characteristic,
 				"Unregister application characteristic" },
-- 
2.13.6

Re: [PATCH BlueZ v4 1/4] client: Fix reading long values

[Szymon Janc]

Hi Grzegorz,

On Tuesday, 20 March 2018 15:05:16 CET Grzegorz Kolodziejczyk wrote:
> While value has more than single MTU can carry long read procedure will
> be triggered. In such cases offset need to bo considered while getting
> value from storage.
> ---
>  client/gatt.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/client/gatt.c b/client/gatt.c
> index 8c818d8c1..7a6035ac1 100644
> --- a/client/gatt.c
> +++ b/client/gatt.c
> @@ -1412,6 +1412,39 @@ static const GDBusPropertyTable chrc_properties[] = {
> { }
>  };
> 
> +static int parse_offset(DBusMessageIter *iter, uint16_t *offset)
> +{
> +	DBusMessageIter dict;
> +
> +	if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_ARRAY)
> +		return -EINVAL;
> +
> +	dbus_message_iter_recurse(iter, &dict);
> +
> +	while (dbus_message_iter_get_arg_type(&dict) == DBUS_TYPE_DICT_ENTRY) {
> +		const char *key;
> +		DBusMessageIter value, entry;
> +		int var;
> +
> +		dbus_message_iter_recurse(&dict, &entry);
> +		dbus_message_iter_get_basic(&entry, &key);
> +
> +		dbus_message_iter_next(&entry);
> +		dbus_message_iter_recurse(&entry, &value);
> +
> +		var = dbus_message_iter_get_arg_type(&value);
> +		if (strcasecmp(key, "offset") == 0) {
> +			if (var != DBUS_TYPE_UINT16)
> +				return -EINVAL;
> +			dbus_message_iter_get_basic(&value, offset);
> +		}
> +
> +		dbus_message_iter_next(&dict);
> +	}
> +
> +	return 0;
> +}
> +
>  static DBusMessage *read_value(DBusMessage *msg, uint8_t *value,
>  						uint16_t value_len)
>  {
> @@ -1433,8 +1466,14 @@ static DBusMessage *chrc_read_value(DBusConnection
> *conn, DBusMessage *msg, void *user_data)
>  {
>  	struct chrc *chrc = user_data;
> +	DBusMessageIter iter;
> +	uint16_t offset = 0;
> +
> +	dbus_message_iter_init(msg, &iter);
> +
> +	parse_offset(&iter, &offset);
> 
> -	return read_value(msg, chrc->value, chrc->value_len);
> +	return read_value(msg, &chrc->value[offset], chrc->value_len - offset);
>  }
> 
>  static int parse_value_arg(DBusMessageIter *iter, uint8_t **value, int
> *len) @@ -1785,8 +1824,14 @@ static DBusMessage
> *desc_read_value(DBusConnection *conn, DBusMessage *msg, void *user_data)
>  {
>  	struct desc *desc = user_data;
> +	DBusMessageIter iter;
> +	uint16_t offset = 0;
> +
> +	dbus_message_iter_init(msg, &iter);
> +
> +	parse_offset(&iter, &offset);
> 
> -	return read_value(msg, desc->value, desc->value_len);
> +	return read_value(msg, &desc->value[offset], desc->value_len - offset);
>  }
> 
>  static DBusMessage *desc_write_value(DBusConnection *conn, DBusMessage
> *msg,

All patches applied, thanks.
As discussed, I changed authz parameter to authorize.


-- 
pozdrawiam
Szymon Janc