[PATCH v3 0/4] support signed write command

[PATCH v3 0/4] support signed write command

[Gu Chaojie]

This patch set submit Makefile.tools change to fix patch set v2 link issue.
And remove type enum in the CSRK related funtion make a call to these API
more clear.

Gu Chaojie (4):
  shared/att.c: Add signed command outgoing and CSRK function
  shared/gatt-client: Add CSRK part to support signed write
  tools/btgatt-client: Add signed write CSRK option
  Modify Makefile.tool to link

 Makefile.tools           |    3 +-
 src/shared/att-types.h   |    3 ++
 src/shared/att.c         |  120 ++++++++++++++++++++++++++++++++++++++++++++--
 src/shared/att.h         |   16 +++++++
 src/shared/gatt-client.c |   27 +++++++++--
 src/shared/gatt-client.h |    4 ++
 tools/btgatt-client.c    |   35 +++++++++++++-
 7 files changed, 197 insertions(+), 11 deletions(-)

-- 
1.7.10.4

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

[Gu Chaojie]

Compared with patch v2, remove type enum in all CSRK related funciton to 
make these API more clear.

---
 src/shared/att-types.h |    3 ++
 src/shared/att.c       |  120 ++++++++++++++++++++++++++++++++++++++++++++++--
 src/shared/att.h       |   16 +++++++
 3 files changed, 136 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 a5cab45..4562a8c 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);
@@ -698,6 +731,8 @@ struct bt_att *bt_att_new(int fd)
 
 	att->fd = fd;
 
+	att->local_sign_cnt = 0;
+	att->remote_sign_cnt = 0;
 	att->mtu = BT_ATT_DEFAULT_LE_MTU;
 	att->buf = malloc(att->mtu);
 	if (!att->buf)
@@ -707,6 +742,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;
@@ -741,6 +780,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);
@@ -931,7 +971,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;
@@ -1116,3 +1156,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 v3 2/4] shared/gatt-client: Add CSRK part to support signed write

[Gu Chaojie]

Compared with patch v2, remove type enum in CSRK funciton for btgatt-client
tool to easy use this API.

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

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 6dc8e95..aee3969 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -1753,19 +1753,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);
 }
@@ -2246,3 +2253,13 @@ 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,
+				bool valid_local_csrk,
+				uint8_t key[16])
+{
+	if (!client)
+		return false;
+
+	return bt_att_set_local_csrk(client->att, valid_local_csrk, key);
+}
diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h
index 6807f6b..0684f30 100644
--- a/src/shared/gatt-client.h
+++ b/src/shared/gatt-client.h
@@ -162,3 +162,7 @@ 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,
+				bool valid_local_csrk,
+				uint8_t key[16]);
-- 
1.7.10.4

[PATCH v3 3/4] tools/btgatt-client: Add signed write CSRK option

[Gu Chaojie]

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

diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
index d900e08..0b6d545 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,11 @@ 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;
+	bool valid_csrk = 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 +554,18 @@ 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;
+			valid_csrk = convert_csrk_key(optarg, csrk);
+			bt_gatt_client_set_local_csrk(cli->gatt,
+						valid_csrk, csrk);
+			break;
 		default:
 			write_value_usage();
 			return;
@@ -588,7 +619,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

[PATCH v3 4/4] Modify Makefile.tool to link

[Gu Chaojie]

Fix patch set v2 link issue when compiling.

---
 Makefile.tools |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Makefile.tools b/Makefile.tools
index 4807813..8823fc0 100644
--- a/Makefile.tools
+++ b/Makefile.tools
@@ -365,7 +365,8 @@ tools_btgatt_client_SOURCES = tools/btgatt-client.c src/uuid-helper.c \
 				src/shared/timeout.h src/shared/timeout-mainloop.c \
 				src/shared/att-types.h src/shared/att.h src/shared/att.c \
 				src/shared/gatt-helpers.h src/shared/gatt-helpers.c \
-				src/shared/gatt-client.h src/shared/gatt-client.c
+				src/shared/gatt-client.h src/shared/gatt-client.c \
+				src/shared/crypto.h src/shared/crypto.c
 tools_btgatt_client_LDADD = lib/libbluetooth-internal.la
 
 EXTRA_DIST += tools/bdaddr.1
-- 
1.7.10.4

