[PATCH v4 0/3] support signed write command

[PATCH v4 0/3] support signed write command

[Gu Chaojie]

This patch set mainly remove valid_csrk parameter in bt_gatt_client_set_local_csrk.
patch v3 has parameter has valid_local_csrk which would make user highly confusing.

Gu Chaojie (3):
  shared/att.c: Add signed command outgoing and CSRK function
  shared/gatt-client: Add CSRK part to support signed write
  Add signed write CSRK option

 src/shared/att-types.h   |    3 ++
 src/shared/att.c         |  122 ++++++++++++++++++++++++++++++++++++++++++++--
 src/shared/att.h         |   16 ++++++
 src/shared/gatt-client.c |   26 ++++++++--
 src/shared/gatt-client.h |    3 ++
 tools/btgatt-client.c    |   33 ++++++++++++-
 6 files changed, 193 insertions(+), 10 deletions(-)

-- 
1.7.10.4

[PATCH v4 1/3] shared/att.c: Add signed command outgoing and CSRK function

[Gu Chaojie]

---
 src/shared/att-types.h |    3 ++
 src/shared/att.c       |  122 ++++++++++++++++++++++++++++++++++++++++++++++--
 src/shared/att.h       |   16 +++++++
 3 files changed, 138 insertions(+), 3 deletions(-)

diff --git a/src/shared/att-types.h b/src/shared/att-types.h
index b85c969..da8b6ae 100644
--- a/src/shared/att-types.h
+++ b/src/shared/att-types.h
@@ -25,6 +25,9 @@
 
 #define BT_ATT_DEFAULT_LE_MTU 23
 
+/* Length of signature in write signed packet */
+#define BT_ATT_SIGNATURE_LEN		12
+
 /* ATT protocol opcodes */
 #define BT_ATT_OP_ERROR_RSP	      		0x01
 #define BT_ATT_OP_MTU_REQ			0x02
diff --git a/src/shared/att.c b/src/shared/att.c
index de35aef..b4921d9 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -36,6 +36,7 @@
 #include "lib/uuid.h"
 #include "src/shared/att.h"
 #include "src/shared/att-types.h"
+#include "src/shared/crypto.h"
 
 #define ATT_MIN_PDU_LEN			1  /* At least 1 byte for the opcode. */
 #define ATT_OP_CMD_MASK			0x40