Re: [PATCH v3 2/4] shared/gatt-client: Add CSRK part to support signed write

[Marcel Holtmann]

Hi Chajie,

> Compared with patch v2, remove type enum in CSRK funciton for btgatt-client
> tool to easy use this API.
> 
> ---
> src/shared/gatt-client.c |   27 ++++++++++++++++++++++-----
> src/shared/gatt-client.h |    4 ++++
> 2 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
> index 6dc8e95..aee3969 100644
> --- a/src/shared/gatt-client.c
> +++ b/src/shared/gatt-client.c
> @@ -1753,19 +1753,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);
> }
> @@ -2246,3 +2253,13 @@ 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,
> +				bool valid_local_csrk,
> +				uint8_t key[16])
> +{
> +	if (!client)
> +		return false;
> +
> +	return bt_att_set_local_csrk(client->att, valid_local_csrk, key);
> +}
> diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h
> index 6807f6b..0684f30 100644
> --- a/src/shared/gatt-client.h
> +++ b/src/shared/gatt-client.h
> @@ -162,3 +162,7 @@ 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,
> +				bool valid_local_csrk,
> +				uint8_t key[16]);

why do we have the parameter valid_local_csrk. I find that highly confusing. If you do not have a valid CSRK, then do not call this function. It should be that simple.

Regards

Marcel

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

[Lukasz Rymanowski]

Hi Chaojie,

On Fri, Sep 26, 2014 at 9:48 AM, Gu Chaojie <chao.jie.gu@intel.com> wrote:
>
> Compared with patch v2, remove type enum in all CSRK related funciton to
> make these API more clear.
>
> ---
>  src/shared/att-types.h |    3 ++
>  src/shared/att.c       |  120 ++++++++++++++++++++++++++++++++++++++++++++++--
>  src/shared/att.h       |   16 +++++++
>  3 files changed, 136 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 a5cab45..4562a8c 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);
> @@ -698,6 +731,8 @@ struct bt_att *bt_att_new(int fd)
>
>         att->fd = fd;
>
> +       att->local_sign_cnt = 0;
> +       att->remote_sign_cnt = 0;
>         att->mtu = BT_ATT_DEFAULT_LE_MTU;
>         att->buf = malloc(att->mtu);
>         if (!att->buf)
> @@ -707,6 +742,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;
> @@ -741,6 +780,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);
> @@ -931,7 +971,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;
> @@ -1116,3 +1156,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)


Why we need that get function ? If we assume that signing is done in
bt_att then there is need only to feed bt_att with the CSRK.
 BTW. There should be a way to set sign_cnt as well as this shall be
in same storage as CSRK

I can guess that idea behind this bt_att_get_ is that client of bt_att
should have possibility to update its sign_cnt after each transaction.
It is in order to update storage. If so  I would propose to have some
callback registered in bt_att for this purpose.

BR
Lukasz
>
> +{
> +       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

Re: [PATCH v3 2/4] shared/gatt-client: Add CSRK part to support signed write

[Lukasz Rymanowski]

Hi Chaojie,

On Fri, Sep 26, 2014 at 9:48 AM, Gu Chaojie <chao.jie.gu@intel.com> wrote:
> Compared with patch v2, remove type enum in CSRK funciton for btgatt-client
> tool to easy use this API.
>
> ---
>  src/shared/gatt-client.c |   27 ++++++++++++++++++++++-----
>  src/shared/gatt-client.h |    4 ++++
>  2 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
> index 6dc8e95..aee3969 100644
> --- a/src/shared/gatt-client.c
> +++ b/src/shared/gatt-client.c
> @@ -1753,19 +1753,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))

In my opinion bt_att_local_csrk_is_valid API seems to be unnecessary
as CSRK can be checked inside bt_att_send function directly.

BR
\Łukasz

> +                       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);
>  }
> @@ -2246,3 +2253,13 @@ 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,
> +                               bool valid_local_csrk,
> +                               uint8_t key[16])
> +{
> +       if (!client)
> +               return false;
> +
> +       return bt_att_set_local_csrk(client->att, valid_local_csrk, key);
> +}
> diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h
> index 6807f6b..0684f30 100644
> --- a/src/shared/gatt-client.h
> +++ b/src/shared/gatt-client.h
> @@ -162,3 +162,7 @@ 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,
> +                               bool valid_local_csrk,
> +                               uint8_t key[16]);
> --
> 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

RE: [PATCH v3 2/4] shared/gatt-client: Add CSRK part to support signed write

[Gu, Chao Jie]

Hi Marcel,

> > +bool bt_gatt_client_set_local_csrk(struct bt_gatt_client *client,
> > +				bool valid_local_csrk,
> > +				uint8_t key[16]);
>=20
> why do we have the parameter valid_local_csrk. I find that highly confusi=
ng. If you
> do not have a valid CSRK, then do not call this function. It should be th=
at simple.
>=20

My initial purpose to set this parameter valid_local_csrk here, user can ca=
ll one API
to set flag valid_local_csrk ture/false and CSRK key in one time.
According to your proposal, I think it really result in confusion for user.

So for more clear to call API , I think we should split two API for upper l=
ayer:
1. When we need to set CSRK , check the CSRK is valid , then call bt_gatt_c=
lient_set_local_csrk
  Just to pass one parameter key to function and in the function to set fla=
g valid_local_csrk is true.

2 When two device dispair each other, we should give user possibility to un=
set flag and CSRK.
  So in this condition , we have to create bt_gatt_client_unset_local_csrk(=
struct bt_gatt_client *client)
  to clear CSRK and set flag valid_local_csrk to false.

Do you think this thought would be ok for you?

Best Regards
Chaojie Gu

Re: [PATCH v3 2/4] shared/gatt-client: Add CSRK part to support signed write

[Marcel Holtmann]

Hi Chao Jie,

>>> +bool bt_gatt_client_set_local_csrk(struct bt_gatt_client *client,
>>> +				bool valid_local_csrk,
>>> +				uint8_t key[16]);
>> 
>> why do we have the parameter valid_local_csrk. I find that highly confusing. If you
>> do not have a valid CSRK, then do not call this function. It should be that simple.
>> 
> 
> My initial purpose to set this parameter valid_local_csrk here, user can call one API
> to set flag valid_local_csrk ture/false and CSRK key in one time.
> According to your proposal, I think it really result in confusion for user.
> 
> So for more clear to call API , I think we should split two API for upper layer:
> 1. When we need to set CSRK , check the CSRK is valid , then call bt_gatt_client_set_local_csrk
>  Just to pass one parameter key to function and in the function to set flag valid_local_csrk is true.
> 
> 2 When two device dispair each other, we should give user possibility to unset flag and CSRK.
>  So in this condition , we have to create bt_gatt_client_unset_local_csrk(struct bt_gatt_client *client)
>  to clear CSRK and set flag valid_local_csrk to false.
> 
> Do you think this thought would be ok for you?

I am not really worried about these details anyway. If you remove the bond between two devices, then the connection will be terminated. Which means that next time around you get a new bt_att object. If you do not have any CSRK the next time, you just not set them in the first place.

Regards

Marcel

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

[Gu, Chao Jie]