@@ -77,6 +78,16 @@ struct bt_att {
 	bt_att_debug_func_t debug_callback;
 	bt_att_destroy_func_t debug_destroy;
 	void *debug_data;
+
+	struct bt_crypto *crypto;
+
+	bool valid_local_csrk;
+	uint8_t local_csrk[16];
+	uint32_t local_sign_cnt;
+
+	bool valid_remote_csrk;
+	uint8_t remote_csrk[16];
+	uint32_t remote_sign_cnt;
 };
 
 enum att_op_type {
@@ -176,6 +187,8 @@ struct att_send_op {
 	bt_att_response_func_t callback;
 	bt_att_destroy_func_t destroy;
 	void *user_data;
+
+	struct bt_att *att;
 };
 
 static void destroy_att_send_op(void *data)
@@ -277,6 +290,10 @@ static bool encode_pdu(struct att_send_op *op, const void *pdu,
 						uint16_t length, uint16_t mtu)
 {
 	uint16_t pdu_len = 1;
+	struct bt_att *att = op->att;
+
+	if (op->opcode & ATT_OP_SIGNED_MASK)
+		pdu_len += BT_ATT_SIGNATURE_LEN;
 
 	if (length && pdu)
 		pdu_len += length;
@@ -293,17 +310,32 @@ static bool encode_pdu(struct att_send_op *op, const void *pdu,
 	if (pdu_len > 1)
 		memcpy(op->pdu + 1, pdu, length);
 
+	if (op->opcode & ATT_OP_SIGNED_MASK) {
+		if (bt_crypto_sign_att(att->crypto,
+					att->local_csrk,
+					op->pdu,
+					1 + length,
+					att->local_sign_cnt,
+					&((uint8_t *) op->pdu)[1 + length]))
+			att->local_sign_cnt++;
+		else
+			return false;
+	}
+
 	return true;
 }
 
-static struct att_send_op *create_att_send_op(uint8_t opcode, const void *pdu,
-						uint16_t length, uint16_t mtu,
+static struct att_send_op *create_att_send_op(uint8_t opcode,
+						const void *pdu,
+						uint16_t length,
+						struct bt_att *att,
 						bt_att_response_func_t callback,
 						void *user_data,
 						bt_att_destroy_func_t destroy)
 {
 	struct att_send_op *op;
 	enum att_op_type op_type;
+	uint16_t mtu = att->mtu;
 
 	if (length && !pdu)
 		return NULL;
@@ -334,6 +366,7 @@ static struct att_send_op *create_att_send_op(uint8_t opcode, const void *pdu,
 	op->callback = callback;
 	op->destroy = destroy;
 	op->user_data = user_data;
+	op->att = att;
 
 	if (!encode_pdu(op, pdu, length, mtu)) {
 		free(op);
@@ -701,6 +734,10 @@ struct bt_att *bt_att_new(int fd)
 
 	att->fd = fd;
 
+	att->local_sign_cnt = 0;
+	att->remote_sign_cnt = 0;
+	att->valid_local_csrk = false;
+	att->valid_remote_csrk = false;
 	att->mtu = BT_ATT_DEFAULT_LE_MTU;
 	att->buf = malloc(att->mtu);
 	if (!att->buf)
@@ -710,6 +747,10 @@ struct bt_att *bt_att_new(int fd)
 	if (!att->io)
 		goto fail;
 
+	att->crypto = bt_crypto_new();
+	if (!att->crypto)
+		goto fail;
+
 	att->req_queue = queue_new();
 	if (!att->req_queue)
 		goto fail;
@@ -744,6 +785,7 @@ fail:
 	queue_destroy(att->write_queue, NULL);
 	queue_destroy(att->notify_list, NULL);
 	queue_destroy(att->disconn_list, NULL);
+	bt_crypto_unref(att->crypto);
 	io_destroy(att->io);
 	free(att->buf);
 	free(att);
@@ -934,7 +976,7 @@ unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
 	if (!att || !att->io)
 		return 0;
 
-	op = create_att_send_op(opcode, pdu, length, att->mtu, callback,
+	op = create_att_send_op(opcode, pdu, length, att, callback,
 							user_data, destroy);
 	if (!op)
 		return 0;
@@ -1119,3 +1161,77 @@ bool bt_att_unregister_all(struct bt_att *att)
 
 	return true;
 }
+
+bool bt_att_set_local_csrk(struct bt_att *att, bool valid_local_csrk,
+						uint8_t key[16])
+{
+	if (!att)
+		return false;
+
+	att->valid_local_csrk = valid_local_csrk;
+	memcpy(att->local_csrk, key, 16);
+
+	return true;
+}
+
+bool bt_att_set_remote_csrk(struct bt_att *att, bool valid_remote_csrk,
+						uint8_t key[16])
+{
+	if (!att)
+		return false;
+
+	att->valid_remote_csrk = valid_remote_csrk;
+	memcpy(att->remote_csrk, key, 16);
+
+	return true;
+
+}
+
+bool bt_att_get_local_csrk(struct bt_att *att, uint8_t key[16],
+					uint32_t *local_sign_cnt)
+{
+	if (!att)
+		return false;
+
+	if (att->valid_local_csrk) {
+		memcpy(key, att->local_csrk, 16);
+		*local_sign_cnt = att->local_sign_cnt;
+	} else {
+		return false;
+	}
+
+	return true;
+}
+
+bool bt_att_get_remote_csrk(struct bt_att *att, uint8_t key[16],
+					uint32_t *remote_sign_cnt)
+{
+	if (!att)
+		return false;
+
+	if (att->valid_remote_csrk) {
+		memcpy(key, att->remote_csrk, 16);
+		*remote_sign_cnt = att->remote_sign_cnt;
+	} else {
+		return false;
+	}
+
+	return true;
+}
+
+bool bt_att_local_csrk_is_valid(struct bt_att *att)
+{
+	if (!att)
+		return false;
+
+	return att->valid_local_csrk;
+}
+
+bool bt_att_remote_csrk_is_valid(struct bt_att *att)
+{
+	if (!att)
+		return false;
+
+	return att->valid_remote_csrk;
+
+}
diff --git a/src/shared/att.h b/src/shared/att.h
index 1063021..306d44c 100644
--- a/src/shared/att.h
+++ b/src/shared/att.h
@@ -76,3 +76,19 @@ unsigned int bt_att_register_disconnect(struct bt_att *att,
 bool bt_att_unregister_disconnect(struct bt_att *att, unsigned int id);
 
 bool bt_att_unregister_all(struct bt_att *att);
+
+bool bt_att_set_local_csrk(struct bt_att *att, bool valid_local_csrk,
+						uint8_t key[16]);
+
+bool bt_att_set_remote_csrk(struct bt_att *att, bool valid_remote_csrk,
+						uint8_t key[16]);
+
+bool bt_att_get_local_csrk(struct bt_att *att, uint8_t key[16],
+					uint32_t *local_sign_cnt);
+
+bool bt_att_get_remote_csrk(struct bt_att *att, uint8_t key[16],
+					uint32_t *remote_sign_cnt);
+
+bool bt_att_local_csrk_is_valid(struct bt_att *att);
+
+bool bt_att_remote_csrk_is_valid(struct bt_att *att);
-- 
1.7.10.4

[PATCH v4 2/3] shared/gatt-client: Add CSRK part to support signed write

[Gu Chaojie]

---
 src/shared/gatt-client.c |   26 +++++++++++++++++++++-----
 src/shared/gatt-client.h |    3 +++
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 03d722f..e476f92 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -1765,19 +1765,26 @@ bool bt_gatt_client_read_long_value(struct bt_gatt_client *client,
 bool bt_gatt_client_write_without_response(struct bt_gatt_client *client,
 					uint16_t value_handle,
 					bool signed_write,
-					uint8_t *value, uint16_t length) {
+					uint8_t *value, uint16_t length)
+{
 	uint8_t pdu[2 + length];
 
 	if (!client)
 		return 0;
 
-	/* TODO: Support this once bt_att_send supports signed writes. */
-	if (signed_write)
-		return 0;
-
 	put_le16(value_handle, pdu);
 	memcpy(pdu + 2, value, length);
 
+	if (signed_write) {
+		if (bt_att_local_csrk_is_valid(client->att))
+			return bt_att_send(client->att,
+					BT_ATT_OP_SIGNED_WRITE_CMD,
+					pdu, sizeof(pdu), NULL, NULL, NULL);
+		else
+			return 0;
+
+	}
+
 	return bt_att_send(client->att, BT_ATT_OP_WRITE_CMD, pdu, sizeof(pdu),
 							NULL, NULL, NULL);
 }
@@ -2258,3 +2265,12 @@ bool bt_gatt_client_unregister_notify(struct bt_gatt_client *client,
 	client->need_notify_cleanup = true;
 	return true;
 }
+
+bool bt_gatt_client_set_local_csrk(struct bt_gatt_client *client,
+							uint8_t key[16])
+{
+	if (!client)
+		return false;
+
+	return bt_att_set_local_csrk(client->att, true, key);
+}
diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h
index 6807f6b..ca0a7e8 100644
--- a/src/shared/gatt-client.h
+++ b/src/shared/gatt-client.h
@@ -162,3 +162,6 @@ bool bt_gatt_client_register_notify(struct bt_gatt_client *client,
 				bt_gatt_client_destroy_func_t destroy);
 bool bt_gatt_client_unregister_notify(struct bt_gatt_client *client,
 							unsigned int id);
+
+bool bt_gatt_client_set_local_csrk(struct bt_gatt_client *client,
+							uint8_t key[16]);
-- 
1.7.10.4

[PATCH v4 3/3] Add signed write CSRK option

[Gu Chaojie]

---
 tools/btgatt-client.c |   33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
index d900e08..9b9d73b 100644
--- a/tools/btgatt-client.c
+++ b/tools/btgatt-client.c
@@ -486,11 +486,13 @@ static void write_value_usage(void)
 	printf("Usage: write-value [options] <value_handle> <value>\n"
 		"Options:\n"
 		"\t-w, --without-response\tWrite without response\n"
+		"\t-c, --csrk <key>\tCSRK for signed write command\n"
 		"e.g.:\n"
 		"\twrite-value 0x0001 00 01 00\n");
 }
 
 static struct option write_value_options[] = {
+	{ "csrk",		1, 0, 'c' },
 	{ "without-response",	0, 0, 'w' },
 	{ }
 };
@@ -504,6 +506,24 @@ static void write_cb(bool success, uint8_t att_ecode, void *user_data)
 	}
 }
 
+static bool convert_csrk_key(char *optarg, uint8_t csrk[16])
+{
+	int i;
+	char value[2];
+
+	if (strlen(optarg) != 32) {
+		printf("csrk length is invalid\n");
+		return false;
+	} else {
+		for (i = 0; i < 16; i++) {
+			strncpy(value, optarg + (i * 2), 2);
+			csrk[i] = strtol(value, NULL, 16);
+		}
+	}
+
+	return true;
+}
+
 static void cmd_write_value(struct client *cli, char *cmd_str)
 {
 	int opt, i;
@@ -515,6 +535,10 @@ static void cmd_write_value(struct client *cli, char *cmd_str)
 	int length;
 	uint8_t *value = NULL;
 	bool without_response = false;
+	bool signed_write = false;
+	uint8_t csrk[16];
+
+	memset(csrk, 0, 16);
 
 	if (!bt_gatt_client_is_ready(cli->gatt)) {
 		printf("GATT client not initialized\n");
@@ -529,12 +553,17 @@ static void cmd_write_value(struct client *cli, char *cmd_str)
 
 	optind = 0;
 	argv[0] = "write-value";
-	while ((opt = getopt_long(argc, argv, "+w", write_value_options,
+	while ((opt = getopt_long(argc, argv, "+wc:", write_value_options,
 								NULL)) != -1) {
 		switch (opt) {
 		case 'w':
 			without_response = true;
 			break;
+		case 'c':
+			signed_write = true;
+			if (convert_csrk_key(optarg, csrk))
+				bt_gatt_client_set_local_csrk(cli->gatt, csrk);
+			break;
 		default:
 			write_value_usage();
 			return;
@@ -588,7 +617,7 @@ static void cmd_write_value(struct client *cli, char *cmd_str)
 
 	if (without_response) {
 		if (!bt_gatt_client_write_without_response(cli->gatt, handle,
-							false, value, length)) {
+						signed_write, value, length)) {
 			printf("Failed to initiate write without response "
 								"procedure\n");
 			goto done;
-- 
1.7.10.4

Re: [PATCH v4 1/3] shared/att.c: Add signed command outgoing and CSRK function

[Luiz Augusto von Dentz]

Hi,

On Wed, Oct 8, 2014 at 11:54 AM, Gu Chaojie <chao.jie.gu@intel.com> wrote:
> ---
>  src/shared/att-types.h |    3 ++
>  src/shared/att.c       |  122 ++++++++++++++++++++++++++++++++++++++++++++++--
>  src/shared/att.h       |   16 +++++++
>  3 files changed, 138 insertions(+), 3 deletions(-)
>
> diff --git a/src/shared/att-types.h b/src/shared/att-types.h
> index b85c969..da8b6ae 100644
> --- a/src/shared/att-types.h
> +++ b/src/shared/att-types.h
> @@ -25,6 +25,9 @@
>
>  #define BT_ATT_DEFAULT_LE_MTU 23
>
> +/* Length of signature in write signed packet */
> +#define BT_ATT_SIGNATURE_LEN           12
> +
>  /* ATT protocol opcodes */
>  #define BT_ATT_OP_ERROR_RSP                    0x01
>  #define BT_ATT_OP_MTU_REQ                      0x02
> diff --git a/src/shared/att.c b/src/shared/att.c
> index de35aef..b4921d9 100644
> --- a/src/shared/att.c
> +++ b/src/shared/att.c
> @@ -36,6 +36,7 @@
>  #include "lib/uuid.h"
>  #include "src/shared/att.h"
>  #include "src/shared/att-types.h"
> +#include "src/shared/crypto.h"
>
>  #define ATT_MIN_PDU_LEN                        1  /* At least 1 byte for the opcode. */
>  #define ATT_OP_CMD_MASK                        0x40
> @@ -77,6 +78,16 @@ struct bt_att {
>         bt_att_debug_func_t debug_callback;
>         bt_att_destroy_func_t debug_destroy;
>         void *debug_data;
> +
> +       struct bt_crypto *crypto;
> +
> +       bool valid_local_csrk;
> +       uint8_t local_csrk[16];
> +       uint32_t local_sign_cnt;
> +
> +       bool valid_remote_csrk;
> +       uint8_t remote_csrk[16];
> +       uint32_t remote_sign_cnt;

Maybe it is better to have pointers to a structs so you can free the
data once it becomes invalid and it also removes the necessity of
extra flags just to tell if the data is still valid since you can
check if the pointer is not NULL then it must be valid.

>  };
>
>  enum att_op_type {
> @@ -176,6 +187,8 @@ struct att_send_op {
>         bt_att_response_func_t callback;
>         bt_att_destroy_func_t destroy;
>         void *user_data;
> +
> +       struct bt_att *att;
>  };
>
>  static void destroy_att_send_op(void *data)
> @@ -277,6 +290,10 @@ static bool encode_pdu(struct att_send_op *op, const void *pdu,
>                                                 uint16_t length, uint16_t mtu)
>  {
>         uint16_t pdu_len = 1;
> +       struct bt_att *att = op->att;
> +
> +       if (op->opcode & ATT_OP_SIGNED_MASK)
> +               pdu_len += BT_ATT_SIGNATURE_LEN;
>
>         if (length && pdu)
>                 pdu_len += length;
> @@ -293,17 +310,32 @@ static bool encode_pdu(struct att_send_op *op, const void *pdu,
>         if (pdu_len > 1)
>                 memcpy(op->pdu + 1, pdu, length);
>
> +       if (op->opcode & ATT_OP_SIGNED_MASK) {
> +               if (bt_crypto_sign_att(att->crypto,
> +                                       att->local_csrk,
> +                                       op->pdu,
> +                                       1 + length,
> +                                       att->local_sign_cnt,
> +                                       &((uint8_t *) op->pdu)[1 + length]))
> +                       att->local_sign_cnt++;
> +               else
> +                       return false;
> +       }
> +

You can probably bail out early if you check !(op->opcode &
ATT_OP_SIGNED_MASK), also perhaps it is a good idea to start defining
structs for the PDUs to simplify the access.

>         return true;
>  }
>
> -static struct att_send_op *create_att_send_op(uint8_t opcode, const void *pdu,
> -                                               uint16_t length, uint16_t mtu,
> +static struct att_send_op *create_att_send_op(uint8_t opcode,
> +                                               const void *pdu,
> +                                               uint16_t length,
> +                                               struct bt_att *att,
>                                                 bt_att_response_func_t callback,
>                                                 void *user_data,
>                                                 bt_att_destroy_func_t destroy)
>  {
>         struct att_send_op *op;
>         enum att_op_type op_type;
> +       uint16_t mtu = att->mtu;
>
>         if (length && !pdu)
>                 return NULL;
> @@ -334,6 +366,7 @@ static struct att_send_op *create_att_send_op(uint8_t opcode, const void *pdu,
>         op->callback = callback;
>         op->destroy = destroy;
>         op->user_data = user_data;
> +       op->att = att;
>
>         if (!encode_pdu(op, pdu, length, mtu)) {
>                 free(op);
> @@ -701,6 +734,10 @@ struct bt_att *bt_att_new(int fd)
>
>         att->fd = fd;
>
> +       att->local_sign_cnt = 0;
> +       att->remote_sign_cnt = 0;
> +       att->valid_local_csrk = false;
> +       att->valid_remote_csrk = false;
>         att->mtu = BT_ATT_DEFAULT_LE_MTU;
>         att->buf = malloc(att->mtu);
>         if (!att->buf)
> @@ -710,6 +747,10 @@ struct bt_att *bt_att_new(int fd)
>         if (!att->io)
>                 goto fail;
>
> +       att->crypto = bt_crypto_new();
> +       if (!att->crypto)
> +               goto fail;
> +
>         att->req_queue = queue_new();
>         if (!att->req_queue)
>                 goto fail;
> @@ -744,6 +785,7 @@ fail:
>         queue_destroy(att->write_queue, NULL);
>         queue_destroy(att->notify_list, NULL);
>         queue_destroy(att->disconn_list, NULL);
> +       bt_crypto_unref(att->crypto);
>         io_destroy(att->io);
>         free(att->buf);
>         free(att);
> @@ -934,7 +976,7 @@ unsigned int bt_att_send(struct bt_att *att, uint8_t opcode,
>         if (!att || !att->io)
>                 return 0;
>
> -       op = create_att_send_op(opcode, pdu, length, att->mtu, callback,
> +       op = create_att_send_op(opcode, pdu, length, att, callback,
>                                                         user_data, destroy);
>         if (!op)
>                 return 0;
> @@ -1119,3 +1161,77 @@ bool bt_att_unregister_all(struct bt_att *att)
>
>         return true;
>  }
> +
> +bool bt_att_set_local_csrk(struct bt_att *att, bool valid_local_csrk,
> +                                               uint8_t key[16])
> +{
> +       if (!att)
> +               return false;
> +
> +       att->valid_local_csrk = valid_local_csrk;
> +       memcpy(att->local_csrk, key, 16);
> +
> +       return true;
> +}
> +
> +bool bt_att_set_remote_csrk(struct bt_att *att, bool valid_remote_csrk,
> +                                               uint8_t key[16])
> +{
> +       if (!att)
> +               return false;
> +
> +       att->valid_remote_csrk = valid_remote_csrk;
> +       memcpy(att->remote_csrk, key, 16);
> +
> +       return true;
> +
> +}
> +
> +bool bt_att_get_local_csrk(struct bt_att *att, uint8_t key[16],
> +                                       uint32_t *local_sign_cnt)
> +{
> +       if (!att)
> +               return false;
> +
> +       if (att->valid_local_csrk) {
> +               memcpy(key, att->local_csrk, 16);
> +               *local_sign_cnt = att->local_sign_cnt;
> +       } else {
> +               return false;
> +       }
> +
> +       return true;
> +}
> +
> +bool bt_att_get_remote_csrk(struct bt_att *att, uint8_t key[16],
> +                                       uint32_t *remote_sign_cnt)
> +{
> +       if (!att)
> +               return false;
> +
> +       if (att->valid_remote_csrk) {
> +               memcpy(key, att->remote_csrk, 16);
> +               *remote_sign_cnt = att->remote_sign_cnt;
> +       } else {
> +               return false;
> +       }
> +
> +       return true;
> +}
> +
> +bool bt_att_local_csrk_is_valid(struct bt_att *att)
> +{
> +       if (!att)
> +               return false;
> +
> +       return att->valid_local_csrk;
> +}
> +
> +bool bt_att_remote_csrk_is_valid(struct bt_att *att)
> +{
> +       if (!att)
> +               return false;
> +
> +       return att->valid_remote_csrk;
> +
> +}
> diff --git a/src/shared/att.h b/src/shared/att.h
> index 1063021..306d44c 100644
> --- a/src/shared/att.h
> +++ b/src/shared/att.h
> @@ -76,3 +76,19 @@ unsigned int bt_att_register_disconnect(struct bt_att *att,
>  bool bt_att_unregister_disconnect(struct bt_att *att, unsigned int id);
>
>  bool bt_att_unregister_all(struct bt_att *att);
> +
> +bool bt_att_set_local_csrk(struct bt_att *att, bool valid_local_csrk,
> +                                               uint8_t key[16]);
> +
> +bool bt_att_set_remote_csrk(struct bt_att *att, bool valid_remote_csrk,
> +                                               uint8_t key[16]);
> +
> +bool bt_att_get_local_csrk(struct bt_att *att, uint8_t key[16],
> +                                       uint32_t *local_sign_cnt);
> +
> +bool bt_att_get_remote_csrk(struct bt_att *att, uint8_t key[16],
> +                                       uint32_t *remote_sign_cnt);
> +
> +bool bt_att_local_csrk_is_valid(struct bt_att *att);
> +
> +bool bt_att_remote_csrk_is_valid(struct bt_att *att);
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Luiz Augusto von Dentz

RE: [PATCH v4 1/3] shared/att.c: Add signed command outgoing and CSRK function

[Gu, Chao Jie]

SGkgTHVpeiwNCg0KPiA+IEBAIC03Nyw2ICs3OCwxNiBAQCBzdHJ1Y3QgYnRfYXR0IHsNCj4gPiAg
ICAgICAgIGJ0X2F0dF9kZWJ1Z19mdW5jX3QgZGVidWdfY2FsbGJhY2s7DQo+ID4gICAgICAgICBi
dF9hdHRfZGVzdHJveV9mdW5jX3QgZGVidWdfZGVzdHJveTsNCj4gPiAgICAgICAgIHZvaWQgKmRl
YnVnX2RhdGE7DQo+ID4gKw0KPiA+ICsgICAgICAgc3RydWN0IGJ0X2NyeXB0byAqY3J5cHRvOw0K
PiA+ICsNCj4gPiArICAgICAgIGJvb2wgdmFsaWRfbG9jYWxfY3NyazsNCj4gPiArICAgICAgIHVp
bnQ4X3QgbG9jYWxfY3Nya1sxNl07DQo+ID4gKyAgICAgICB1aW50MzJfdCBsb2NhbF9zaWduX2Nu
dDsNCj4gPiArDQo+ID4gKyAgICAgICBib29sIHZhbGlkX3JlbW90ZV9jc3JrOw0KPiA+ICsgICAg
ICAgdWludDhfdCByZW1vdGVfY3Nya1sxNl07DQo+ID4gKyAgICAgICB1aW50MzJfdCByZW1vdGVf
c2lnbl9jbnQ7DQo+IA0KPiBNYXliZSBpdCBpcyBiZXR0ZXIgdG8gaGF2ZSBwb2ludGVycyB0byBh
IHN0cnVjdHMgc28geW91IGNhbiBmcmVlIHRoZSBkYXRhIG9uY2UgaXQNCj4gYmVjb21lcyBpbnZh
bGlkIGFuZCBpdCBhbHNvIHJlbW92ZXMgdGhlIG5lY2Vzc2l0eSBvZiBleHRyYSBmbGFncyBqdXN0
IHRvIHRlbGwgaWYgdGhlIGRhdGENCj4gaXMgc3RpbGwgdmFsaWQgc2luY2UgeW91IGNhbiBjaGVj
ayBpZiB0aGUgcG9pbnRlciBpcyBub3QgTlVMTCB0aGVuIGl0IG11c3QgYmUgdmFsaWQuDQoNClRo
aXMgaXMgZ29vZCBpZGVhIHRvIHJlbW92ZSB0aGUgZXh0cmEgZmxhZ3MuIEhvd2V2ZXIsIHdlIGRl
ZmluZSB0aGUgc3RydWN0IHRvIGRvIHRoaXMgd2hpY2gNCndvdWxkIHJlc3VsdCBpbiBzb21lIHBy
b2JsZW0uIE5vdyB3ZSBpbml0aWFsaXplIHRoZSBsb2NhbF9zaWduX2NudCBpbiB0aGUgYnRfYXR0
X25ldyBmdW5jdGlvbg0KYW5kIG9ubHkgYWRkIGxvY2FsX3NpZ25fY250IHZhbHVlIHdoZW4gc2ln
bmVkIHdyaXRlIGNvbW1hbmQgb3V0Z29pbmcuIE1lYW53aWxlLCB3ZSBzZXQgbG9jYWwNCkNTUksg
aW4gdGhlIGJ0X2dhdHRfY2xpZW50X3NldF9sb2NhbF9jc3JrIGZ1bmN0aW9uIHdoZW4gdGhlcmUg
aXMgdmFsaWQgQ1NSSyBwcm92aWRlZC4gVGhpcyB0d28NClBhcmFtZXRlciBzZXQgYXQgZGlmZmVy
ZW50IHRpbWUgbm93Lg0KDQpJZiB3ZSBoYXZlIHBvaW50ZXJzIHRvIGEgc3RydWN0IGluY2x1ZGlu
ZyBsb2NhbF9jc3JrIGFuZCBsb2NhbF9zaWduZF9jbnQsIHdlIGluaXRpYWxpemUgdGhlIGxvY2Fs
X3NpZ25fY250IA0KbWVhbndoaWxlIHRoZSBzdHJ1Y3Qgc2hvdWxkIG5vdCBiZSBOVUxMIGJlY2F1
c2Ugb2YgaW5jdWRpbmcgaXQuIEluIGZhY3QsIGxvY2FsX2NzcmsgaXMgbm90IHZhbGlkIGF0IHRo
aXMgdGltZS4NCklmIHdlIGluaXRpYWxpemUgdGhlIGxvY2FsX3NpZ25lZF9jbnQgd2hlbiBDU1JL
IGlzIHZhbGlkICwgaXQgd291bGQgYmUgcmlzayB0byBjaGFuZ2UgbG9jYWxfc2lnbmVkX2NudCBi
eSB1c2VyDQpjYWxsaW5nIGJ0X2dhdHRfY2xpZW50X3NldF9sb2NhbF9jc3JrLg0KPiA+ICB9Ow0K
PiA+DQo+ID4gIGVudW0gYXR0X29wX3R5cGUgew0KPiA+IEBAIC0xNzYsNiArMTg3LDggQEAgc3Ry
dWN0IGF0dF9zZW5kX29wIHsNCj4gPiAgICAgICAgIGJ0X2F0dF9yZXNwb25zZV9mdW5jX3QgY2Fs
bGJhY2s7DQo+ID4gICAgICAgICBidF9hdHRfZGVzdHJveV9mdW5jX3QgZGVzdHJveTsNCj4gPiAg
ICAgICAgIHZvaWQgKnVzZXJfZGF0YTsNCj4gPiArDQo+ID4gKyAgICAgICBzdHJ1Y3QgYnRfYXR0
ICphdHQ7DQo+ID4gIH07DQo+ID4NCj4gPiAgc3RhdGljIHZvaWQgZGVzdHJveV9hdHRfc2VuZF9v
cCh2b2lkICpkYXRhKSBAQCAtMjc3LDYgKzI5MCwxMCBAQA0KPiA+IHN0YXRpYyBib29sIGVuY29k
ZV9wZHUoc3RydWN0IGF0dF9zZW5kX29wICpvcCwgY29uc3Qgdm9pZCAqcGR1LA0KPiA+ICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIHVpbnQxNl90IGxlbmd0
aCwNCj4gPiB1aW50MTZfdCBtdHUpICB7DQo+ID4gICAgICAgICB1aW50MTZfdCBwZHVfbGVuID0g
MTsNCj4gPiArICAgICAgIHN0cnVjdCBidF9hdHQgKmF0dCA9IG9wLT5hdHQ7DQo+ID4gKw0KPiA+
ICsgICAgICAgaWYgKG9wLT5vcGNvZGUgJiBBVFRfT1BfU0lHTkVEX01BU0spDQo+ID4gKyAgICAg
ICAgICAgICAgIHBkdV9sZW4gKz0gQlRfQVRUX1NJR05BVFVSRV9MRU47DQo+ID4NCj4gPiAgICAg
ICAgIGlmIChsZW5ndGggJiYgcGR1KQ0KPiA+ICAgICAgICAgICAgICAgICBwZHVfbGVuICs9IGxl
bmd0aDsNCj4gPiBAQCAtMjkzLDE3ICszMTAsMzIgQEAgc3RhdGljIGJvb2wgZW5jb2RlX3BkdShz
dHJ1Y3QgYXR0X3NlbmRfb3AgKm9wLCBjb25zdA0KPiB2b2lkICpwZHUsDQo+ID4gICAgICAgICBp
ZiAocGR1X2xlbiA+IDEpDQo+ID4gICAgICAgICAgICAgICAgIG1lbWNweShvcC0+cGR1ICsgMSwg
cGR1LCBsZW5ndGgpOw0KPiA+DQo+ID4gKyAgICAgICBpZiAob3AtPm9wY29kZSAmIEFUVF9PUF9T
SUdORURfTUFTSykgew0KPiA+ICsgICAgICAgICAgICAgICBpZiAoYnRfY3J5cHRvX3NpZ25fYXR0
KGF0dC0+Y3J5cHRvLA0KPiA+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICBhdHQtPmxvY2FsX2NzcmssDQo+ID4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgIG9wLT5wZHUsDQo+ID4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgIDEgKyBsZW5ndGgsDQo+ID4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgIGF0dC0+bG9jYWxfc2lnbl9jbnQsDQo+ID4gKyAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICYoKHVpbnQ4X3QgKikgb3AtPnBkdSlbMSArDQo+IGxlbmd0aF0pKQ0KPiA+
ICsgICAgICAgICAgICAgICAgICAgICAgIGF0dC0+bG9jYWxfc2lnbl9jbnQrKzsNCj4gPiArICAg
ICAgICAgICAgICAgZWxzZQ0KPiA+ICsgICAgICAgICAgICAgICAgICAgICAgIHJldHVybiBmYWxz
ZTsNCj4gPiArICAgICAgIH0NCj4gPiArDQo+IA0KPiBZb3UgY2FuIHByb2JhYmx5IGJhaWwgb3V0
IGVhcmx5IGlmIHlvdSBjaGVjayAhKG9wLT5vcGNvZGUgJiBBVFRfT1BfU0lHTkVEX01BU0spLA0K
PiBhbHNvIHBlcmhhcHMgaXQgaXMgYSBnb29kIGlkZWEgdG8gc3RhcnQgZGVmaW5pbmcgc3RydWN0
cyBmb3IgdGhlIFBEVXMgdG8gc2ltcGxpZnkgdGhlDQo+IGFjY2Vzcy4NCj4gDQpJZiB3ZSBiYWls
IG91dCB0aGlzIGNoZWNraW5nIGVhcmx5ICwgbWF5YmUgd2UgaGF2ZSB0byBuZWVkIHR3byBkaWZm
ZXJlbnQgZW5jb2RlX3BkdQ0KZnVuY3Rpb24gdG8gaW1wbGVtZW50IGl0IHN1Y2ggYXMgbmFtaW5n
IG5ldyBlbmNvZGVfc2lnbmVkX3BkdSBmdW5jdGlvbiB0byBkbyB0aGlzLg0KDQpIb3dldmVyLCBJ
IHRoaW5rIGVuY29kaW5nIHNpZ25lZF93cml0ZV9jb21tYW5kIHBkdSBtb3N0IGRpZmZlcmVuY2Ug
aXMgYWRkaW5nIA0Kc2lnbmF0Z3VyZSBpbiB0aGUgZW5kIG9mIHBkdS4gQXMgb2xkIHN0YWNrLCB3
ZSBjYW4gc2VlIGV2ZXJ5IGNvbW1hbmQgaGFzIHRoZWlyIA0Kb3duIGVuY29kZV9wZHUgZnVuY3Rp
b24uIEluIGZhY3QsIG1vc3QgcHJvY2VkdXJlIGlzIGNvbW1vbi4gU28gSSB0aGluayBhbGwgaW4g
b25lIA0KZW5jb2RlX3BkdSBmdW5jdGlvbiBpcyBuZXcgZmVhdHVyZSBpbiBuZXcgQVRUIFN0YWNr
LCBzbyB0aGlzIGltcGxlbWVudGF0aW9uIGlzIA0KYXQgbG93ZXN0IHByaWNlIHRvIGNvbXBhdGli
bGUgd2l0aCBuZXcgQVRUIFN0YWNrLiBUaGF0IGlzIHdoeSBJIGRpZG7igJl0IGJhaWwgb3V0IGVh
cmx5DQp0aGlzIGNoZWNraW5nIChvcC0+b3Bjb2RlICYgQVRUX09QX1NJR05FRF9NQVNLKS4gRm9s
bG93aW5nIHRoaXMgaW1wbGVtZW50YXRpb24sDQp0aGVyZSBpcyBubyBuZWVkIHRvIGNyZWF0ZSBh
bm90aGVyIHRoZSBlbmNvZGUgZnVuY3Rpb24gdG8gaW1wbGVtZW50IHNpZ25lZF93cml0ZSwNCkp1
c3QgdXNpbmcgb25lIGVuY29kZV9wZHUgZnVuY3Rpb24gdG8gZG8gdGhpcyBpcyBPSy4NCg0KRG8g
eW91IHRoaW5rIHRoaXMgbWFrZSBzZW5zZT8NCg0KVGhhbmtzDQpDaGFvamllIEd1DQoNCg==

Re: [PATCH v4 1/3] shared/att.c: Add signed command outgoing and CSRK function

[Luiz Augusto von Dentz]

Hi,

On Thu, Oct 9, 2014 at 7:02 AM, Gu, Chao Jie <chao.jie.gu@intel.com> wrote:
> Hi Luiz,
>
>> > @@ -77,6 +78,16 @@ struct bt_att {
>> >         bt_att_debug_func_t debug_callback;
>> >         bt_att_destroy_func_t debug_destroy;
>> >         void *debug_data;
>> > +
>> > +       struct bt_crypto *crypto;
>> > +
>> > +       bool valid_local_csrk;
>> > +       uint8_t local_csrk[16];
>> > +       uint32_t local_sign_cnt;
>> > +
>> > +       bool valid_remote_csrk;
>> > +       uint8_t remote_csrk[16];
>> > +       uint32_t remote_sign_cnt;
>>
>> Maybe it is better to have pointers to a structs so you can free the data once it
>> becomes invalid and it also removes the necessity of extra flags just to tell if the data
>> is still valid since you can check if the pointer is not NULL then it must be valid.
>
> This is good idea to remove the extra flags. However, we define the struct to do this which
> would result in some problem. Now we initialize the local_sign_cnt in the bt_att_new function
> and only add local_sign_cnt value when signed write command outgoing. Meanwile, we set local
> CSRK in the bt_gatt_client_set_local_csrk function when there is valid CSRK provided. This two
> Parameter set at different time now.

Well I guess this was your design, it doesn't have to be this way, in
fact I believe there is no point of initializing the counter if there
is no valid CSRK because one depend on the other, also perhaps you
have a bug in bt_gatt_client_set_local_csrk since I believe every time
you set CSRK you should reset the counter otherwise you may be
carrying the counters from the past CSRK which probably won't work.

Also don't to have to pass the counter when setting the key, the
counter probably need to be restored on every connection so
bt_gatt_client_set_local_csrk and bt_att_set_remote_csrk probably
needs to take the counter and instead of having a valid flag you can
probably reset by setting NULL as key or an invalid counter if one
exists.

> If we have pointers to a struct including local_csrk and local_signd_cnt, we initialize the local_sign_cnt
> meanwhile the struct should not be NULL because of incuding it. In fact, local_csrk is not valid at this time.
> If we initialize the local_signed_cnt when CSRK is valid , it would be risk to change local_signed_cnt by user
> calling bt_gatt_client_set_local_csrk.
>> >  };
>> >
>> >  enum att_op_type {
>> > @@ -176,6 +187,8 @@ struct att_send_op {
>> >         bt_att_response_func_t callback;
>> >         bt_att_destroy_func_t destroy;
>> >         void *user_data;
>> > +
>> > +       struct bt_att *att;
>> >  };
>> >
>> >  static void destroy_att_send_op(void *data) @@ -277,6 +290,10 @@
>> > static bool encode_pdu(struct att_send_op *op, const void *pdu,
>> >                                                 uint16_t length,
>> > uint16_t mtu)  {
>> >         uint16_t pdu_len = 1;
>> > +       struct bt_att *att = op->att;
>> > +
>> > +       if (op->opcode & ATT_OP_SIGNED_MASK)
>> > +               pdu_len += BT_ATT_SIGNATURE_LEN;
>> >
>> >         if (length && pdu)
>> >                 pdu_len += length;
>> > @@ -293,17 +310,32 @@ static bool encode_pdu(struct att_send_op *op, const
>> void *pdu,
>> >         if (pdu_len > 1)
>> >                 memcpy(op->pdu + 1, pdu, length);
>> >
>> > +       if (op->opcode & ATT_OP_SIGNED_MASK) {
>> > +               if (bt_crypto_sign_att(att->crypto,
>> > +                                       att->local_csrk,
>> > +                                       op->pdu,
>> > +                                       1 + length,
>> > +                                       att->local_sign_cnt,
>> > +                                       &((uint8_t *) op->pdu)[1 +
>> length]))
>> > +                       att->local_sign_cnt++;
>> > +               else
>> > +                       return false;
>> > +       }
>> > +
>>
>> You can probably bail out early if you check !(op->opcode & ATT_OP_SIGNED_MASK),
>> also perhaps it is a good idea to start defining structs for the PDUs to simplify the
>> access.
>>
> If we bail out this checking early , maybe we have to need two different encode_pdu
> function to implement it such as naming new encode_signed_pdu function to do this.

But you return true anyway after this code, no idea why you need a new
function for that, all I was suggesting is the following:

if (!(op->opcode & ATT_OP_SIGNED_MASK))
    return true;

if (!bt_crypto_sign_att...)
    return false;

att->local_sign_cnt++;

return true;

> However, I think encoding signed_write_command pdu most difference is adding
> signatgure in the end of pdu. As old stack, we can see every command has their
> own encode_pdu function. In fact, most procedure is common. So I think all in one
> encode_pdu function is new feature in new ATT Stack, so this implementation is
> at lowest price to compatible with new ATT Stack. That is why I didn’t bail out early
> this checking (op->opcode & ATT_OP_SIGNED_MASK). Following this implementation,
> there is no need to create another the encode function to implement signed_write,
> Just using one encode_pdu function to do this is OK.
>
> Do you think this make sense?



-- 
Luiz Augusto von Dentz

RE: [PATCH v4 1/3] shared/att.c: Add signed command outgoing and CSRK function

[Gu, Chao Jie]

SGkgTHVpeiwNCj4gPg0KPiA+PiA+IEBAIC03Nyw2ICs3OCwxNiBAQCBzdHJ1Y3QgYnRfYXR0IHsN
Cj4gPj4gPiAgICAgICAgIGJ0X2F0dF9kZWJ1Z19mdW5jX3QgZGVidWdfY2FsbGJhY2s7DQo+ID4+
ID4gICAgICAgICBidF9hdHRfZGVzdHJveV9mdW5jX3QgZGVidWdfZGVzdHJveTsNCj4gPj4gPiAg
ICAgICAgIHZvaWQgKmRlYnVnX2RhdGE7DQo+ID4+ID4gKw0KPiA+PiA+ICsgICAgICAgc3RydWN0
IGJ0X2NyeXB0byAqY3J5cHRvOw0KPiA+PiA+ICsNCj4gPj4gPiArICAgICAgIGJvb2wgdmFsaWRf
bG9jYWxfY3NyazsNCj4gPj4gPiArICAgICAgIHVpbnQ4X3QgbG9jYWxfY3Nya1sxNl07DQo+ID4+
ID4gKyAgICAgICB1aW50MzJfdCBsb2NhbF9zaWduX2NudDsNCj4gPj4gPiArDQo+ID4+ID4gKyAg
ICAgICBib29sIHZhbGlkX3JlbW90ZV9jc3JrOw0KPiA+PiA+ICsgICAgICAgdWludDhfdCByZW1v
dGVfY3Nya1sxNl07DQo+ID4+ID4gKyAgICAgICB1aW50MzJfdCByZW1vdGVfc2lnbl9jbnQ7DQo+
ID4+DQo+ID4+IE1heWJlIGl0IGlzIGJldHRlciB0byBoYXZlIHBvaW50ZXJzIHRvIGEgc3RydWN0
cyBzbyB5b3UgY2FuIGZyZWUgdGhlDQo+ID4+IGRhdGEgb25jZSBpdCBiZWNvbWVzIGludmFsaWQg
YW5kIGl0IGFsc28gcmVtb3ZlcyB0aGUgbmVjZXNzaXR5IG9mDQo+ID4+IGV4dHJhIGZsYWdzIGp1
c3QgdG8gdGVsbCBpZiB0aGUgZGF0YSBpcyBzdGlsbCB2YWxpZCBzaW5jZSB5b3UgY2FuIGNoZWNr
IGlmIHRoZSBwb2ludGVyIGlzDQo+IG5vdCBOVUxMIHRoZW4gaXQgbXVzdCBiZSB2YWxpZC4NCj4g
Pg0KPiA+IFRoaXMgaXMgZ29vZCBpZGVhIHRvIHJlbW92ZSB0aGUgZXh0cmEgZmxhZ3MuIEhvd2V2
ZXIsIHdlIGRlZmluZSB0aGUNCj4gPiBzdHJ1Y3QgdG8gZG8gdGhpcyB3aGljaCB3b3VsZCByZXN1
bHQgaW4gc29tZSBwcm9ibGVtLiBOb3cgd2UNCj4gPiBpbml0aWFsaXplIHRoZSBsb2NhbF9zaWdu
X2NudCBpbiB0aGUgYnRfYXR0X25ldyBmdW5jdGlvbiBhbmQgb25seSBhZGQNCj4gPiBsb2NhbF9z
aWduX2NudCB2YWx1ZSB3aGVuIHNpZ25lZCB3cml0ZSBjb21tYW5kIG91dGdvaW5nLiBNZWFud2ls
ZSwgd2UNCj4gPiBzZXQgbG9jYWwgQ1NSSyBpbiB0aGUgYnRfZ2F0dF9jbGllbnRfc2V0X2xvY2Fs
X2NzcmsgZnVuY3Rpb24gd2hlbiB0aGVyZSBpcyB2YWxpZA0KPiBDU1JLIHByb3ZpZGVkLiBUaGlz
IHR3byBQYXJhbWV0ZXIgc2V0IGF0IGRpZmZlcmVudCB0aW1lIG5vdy4NCj4gDQo+IFdlbGwgSSBn
dWVzcyB0aGlzIHdhcyB5b3VyIGRlc2lnbiwgaXQgZG9lc24ndCBoYXZlIHRvIGJlIHRoaXMgd2F5
LCBpbiBmYWN0IEkgYmVsaWV2ZQ0KPiB0aGVyZSBpcyBubyBwb2ludCBvZiBpbml0aWFsaXppbmcg
dGhlIGNvdW50ZXIgaWYgdGhlcmUgaXMgbm8gdmFsaWQgQ1NSSyBiZWNhdXNlIG9uZQ0KPiBkZXBl
bmQgb24gdGhlIG90aGVyLCBhbHNvIHBlcmhhcHMgeW91IGhhdmUgYSBidWcgaW4gYnRfZ2F0dF9j
bGllbnRfc2V0X2xvY2FsX2NzcmsNCj4gc2luY2UgSSBiZWxpZXZlIGV2ZXJ5IHRpbWUgeW91IHNl
dCBDU1JLIHlvdSBzaG91bGQgcmVzZXQgdGhlIGNvdW50ZXIgb3RoZXJ3aXNlIHlvdQ0KPiBtYXkg
YmUgY2FycnlpbmcgdGhlIGNvdW50ZXJzIGZyb20gdGhlIHBhc3QgQ1NSSyB3aGljaCBwcm9iYWJs
eSB3b24ndCB3b3JrLg0KDQpZb3Ugc2FpZCBjb25kaXRpb24gcmVzdWlsdCBpbiBidWcgd2hpY2gg
SSB3YW50IHRvIGF2b2lkLCBiZWNhdXNlIHdlIHVzZSB0aGlzIGNvbW1hbmQgbGluZQ0KQnkgYnRn
YXR0LWNsaWVudCB0b29sDQpXcml0ZS12YWx1ZSAtdyAtYyA8Q1NSSz4gPHZhbHVlX2hhbmRsZT4g
PHZhbHVlPg0KSWYgdXNlciB3cml0ZSBzaWduZWQgY29tbWFuZCB0d2ljZSwgaXQgd291bGQgY2Fs
bCBidF9nYXR0X2NsaWVudF9zZXRfbG9jYWxfY3NyayB0d2ljZS4gDQpTbyBpdCB3aWxsIHJlc3Rv
cmUgc2lnbl9jbnQsIHRoYXQncyB3aHkgSSBpbml0aWFsaXplIHRoZSBzaWduX2NudCBpbiBidF9h
dHRfbmV3IHRvIGF2b2lkIHRoaXMgYnVnLg0KDQpJbiBmYWN0LCB0aGUgb25seSB3YXkgaXMgdGhh
dCB3ZSBzaG91bGQgdGVsbCB1c2VyIHNldCBDU1JLIG9uY2UgaWYgQ1NSSyBkaWQgbm90IGNoYW5n
ZSB0byANCnJlc29sdmUgdGhpcyBwcm9ibGVtLg0KIA0KPiBBbHNvIGRvbid0IHRvIGhhdmUgdG8g
cGFzcyB0aGUgY291bnRlciB3aGVuIHNldHRpbmcgdGhlIGtleSwgdGhlIGNvdW50ZXIgcHJvYmFi
bHkNCj4gbmVlZCB0byBiZSByZXN0b3JlZCBvbiBldmVyeSBjb25uZWN0aW9uIHNvIGJ0X2dhdHRf
Y2xpZW50X3NldF9sb2NhbF9jc3JrIGFuZA0KPiBidF9hdHRfc2V0X3JlbW90ZV9jc3JrIHByb2Jh
Ymx5IG5lZWRzIHRvIHRha2UgdGhlIGNvdW50ZXIgYW5kIGluc3RlYWQgb2YgaGF2aW5nIGENCj4g
dmFsaWQgZmxhZyB5b3UgY2FuIHByb2JhYmx5IHJlc2V0IGJ5IHNldHRpbmcgTlVMTCBhcyBrZXkg
b3IgYW4gaW52YWxpZCBjb3VudGVyIGlmIG9uZQ0KPiBleGlzdHMuDQoNCklmIHdlIGRlZmluZSB0
aGUgbmV3IHN0cnVjdCBuYW1pbmcgc2lnbmVkX2luZm8gdG8gc3RvcmUgY3NyayBhbmQgc2lnbl9j
bnQsIHdoYXQncyB5b3VyIHBvaW50Pw0KDQo+IA0KPiA+IElmIHdlIGhhdmUgcG9pbnRlcnMgdG8g
YSBzdHJ1Y3QgaW5jbHVkaW5nIGxvY2FsX2NzcmsgYW5kDQo+ID4gbG9jYWxfc2lnbmRfY250LCB3
ZSBpbml0aWFsaXplIHRoZSBsb2NhbF9zaWduX2NudCBtZWFud2hpbGUgdGhlIHN0cnVjdCBzaG91
bGQgbm90DQo+IGJlIE5VTEwgYmVjYXVzZSBvZiBpbmN1ZGluZyBpdC4gSW4gZmFjdCwgbG9jYWxf
Y3NyayBpcyBub3QgdmFsaWQgYXQgdGhpcyB0aW1lLg0KPiA+IElmIHdlIGluaXRpYWxpemUgdGhl
IGxvY2FsX3NpZ25lZF9jbnQgd2hlbiBDU1JLIGlzIHZhbGlkICwgaXQgd291bGQgYmUNCj4gPiBy
aXNrIHRvIGNoYW5nZSBsb2NhbF9zaWduZWRfY250IGJ5IHVzZXIgY2FsbGluZyBidF9nYXR0X2Ns
aWVudF9zZXRfbG9jYWxfY3Nyay4NCj4gPj4gPiAgfTsNCj4gPj4gPg0KPiA+PiA+ICBlbnVtIGF0
dF9vcF90eXBlIHsNCj4gPj4gPiBAQCAtMTc2LDYgKzE4Nyw4IEBAIHN0cnVjdCBhdHRfc2VuZF9v
cCB7DQo+ID4+ID4gICAgICAgICBidF9hdHRfcmVzcG9uc2VfZnVuY190IGNhbGxiYWNrOw0KPiA+
PiA+ICAgICAgICAgYnRfYXR0X2Rlc3Ryb3lfZnVuY190IGRlc3Ryb3k7DQo+ID4+ID4gICAgICAg
ICB2b2lkICp1c2VyX2RhdGE7DQo+ID4+ID4gKw0KPiA+PiA+ICsgICAgICAgc3RydWN0IGJ0X2F0
dCAqYXR0Ow0KPiA+PiA+ICB9Ow0KPiA+PiA+DQo+ID4+ID4gIHN0YXRpYyB2b2lkIGRlc3Ryb3lf
YXR0X3NlbmRfb3Aodm9pZCAqZGF0YSkgQEAgLTI3Nyw2ICsyOTAsMTAgQEANCj4gPj4gPiBzdGF0
aWMgYm9vbCBlbmNvZGVfcGR1KHN0cnVjdCBhdHRfc2VuZF9vcCAqb3AsIGNvbnN0IHZvaWQgKnBk
dSwNCj4gPj4gPiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICB1aW50MTZfdCBsZW5ndGgsDQo+ID4+ID4gdWludDE2X3QgbXR1KSAgew0KPiA+PiA+ICAgICAg
ICAgdWludDE2X3QgcGR1X2xlbiA9IDE7DQo+ID4+ID4gKyAgICAgICBzdHJ1Y3QgYnRfYXR0ICph
dHQgPSBvcC0+YXR0Ow0KPiA+PiA+ICsNCj4gPj4gPiArICAgICAgIGlmIChvcC0+b3Bjb2RlICYg
QVRUX09QX1NJR05FRF9NQVNLKQ0KPiA+PiA+ICsgICAgICAgICAgICAgICBwZHVfbGVuICs9IEJU
X0FUVF9TSUdOQVRVUkVfTEVOOw0KPiA+PiA+DQo+ID4+ID4gICAgICAgICBpZiAobGVuZ3RoICYm
IHBkdSkNCj4gPj4gPiAgICAgICAgICAgICAgICAgcGR1X2xlbiArPSBsZW5ndGg7DQo+ID4+ID4g
QEAgLTI5MywxNyArMzEwLDMyIEBAIHN0YXRpYyBib29sIGVuY29kZV9wZHUoc3RydWN0IGF0dF9z
ZW5kX29wDQo+ID4+ID4gKm9wLCBjb25zdA0KPiA+PiB2b2lkICpwZHUsDQo+ID4+ID4gICAgICAg
ICBpZiAocGR1X2xlbiA+IDEpDQo+ID4+ID4gICAgICAgICAgICAgICAgIG1lbWNweShvcC0+cGR1
ICsgMSwgcGR1LCBsZW5ndGgpOw0KPiA+PiA+DQo+ID4+ID4gKyAgICAgICBpZiAob3AtPm9wY29k
ZSAmIEFUVF9PUF9TSUdORURfTUFTSykgew0KPiA+PiA+ICsgICAgICAgICAgICAgICBpZiAoYnRf
Y3J5cHRvX3NpZ25fYXR0KGF0dC0+Y3J5cHRvLA0KPiA+PiA+ICsgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICBhdHQtPmxvY2FsX2NzcmssDQo+ID4+ID4gKyAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgIG9wLT5wZHUsDQo+ID4+ID4gKyAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgIDEgKyBsZW5ndGgsDQo+ID4+ID4gKyAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGF0dC0+bG9jYWxfc2lnbl9jbnQsDQo+ID4+
ID4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICYoKHVpbnQ4X3QgKikg
b3AtPnBkdSlbMSArDQo+ID4+IGxlbmd0aF0pKQ0KPiA+PiA+ICsgICAgICAgICAgICAgICAgICAg
ICAgIGF0dC0+bG9jYWxfc2lnbl9jbnQrKzsNCj4gPj4gPiArICAgICAgICAgICAgICAgZWxzZQ0K
PiA+PiA+ICsgICAgICAgICAgICAgICAgICAgICAgIHJldHVybiBmYWxzZTsNCj4gPj4gPiArICAg
ICAgIH0NCj4gPj4gPiArDQo+ID4+DQo+ID4+IFlvdSBjYW4gcHJvYmFibHkgYmFpbCBvdXQgZWFy
bHkgaWYgeW91IGNoZWNrICEob3AtPm9wY29kZSAmDQo+ID4+IEFUVF9PUF9TSUdORURfTUFTSyks
IGFsc28gcGVyaGFwcyBpdCBpcyBhIGdvb2QgaWRlYSB0byBzdGFydCBkZWZpbmluZw0KPiA+PiBz
dHJ1Y3RzIGZvciB0aGUgUERVcyB0byBzaW1wbGlmeSB0aGUgYWNjZXNzLg0KPiA+Pg0KPiA+IElm
IHdlIGJhaWwgb3V0IHRoaXMgY2hlY2tpbmcgZWFybHkgLCBtYXliZSB3ZSBoYXZlIHRvIG5lZWQg
dHdvDQo+ID4gZGlmZmVyZW50IGVuY29kZV9wZHUgZnVuY3Rpb24gdG8gaW1wbGVtZW50IGl0IHN1
Y2ggYXMgbmFtaW5nIG5ldw0KPiBlbmNvZGVfc2lnbmVkX3BkdSBmdW5jdGlvbiB0byBkbyB0aGlz
Lg0KPiANCj4gQnV0IHlvdSByZXR1cm4gdHJ1ZSBhbnl3YXkgYWZ0ZXIgdGhpcyBjb2RlLCBubyBp
ZGVhIHdoeSB5b3UgbmVlZCBhIG5ldyBmdW5jdGlvbiBmb3INCj4gdGhhdCwgYWxsIEkgd2FzIHN1
Z2dlc3RpbmcgaXMgdGhlIGZvbGxvd2luZzoNCj4gDQo+IGlmICghKG9wLT5vcGNvZGUgJiBBVFRf
T1BfU0lHTkVEX01BU0spKQ0KPiAgICAgcmV0dXJuIHRydWU7DQo+IA0KPiBpZiAoIWJ0X2NyeXB0
b19zaWduX2F0dC4uLikNCj4gICAgIHJldHVybiBmYWxzZTsNCj4gDQo+IGF0dC0+bG9jYWxfc2ln
bl9jbnQrKzsNCj4gDQo+IHJldHVybiB0cnVlOw0KPiANCg0KSSBnb3QgeW91ciBtZWFuaW5nIG5v
dy4gSSBoYXZlIHJldHVybmVkIGZhbHNlIHdoZW4gYnRfY3J5cHRvX3NpZ25fYXR0LiBGYWlsZWQN
Cg0KICsgICAgICAgICAgICAgICBlbHNlDQogKyAgICAgICAgICAgICAgICAgICAgICAgcmV0dXJu
IGZhbHNlOw0KDQpZb3VyIHByb3Bvc2FsIGlzIGdvb2Qgc28gYXMgdG8gbWFrZSBjb2RlIG1vcmUg
Y2xlYXJlci4gSSB3b3VsZCBhY2NlcHQgdGhhdC4NCg0KVGhhbmtzDQpDaGFvamllIEd1DQo=

Re: [PATCH v4 1/3] shared/att.c: Add signed command outgoing and CSRK function

[Luiz Augusto von Dentz]

Hi,

On Thu, Oct 9, 2014 at 12:38 PM, Gu, Chao Jie <chao.jie.gu@intel.com> wrote:
> Hi Luiz,
>> >
>> >> > @@ -77,6 +78,16 @@ struct bt_att {
>> >> >         bt_att_debug_func_t debug_callback;
>> >> >         bt_att_destroy_func_t debug_destroy;
>> >> >         void *debug_data;
>> >> > +
>> >> > +       struct bt_crypto *crypto;
>> >> > +
>> >> > +       bool valid_local_csrk;
>> >> > +       uint8_t local_csrk[16];
>> >> > +       uint32_t local_sign_cnt;
>> >> > +
>> >> > +       bool valid_remote_csrk;
>> >> > +       uint8_t remote_csrk[16];
>> >> > +       uint32_t remote_sign_cnt;
>> >>
>> >> Maybe it is better to have pointers to a structs so you can free the
>> >> data once it becomes invalid and it also removes the necessity of
>> >> extra flags just to tell if the data is still valid since you can check if the pointer is
>> not NULL then it must be valid.
>> >
>> > This is good idea to remove the extra flags. However, we define the
>> > struct to do this which would result in some problem. Now we
>> > initialize the local_sign_cnt in the bt_att_new function and only add
>> > local_sign_cnt value when signed write command outgoing. Meanwile, we
>> > set local CSRK in the bt_gatt_client_set_local_csrk function when there is valid
>> CSRK provided. This two Parameter set at different time now.
>>
>> Well I guess this was your design, it doesn't have to be this way, in fact I believe
>> there is no point of initializing the counter if there is no valid CSRK because one
>> depend on the other, also perhaps you have a bug in bt_gatt_client_set_local_csrk
>> since I believe every time you set CSRK you should reset the counter otherwise you
>> may be carrying the counters from the past CSRK which probably won't work.
>
> You said condition resuilt in bug which I want to avoid, because we use this command line
> By btgatt-client tool
> Write-value -w -c <CSRK> <value_handle> <value>
> If user write signed command twice, it would call bt_gatt_client_set_local_csrk twice.
> So it will restore sign_cnt, that's why I initialize the sign_cnt in bt_att_new to avoid this bug.

Considering what the spec says about SignCounter and that we do in
fact store the counter persistently I believe the counter is valid as
long as CSRK is valid, which means it is not per connection and
initializing it on bt_att_new is probably wrong since the counter will
probably be out of sync. We should either create a dedicate command to
set CSRK and the SignCounter or make write-value to take the counter
as well otherwise it wont work except with a fresh CSRK and
SignCounter which perhaps is the case of btgatt-client but not if we
want to reuse in the daemons.

> In fact, the only way is that we should tell user set CSRK once if CSRK did not change to
> resolve this problem.
>
>> Also don't to have to pass the counter when setting the key, the counter probably
>> need to be restored on every connection so bt_gatt_client_set_local_csrk and
>> bt_att_set_remote_csrk probably needs to take the counter and instead of having a
>> valid flag you can probably reset by setting NULL as key or an invalid counter if one
>> exists.
>
> If we define the new struct naming signed_info to store csrk and sign_cnt, what's your point?
>
>>
>> > If we have pointers to a struct including local_csrk and
>> > local_signd_cnt, we initialize the local_sign_cnt meanwhile the struct should not
>> be NULL because of incuding it. In fact, local_csrk is not valid at this time.
>> > If we initialize the local_signed_cnt when CSRK is valid , it would be
>> > risk to change local_signed_cnt by user calling bt_gatt_client_set_local_csrk.
>> >> >  };
>> >> >
>> >> >  enum att_op_type {
>> >> > @@ -176,6 +187,8 @@ struct att_send_op {
>> >> >         bt_att_response_func_t callback;
>> >> >         bt_att_destroy_func_t destroy;
>> >> >         void *user_data;
>> >> > +
>> >> > +       struct bt_att *att;
>> >> >  };
>> >> >
>> >> >  static void destroy_att_send_op(void *data) @@ -277,6 +290,10 @@
>> >> > static bool encode_pdu(struct att_send_op *op, const void *pdu,
>> >> >                                                 uint16_t length,
>> >> > uint16_t mtu)  {
>> >> >         uint16_t pdu_len = 1;
>> >> > +       struct bt_att *att = op->att;
>> >> > +
>> >> > +       if (op->opcode & ATT_OP_SIGNED_MASK)
>> >> > +               pdu_len += BT_ATT_SIGNATURE_LEN;
>> >> >
>> >> >         if (length && pdu)
>> >> >                 pdu_len += length;
>> >> > @@ -293,17 +310,32 @@ static bool encode_pdu(struct att_send_op
>> >> > *op, const
>> >> void *pdu,
>> >> >         if (pdu_len > 1)
>> >> >                 memcpy(op->pdu + 1, pdu, length);
>> >> >
>> >> > +       if (op->opcode & ATT_OP_SIGNED_MASK) {
>> >> > +               if (bt_crypto_sign_att(att->crypto,
>> >> > +                                       att->local_csrk,
>> >> > +                                       op->pdu,
>> >> > +                                       1 + length,
>> >> > +                                       att->local_sign_cnt,
>> >> > +                                       &((uint8_t *) op->pdu)[1 +
>> >> length]))
>> >> > +                       att->local_sign_cnt++;
>> >> > +               else
>> >> > +                       return false;
>> >> > +       }
>> >> > +
>> >>
>> >> You can probably bail out early if you check !(op->opcode &
>> >> ATT_OP_SIGNED_MASK), also perhaps it is a good idea to start defining
>> >> structs for the PDUs to simplify the access.
>> >>
>> > If we bail out this checking early , maybe we have to need two
>> > different encode_pdu function to implement it such as naming new
>> encode_signed_pdu function to do this.
>>
>> But you return true anyway after this code, no idea why you need a new function for
>> that, all I was suggesting is the following:
>>
>> if (!(op->opcode & ATT_OP_SIGNED_MASK))
>>     return true;
>>
>> if (!bt_crypto_sign_att...)
>>     return false;
>>
>> att->local_sign_cnt++;
>>
>> return true;
>>
>
> I got your meaning now. I have returned false when bt_crypto_sign_att. Failed
>
>  +               else
>  +                       return false;
>
> Your proposal is good so as to make code more clearer. I would accept that.
>
> Thanks
> Chaojie Gu



-- 
Luiz Augusto von Dentz