SGkgTHVrYXN6LA0KDQo+ID4gKw0KPiA+ICtib29sIGJ0X2F0dF9nZXRfbG9jYWxfY3NyayhzdHJ1
Y3QgYnRfYXR0ICphdHQsIHVpbnQ4X3Qga2V5WzE2XSwNCj4gPiArICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgdWludDMyX3QgKmxvY2FsX3NpZ25fY250KQ0KPiANCj4gDQo+
IFdoeSB3ZSBuZWVkIHRoYXQgZ2V0IGZ1bmN0aW9uID8gSWYgd2UgYXNzdW1lIHRoYXQgc2lnbmlu
ZyBpcyBkb25lIGluIGJ0X2F0dCB0aGVuDQo+IHRoZXJlIGlzIG5lZWQgb25seSB0byBmZWVkIGJ0
X2F0dCB3aXRoIHRoZSBDU1JLLg0KPiAgQlRXLiBUaGVyZSBzaG91bGQgYmUgYSB3YXkgdG8gc2V0
IHNpZ25fY250IGFzIHdlbGwgYXMgdGhpcyBzaGFsbCBiZSBpbiBzYW1lDQo+IHN0b3JhZ2UgYXMg
Q1NSSw0KPiANCj4gSSBjYW4gZ3Vlc3MgdGhhdCBpZGVhIGJlaGluZCB0aGlzIGJ0X2F0dF9nZXRf
IGlzIHRoYXQgY2xpZW50IG9mIGJ0X2F0dCBzaG91bGQgaGF2ZQ0KPiBwb3NzaWJpbGl0eSB0byB1
cGRhdGUgaXRzIHNpZ25fY250IGFmdGVyIGVhY2ggdHJhbnNhY3Rpb24uDQo+IEl0IGlzIGluIG9y
ZGVyIHRvIHVwZGF0ZSBzdG9yYWdlLiBJZiBzbyAgSSB3b3VsZCBwcm9wb3NlIHRvIGhhdmUgc29t
ZSBjYWxsYmFjaw0KPiByZWdpc3RlcmVkIGluIGJ0X2F0dCBmb3IgdGhpcyBwdXJwb3NlLg0KPg0K
QWNjb3JkaW5nIHRvIHlvdXIgcHJvcG9zYWwsIEkgYW0gbm90IGNsZWFyIHdoZXJlIGFuZCB3aGVu
IHlvdSB3YW50IHRvIHJlZ2lzdGVyDQpjYWxsYmFjayB0byBnZXQgc2lnbl9jbnQsIGFuZCBJIHRo
aW5rIHJlZ2lzdGVyIG5ldyBjYWxsYmFjayBmb3Igb25seSBvbmUgc2lnbmVkX2NudA0KUGFyYW1l
dGVyLCBpdCBtYWtlIHNlbnNlID8NCg0KSW4gbXkgdGhvdWdodCAsIHRoaXMgbWV0aG9kIHVzZXIg
YWxzbyBjYW4gZ2V0IHNpZ25kX2NudCBhZnRlciBzaWduZWRfd3JpdGVfY21kLg0KSW4gZW5jb2Rl
X3BkdSBmdW5jdGlvbiwgYWZ0ZXIgd2UgZW5jb2RlIHBkdSB3aXRoIENTUkssIHRoZW4gaXQgd2ls
bCB1cGRhdGUgc2lnbl9jbnQNCmluIHRoZSBidF9hdHQgc3RydWN0dXJlIGZvciBuZXh0IHNpZ25l
ZCB3cml0ZSBjb21tYW5kLg0KDQpJZiBjbGllbnQgb2YgYnRfYXR0IG5lZWQgdG8gdXBkYXRlIGl0
cyBzaWduX2NudCBpbiB0aGUgc3RvcmFnZSwgd2hlbiBidF9hdHRfc2VuZCBmb3INClNJR05FRF9X
UklURV9DT01NQU5EIHJldHVybiB0dXJlLCB1c2VyIGNhbiBjYWxsIGJ0X2F0dF9nZXRfbG9jYWxf
Y3NyayB0byANCmdldCBuZXdlc3Qgc2lnbl9jbnQgYW5kIHN0b3JlIGl0Lg0KDQpCZXN0IFJlZ2Fy
ZHMNCkNoYW9qaWUgR3UNCg==

RE: [PATCH v3 2/4] shared/gatt-client: Add CSRK part to support signed write

[Gu, Chao Jie]

SGkgTHVrYXN6LA0KPiA+IC0tLQ0KPiA+ICBzcmMvc2hhcmVkL2dhdHQtY2xpZW50LmMgfCAgIDI3
ICsrKysrKysrKysrKysrKysrKysrKystLS0tLQ0KPiA+ICBzcmMvc2hhcmVkL2dhdHQtY2xpZW50
LmggfCAgICA0ICsrKysNCj4gPiAgMiBmaWxlcyBjaGFuZ2VkLCAyNiBpbnNlcnRpb25zKCspLCA1
IGRlbGV0aW9ucygtKQ0KPiA+DQo+ID4gZGlmZiAtLWdpdCBhL3NyYy9zaGFyZWQvZ2F0dC1jbGll
bnQuYyBiL3NyYy9zaGFyZWQvZ2F0dC1jbGllbnQuYyBpbmRleA0KPiA+IDZkYzhlOTUuLmFlZTM5
NjkgMTAwNjQ0DQo+ID4gLS0tIGEvc3JjL3NoYXJlZC9nYXR0LWNsaWVudC5jDQo+ID4gKysrIGIv
c3JjL3NoYXJlZC9nYXR0LWNsaWVudC5jDQo+ID4gQEAgLTE3NTMsMTkgKzE3NTMsMjYgQEAgYm9v
bCBidF9nYXR0X2NsaWVudF9yZWFkX2xvbmdfdmFsdWUoc3RydWN0DQo+ID4gYnRfZ2F0dF9jbGll
bnQgKmNsaWVudCwgIGJvb2wgYnRfZ2F0dF9jbGllbnRfd3JpdGVfd2l0aG91dF9yZXNwb25zZShz
dHJ1Y3QNCj4gYnRfZ2F0dF9jbGllbnQgKmNsaWVudCwNCj4gPiAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgdWludDE2X3QgdmFsdWVfaGFuZGxlLA0KPiA+ICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBib29sIHNpZ25lZF93cml0ZSwNCj4gPiAt
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgdWludDhfdCAqdmFsdWUsIHVp
bnQxNl90IGxlbmd0aCkgew0KPiA+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICB1aW50OF90ICp2YWx1ZSwgdWludDE2X3QNCj4gPiArbGVuZ3RoKSB7DQo+ID4gICAgICAg
ICB1aW50OF90IHBkdVsyICsgbGVuZ3RoXTsNCj4gPg0KPiA+ICAgICAgICAgaWYgKCFjbGllbnQp
DQo+ID4gICAgICAgICAgICAgICAgIHJldHVybiAwOw0KPiA+DQo+ID4gLSAgICAgICAvKiBUT0RP
OiBTdXBwb3J0IHRoaXMgb25jZSBidF9hdHRfc2VuZCBzdXBwb3J0cyBzaWduZWQgd3JpdGVzLiAq
Lw0KPiA+IC0gICAgICAgaWYgKHNpZ25lZF93cml0ZSkNCj4gPiAtICAgICAgICAgICAgICAgcmV0
dXJuIDA7DQo+ID4gLQ0KPiA+ICAgICAgICAgcHV0X2xlMTYodmFsdWVfaGFuZGxlLCBwZHUpOw0K
PiA+ICAgICAgICAgbWVtY3B5KHBkdSArIDIsIHZhbHVlLCBsZW5ndGgpOw0KPiA+DQo+ID4gKyAg
ICAgICBpZiAoc2lnbmVkX3dyaXRlKSB7DQo+ID4gKyAgICAgICAgICAgICAgIGlmIChidF9hdHRf
bG9jYWxfY3Nya19pc192YWxpZChjbGllbnQtPmF0dCkpDQo+IA0KPiBJbiBteSBvcGluaW9uIGJ0
X2F0dF9sb2NhbF9jc3JrX2lzX3ZhbGlkIEFQSSBzZWVtcyB0byBiZSB1bm5lY2Vzc2FyeSBhcyBD
U1JLIGNhbg0KPiBiZSBjaGVja2VkIGluc2lkZSBidF9hdHRfc2VuZCBmdW5jdGlvbiBkaXJlY3Rs
eS4NCj4gDQoNCkkgYWdyZWUgd2l0aCB5b3UgdGhhdCBpdCBpcyBhbHNvIG9rIHRvIGJlIGNoZWNr
ZWQgaW5zaWRlIGJ0X2F0dF9zZW5kIGZ1bmN0aW9uIGFuZCBoYXZlIHRob3VnaHQNCmFib3V0IHRo
aXMgcHJvYmxlbSBiZWZvcmUuIA0KQW5kIEkgdGhpbmsgdGhlcmUgYXJlIHR3byBhZHZhbnRhZ2Vz
IG9mIGNoZWNraW5nIHZhbGlkIG91dHNpZGUgYnRfYXR0X3NlbmQgZnVuY3Rpb24NCg0KMS4gcHJv
dmlkZSBBUEkgdG8gdXNlciB0byBjaGVjayBDU1JLIHN0YXR1cyBpZiB0aGV5IHdhbnQuDQoyLiBp
ZiB0aGUgQ1NSSyBpcyBpbnZhbGlkICwgd2UgY2FuIGp1c3QgcmV0dXJuIGZhbHNlIHF1aWNrbHkg
LCB0aGVyZSBpcyBubyBuZWVkIHRvIGdvIG9uIGNhbGxpbmcNCmRvd24gYnRfYXR0X3NlbmQgdGhl
biByZXR1cm4gZmFsc2UgYmFjay4gSXQgd2lsbCBiZSBtb3JlIGVmZmljaWVudCBJIHRoaW5rLg0K
DQpXaGF0J3MgeW91ciBwb2ludCBhYm91dCB0aGlzID8NCg0KQmVzdCBSZWdhcmRzDQpDaGFvamll
IEd1DQoNCg==

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

[Lukasz Rymanowski]

Hi Chaojie,

On Sat, Sep 27, 2014 at 11:26 AM, Gu, Chao Jie <chao.jie.gu@intel.com> wrote:
> Hi Lukasz,
>
>> > +
>> > +bool bt_att_get_local_csrk(struct bt_att *att, uint8_t key[16],
>> > +                                       uint32_t *local_sign_cnt)
>>
>>
>> Why we need that get function ? If we assume that signing is done in bt_att then
>> there is need only to feed bt_att with the CSRK.
>>  BTW. There should be a way to set sign_cnt as well as this shall be in same
>> storage as CSRK
>>
>> I can guess that idea behind this bt_att_get_ is that client of bt_att should have
>> possibility to update its sign_cnt after each transaction.
>> It is in order to update storage. If so  I would propose to have some callback
>> registered in bt_att for this purpose.
>>
> According to your proposal, I am not clear where and when you want to register
> callback to get sign_cnt,

At the same time and from the same place where CSRK is provided to bt_att.
I guess that both values (key and counter) will be stored in same database.

> and I think register new callback for only one signed_cnt
> Parameter, it make sense ?

Why not? :)

>
> In my thought , this method user also can get signd_cnt after signed_write_cmd.
> In encode_pdu function, after we encode pdu with CSRK, then it will update sign_cnt
> in the bt_att structure for next signed write command.
>
> If client of bt_att need to update its sign_cnt in the storage, when bt_att_send for
> SIGNED_WRITE_COMMAND return ture, user can call bt_att_get_local_csrk to
> get newest sign_cnt and store it.

The thing is that it is not "if".  We need to make sure that sign
counter is updated in the database
and I think that having callback is convenient for that. Of course you
can have some helper which
retrieves sign counter after each att transaction and then store it.

We need to remember  that local sign counter is valid as long as CSRK
is, so in order to do any sign write after
reconnect we need to have updated sing counter, otherwise remote will
reject write command.

BR
Lukasz
>
> Best Regards
> Chaojie Gu

Re: [PATCH v3 2/4] shared/gatt-client: Add CSRK part to support signed write

[Lukasz Rymanowski]

HI Chaojie,

On Sat, Sep 27, 2014 at 11:36 AM, Gu, Chao Jie <chao.jie.gu@intel.com> wrote:
> Hi Lukasz,
>> > ---
>> >  src/shared/gatt-client.c |   27 ++++++++++++++++++++++-----
>> >  src/shared/gatt-client.h |    4 ++++
>> >  2 files changed, 26 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c index
>> > 6dc8e95..aee3969 100644
>> > --- a/src/shared/gatt-client.c
>> > +++ b/src/shared/gatt-client.c
>> > @@ -1753,19 +1753,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))
>>
>> In my opinion bt_att_local_csrk_is_valid API seems to be unnecessary as CSRK can
>> be checked inside bt_att_send function directly.
>>
>
> I agree with you that it is also ok to be checked inside bt_att_send function and have thought
> about this problem before.
> And I think there are two advantages of checking valid outside bt_att_send function
>
> 1. provide API to user to check CSRK status if they want.

Ok, the question is why user would like  to do it? Maybe I'm missing something.

> 2. if the CSRK is invalid , we can just return false quickly , there is no need to go on calling
> down bt_att_send then return false back. It will be more efficient I think.

If  bt_att_send() will check the CSRK on the beginning then there is
no difference.

>
> What's your point about this ?
>
> Best Regards
> Chaojie Gu
>

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

[Gu, Chao Jie]

SGkgTHVrYXN6LA0KIA0KPiA+DQo+ID4gSW4gbXkgdGhvdWdodCAsIHRoaXMgbWV0aG9kIHVzZXIg
YWxzbyBjYW4gZ2V0IHNpZ25kX2NudCBhZnRlciBzaWduZWRfd3JpdGVfY21kLg0KPiA+IEluIGVu
Y29kZV9wZHUgZnVuY3Rpb24sIGFmdGVyIHdlIGVuY29kZSBwZHUgd2l0aCBDU1JLLCB0aGVuIGl0
IHdpbGwNCj4gPiB1cGRhdGUgc2lnbl9jbnQgaW4gdGhlIGJ0X2F0dCBzdHJ1Y3R1cmUgZm9yIG5l
eHQgc2lnbmVkIHdyaXRlIGNvbW1hbmQuDQo+ID4NCj4gPiBJZiBjbGllbnQgb2YgYnRfYXR0IG5l
ZWQgdG8gdXBkYXRlIGl0cyBzaWduX2NudCBpbiB0aGUgc3RvcmFnZSwgd2hlbg0KPiA+IGJ0X2F0
dF9zZW5kIGZvciBTSUdORURfV1JJVEVfQ09NTUFORCByZXR1cm4gdHVyZSwgdXNlciBjYW4gY2Fs
bA0KPiA+IGJ0X2F0dF9nZXRfbG9jYWxfY3NyayB0byBnZXQgbmV3ZXN0IHNpZ25fY250IGFuZCBz
dG9yZSBpdC4NCj4gDQo+IFRoZSB0aGluZyBpcyB0aGF0IGl0IGlzIG5vdCAiaWYiLiAgV2UgbmVl
ZCB0byBtYWtlIHN1cmUgdGhhdCBzaWduIGNvdW50ZXIgaXMgdXBkYXRlZA0KPiBpbiB0aGUgZGF0
YWJhc2UgYW5kIEkgdGhpbmsgdGhhdCBoYXZpbmcgY2FsbGJhY2sgaXMgY29udmVuaWVudCBmb3Ig
dGhhdC4gT2YgY291cnNlIHlvdQ0KPiBjYW4gaGF2ZSBzb21lIGhlbHBlciB3aGljaCByZXRyaWV2
ZXMgc2lnbiBjb3VudGVyIGFmdGVyIGVhY2ggYXR0IHRyYW5zYWN0aW9uIGFuZA0KPiB0aGVuIHN0
b3JlIGl0Lg0KPiANCj4gV2UgbmVlZCB0byByZW1lbWJlciAgdGhhdCBsb2NhbCBzaWduIGNvdW50
ZXIgaXMgdmFsaWQgYXMgbG9uZyBhcyBDU1JLIGlzLCBzbyBpbg0KPiBvcmRlciB0byBkbyBhbnkg
c2lnbiB3cml0ZSBhZnRlciByZWNvbm5lY3Qgd2UgbmVlZCB0byBoYXZlIHVwZGF0ZWQgc2luZyBj
b3VudGVyLA0KPiBvdGhlcndpc2UgcmVtb3RlIHdpbGwgcmVqZWN0IHdyaXRlIGNvbW1hbmQuDQo+
IA0KDQpXZSBjYW4gc2VlIGFuZHJvaWQvZ2F0dC5jICwgd2hlbiBnYXR0X3NpZ25lZF93cml0ZV9j
bWQgZG9uZSwNCkNhbGwgYnRfdXBkYXRlX3NpZ25fY291bnQgdG8gdXBhdGUgYW5kIHN0b3JlIHRo
ZSBzaWduZWRfY250Lg0KDQpUaGVyZSBpcyB0aGUgc2FtZSBjb25kaXRpb24gaGVyZS4gWW91IGNh
biBjYWxsIGJ0X2F0dF9nZXRfbG9jYWxfY3NyayB0byB1cGRhdGUgYW5kIA0KU3RvcmUgaXQgYWZ0
ZXIgc2lnbmVkIHdyaXRlIGNvbW1hbmQsIGl0IGlzIG1vcmUgZWFzeSB0aGFuIHRvIHJlZ2lzdGVy
IGNhbGxiYWNrIHRvIA0KbGlzdGVuIHRoZSBTaWduZWRfY250IGNoYW5nZS4gQW5kIEkgZG9uJ3Qg
c2VlIGFueSBjYWxsYmFjayB0cmlnZ2VyIGNvbmRpdGlvbiBmb3IgdGhpcyANCnNpZ25lZF9jbnQg
cGFyYW1ldGVyIGluIHRoZSBidF9hdHRfc2VuZCBmdW5jdGlvbi4NCiANCkJlc3QgUmVnYXJkcw0K
Q2hhb2ppZSBHdQ0K

RE: [PATCH v3 2/4] shared/gatt-client: Add CSRK part to support signed write

[Gu, Chao Jie]

SGkgTHVrYXN6LA0KDQo+ID4+IEluIG15IG9waW5pb24gYnRfYXR0X2xvY2FsX2NzcmtfaXNfdmFs
aWQgQVBJIHNlZW1zIHRvIGJlIHVubmVjZXNzYXJ5DQo+ID4+IGFzIENTUksgY2FuIGJlIGNoZWNr
ZWQgaW5zaWRlIGJ0X2F0dF9zZW5kIGZ1bmN0aW9uIGRpcmVjdGx5Lg0KPiA+Pg0KPiA+DQo+ID4g
SSBhZ3JlZSB3aXRoIHlvdSB0aGF0IGl0IGlzIGFsc28gb2sgdG8gYmUgY2hlY2tlZCBpbnNpZGUg
YnRfYXR0X3NlbmQNCj4gPiBmdW5jdGlvbiBhbmQgaGF2ZSB0aG91Z2h0IGFib3V0IHRoaXMgcHJv
YmxlbSBiZWZvcmUuDQo+ID4gQW5kIEkgdGhpbmsgdGhlcmUgYXJlIHR3byBhZHZhbnRhZ2VzIG9m
IGNoZWNraW5nIHZhbGlkIG91dHNpZGUNCj4gPiBidF9hdHRfc2VuZCBmdW5jdGlvbg0KPiA+DQo+
ID4gMS4gcHJvdmlkZSBBUEkgdG8gdXNlciB0byBjaGVjayBDU1JLIHN0YXR1cyBpZiB0aGV5IHdh
bnQuDQo+IA0KPiBPaywgdGhlIHF1ZXN0aW9uIGlzIHdoeSB1c2VyIHdvdWxkIGxpa2UgIHRvIGRv
IGl0PyBNYXliZSBJJ20gbWlzc2luZyBzb21ldGhpbmcuDQo+IA0KPiA+IDIuIGlmIHRoZSBDU1JL
IGlzIGludmFsaWQgLCB3ZSBjYW4ganVzdCByZXR1cm4gZmFsc2UgcXVpY2tseSAsIHRoZXJlDQo+
ID4gaXMgbm8gbmVlZCB0byBnbyBvbiBjYWxsaW5nIGRvd24gYnRfYXR0X3NlbmQgdGhlbiByZXR1
cm4gZmFsc2UgYmFjay4gSXQgd2lsbCBiZQ0KPiBtb3JlIGVmZmljaWVudCBJIHRoaW5rLg0KPiAN
Cj4gSWYgIGJ0X2F0dF9zZW5kKCkgd2lsbCBjaGVjayB0aGUgQ1NSSyBvbiB0aGUgYmVnaW5uaW5n
IHRoZW4gdGhlcmUgaXMgbm8gZGlmZmVyZW5jZS4NCj4gDQoNClllcyAsIHdlIGNhbiBjaGVjayB0
aGUgQ1NSSyBpbiB0aGUgYmVnaW5uaW5nIG9mIHRoZSBidF9hdHRfc2VuZCBmdW5jdGlvbi4gSG93
ZXZlciwNCllvdSBjYW4gc2VlIGluIHRoZSBidF9hdHRfc2VuZCBmdW5jdGlvbiB0aGVyZSBpcyBj
bGVhciBhbmQgZ2VuZXJhbCBoYW5kbGUgY29tbWFuZA0KcHJvY2VzcyAsIEkgZG8gbm90IHRoaW5r
IGlzIGEgZ29vZCBzdHlsZSBoZXJlIHRvIHB1dCBhIHNwZWNpZmljIHNpZ25lZF93cml0ZV9jbWQg
c3R1ZmYgDQppbiB0aGUgYmVnaW5uaW5nIHRvIGNoZWNrIENTUksgdmFsaWQgb3Igbm90Lg0KDQpU
aGFua3MNCkNoYW9qaWUgR3UNCg==