[PATCH v5 0/3] support signed write command

[PATCH v5 0/3] support signed write command

[Gu Chaojie]

* This patch set give user dedicated command to set CSRK option and seperate from signed write command implementation in the previous patch
* Remove the valid_csrk flag, use struct signed_write_info make implementation more clear and simple
* signed_counter will initialize when CSRK be set, remove signed_counter initialization in bt_att_new procedure.

Gu Chaojie (3):
  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 with CSRK option

 src/shared/att-types.h   |   3 +
 src/shared/att.c         | 146 ++++++++++++++++++++++++++++++++++++++++++++++-
 src/shared/att.h         |  12 ++++
 src/shared/gatt-client.c |  21 +++++--
 src/shared/gatt-client.h |   4 ++
 tools/btgatt-client.c    |  69 +++++++++++++++++++++-
 6 files changed, 245 insertions(+), 10 deletions(-)

-- 
1.9.1

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

[Gu Chaojie]

---
 src/shared/att-types.h |   3 +
 src/shared/att.c       | 146 ++++++++++++++++++++++++++++++++++++++++++++++++-
 src/shared/att.h       |  12 ++++
 3 files changed, 158 insertions(+), 3 deletions(-)

diff --git a/src/shared/att-types.h b/src/shared/att-types.h
index a6b23e4..1bea1ef 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 6adde22..988eaff 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
@@ -44,6 +45,8 @@
 
 struct att_send_op;
 
+struct sign_write_info;
+
 struct bt_att {
 	int ref_count;
 	int fd;
@@ -79,6 +82,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;
+
+	struct sign_write_info *local_info;
+	struct sign_write_info *remote_info;
+};
+
+struct sign_write_info {
+	uint8_t csrk[16];
+	uint32_t sign_cnt;
 };
 
 enum att_op_type {
@@ -178,6 +191,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)
@@ -289,6 +304,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;
@@ -305,17 +324,36 @@ 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))
+		return true;
+
+	if (!att->local_info)
+		return false;
+
+	if (!(bt_crypto_sign_att(att->crypto,
+				att->local_info->csrk,
+				op->pdu,
+				1 + length,
+				att->local_info->sign_cnt,
+				&((uint8_t *) op->pdu)[1 + length])))
+		return false;
+
+	att->local_info->sign_cnt++;
+
 	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;
@@ -346,6 +384,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);
@@ -756,6 +795,8 @@ struct bt_att *bt_att_new(int fd)
 
 	att->fd = fd;
 
+	att->local_info = NULL;
+	att->remote_info = NULL;
 	att->mtu = BT_ATT_DEFAULT_LE_MTU;
 	att->buf = malloc(att->mtu);
 	if (!att->buf)
@@ -765,6 +806,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;
@@ -799,6 +844,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);
@@ -852,6 +898,16 @@ void bt_att_unref(struct bt_att *att)
 	if (att->debug_destroy)
 		att->debug_destroy(att->debug_data);
 
+	if (att->local_info) {
+		free(att->local_info);
+		att->local_info = NULL;
+	}
+
+	if (att->remote_info) {
+		free(att->remote_info);
+		att->remote_info = NULL;
+	}
+
 	free(att->buf);
 	att->buf = NULL;
 
@@ -995,7 +1051,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;
@@ -1178,3 +1234,87 @@ bool bt_att_unregister_all(struct bt_att *att)
 
 	return true;
 }
+
+bool bt_att_set_local_csrk(struct bt_att *att, uint32_t local_sign_cnt,
+						uint8_t local_key[16])
+{
+	struct sign_write_info *local_sign_info;
+
+	if (!att)
+		return false;
+
+	local_sign_info = att->local_info;
+
+	if (!local_sign_info) {
+		local_sign_info = new0(struct sign_write_info, 1);
+		if (!local_sign_info)
+			return false;
+		att->local_info = local_sign_info;
+	}
+
+	local_sign_info->sign_cnt = local_sign_cnt;
+	memcpy(local_sign_info->csrk, local_key, 16);
+
+	return true;
+}
+
+bool bt_att_set_remote_csrk(struct bt_att *att, uint32_t remote_sign_cnt,
+						uint8_t remote_key[16])
+{
+	struct sign_write_info *remote_sign_info;
+
+	if (!att)
+		return false;
+
+	remote_sign_info = att->remote_info;
+
+	if (!remote_sign_info) {
+		remote_sign_info = new0(struct sign_write_info, 1);
+		if (!remote_sign_info)
+			return false;
+		att->remote_info = remote_sign_info;
+	}
+
+	remote_sign_info->sign_cnt = remote_sign_cnt;
+	memcpy(remote_sign_info->csrk, remote_key, 16);
+
+	return true;
+}
+
+bool bt_att_get_local_csrk(struct bt_att *att, uint8_t local_key[16],
+					uint32_t *local_sign_cnt)
+{
+	struct sign_write_info *local_sign_info;
+
+	if (!att)
+		return false;
+
+	if (!att->local_info)
+		return false;
+
+	local_sign_info = att->local_info;
+
+	memcpy(local_key, local_sign_info->csrk, 16);
+	*local_sign_cnt = local_sign_info->sign_cnt;
+
+	return true;
+}
+
+bool bt_att_get_remote_csrk(struct bt_att *att, uint8_t remote_key[16],
+					uint32_t *remote_sign_cnt)
+{
+	struct sign_write_info *remote_sign_info;
+
+	if (!att)
+		return false;
+
+	if (!att->remote_info)
+		return false;
+
+	remote_sign_info = att->remote_info;
+
+	memcpy(remote_key, remote_sign_info->csrk, 16);
+	*remote_sign_cnt = remote_sign_info->sign_cnt;
+
+	return true;
+}
diff --git a/src/shared/att.h b/src/shared/att.h
index 1063021..d232d22 100644
--- a/src/shared/att.h
+++ b/src/shared/att.h
@@ -76,3 +76,15 @@ 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, uint32_t local_sign_cnt,
+						uint8_t local_key[16]);
+
+bool bt_att_set_remote_csrk(struct bt_att *att, uint32_t remote_sign_cnt,
+						uint8_t remote_key[16]);
+
+bool bt_att_get_local_csrk(struct bt_att *att, uint8_t local_key[16],
+					uint32_t *local_sign_cnt);
+
+bool bt_att_get_remote_csrk(struct bt_att *att, uint8_t remote_key[16],
+					uint32_t *remote_sign_cnt);
-- 
1.9.1

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

[Szymon Janc]

Hi Gu Chaojie,

commit prefix should be "shared/att: "

I'm not fully getting commit subject... maybe something like
"shared/att: Add support for signed write command"

Also please put some explanation what is being done in patch in commit body.

Note that I might have more comments once this is rebased.

On Friday 07 of November 2014 17:04:31 Gu Chaojie wrote:
> ---
>  src/shared/att-types.h |   3 +
>  src/shared/att.c       | 146 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  src/shared/att.h       |  12 ++++
>  3 files changed, 158 insertions(+), 3 deletions(-)
> 
> diff --git a/src/shared/att-types.h b/src/shared/att-types.h
> index a6b23e4..1bea1ef 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

I don't think this needs to be exposed so it could be defined in att.c

> +
>  /* 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 6adde22..988eaff 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
> @@ -44,6 +45,8 @@
>  
>  struct att_send_op;
>  
> +struct sign_write_info;
> +

This is not needed, just define this struct here.

>  struct bt_att {
>  	int ref_count;
>  	int fd;
> @@ -79,6 +82,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;
> +
> +	struct sign_write_info *local_info;
> +	struct sign_write_info *remote_info;
> +};
> +
> +struct sign_write_info {
> +	uint8_t csrk[16];
> +	uint32_t sign_cnt;
>  };
>  
>  enum att_op_type {
> @@ -178,6 +191,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)
> @@ -289,6 +304,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;
> @@ -305,17 +324,36 @@ 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))
> +		return true;
> +
> +	if (!att->local_info)
> +		return false;

You are leaking pdu here op->pdu here and after bt_crypto_sign_att() failure.

> +
> +	if (!(bt_crypto_sign_att(att->crypto,
> +				att->local_info->csrk,
> +				op->pdu,
> +				1 + length,
> +				att->local_info->sign_cnt,
> +				&((uint8_t *) op->pdu)[1 + length])))
> +		return false;

This should be formatted like:
	if (!(bt_crypto_sign_att(att->crypto, att->local_info->csrk, op->pdu,
					1 + length, att->local_info->sign_cnt,
					&((uint8_t *) op->pdu)[1 + length])))

> +
> +	att->local_info->sign_cnt++;
> +
>  	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;
> @@ -346,6 +384,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);
> @@ -756,6 +795,8 @@ struct bt_att *bt_att_new(int fd)
>  
>  	att->fd = fd;
>  
> +	att->local_info = NULL;
> +	att->remote_info = NULL;

This is not needed since att is allocated wit new0.

>  	att->mtu = BT_ATT_DEFAULT_LE_MTU;
>  	att->buf = malloc(att->mtu);
>  	if (!att->buf)
> @@ -765,6 +806,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;
> @@ -799,6 +844,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);
> @@ -852,6 +898,16 @@ void bt_att_unref(struct bt_att *att)
>  	if (att->debug_destroy)
>  		att->debug_destroy(att->debug_data);
>  
> +	if (att->local_info) {
> +		free(att->local_info);
> +		att->local_info = NULL;

Since we free att few lines later there is no need to set local_info to NULL.
(actually this should be fixed in all cases in this function)

> +	}
> +
> +	if (att->remote_info) {
> +		free(att->remote_info);
> +		att->remote_info = NULL;

Same here.

> +	}
> +
>  	free(att->buf);
>  	att->buf = NULL;
>  
> @@ -995,7 +1051,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;
> @@ -1178,3 +1234,87 @@ bool bt_att_unregister_all(struct bt_att *att)
>  
>  	return true;
>  }
> +
> +bool bt_att_set_local_csrk(struct bt_att *att, uint32_t local_sign_cnt,
> +						uint8_t local_key[16])

Name those sing_cnt and key.

> +{
> +	struct sign_write_info *local_sign_info;

I don't see how this local variable helps. Just use att->local_info

> +
> +	if (!att)
> +		return false;
> +
> +	local_sign_info = att->local_info;
> +
> +	if (!local_sign_info) {
> +		local_sign_info = new0(struct sign_write_info, 1);
> +		if (!local_sign_info)
> +			return false;
> +		att->local_info = local_sign_info;
> +	}
> +
> +	local_sign_info->sign_cnt = local_sign_cnt;
> +	memcpy(local_sign_info->csrk, local_key, 16);

Why so complicated?

	if (!att->local_info)
		att->local_info = new0(struct sign_write_info, 1);
	
	if (!att->local_info)
		return false;

	att->local_info->sign_cnt = local_sign_cnt;
	memcpy(att->local_info->csrk, local_key, 16);


There is also question if we should allow to change csrk ie. if info is already
set maybe this function should fail?


Same goes for remote_csrk variant.

> +
> +	return true;
> +}
> +
> +bool bt_att_set_remote_csrk(struct bt_att *att, uint32_t remote_sign_cnt,
> +						uint8_t remote_key[16])
> +{
> +	struct sign_write_info *remote_sign_info;
> +
> +	if (!att)
> +		return false;
> +
> +	remote_sign_info = att->remote_info;
> +
> +	if (!remote_sign_info) {
> +		remote_sign_info = new0(struct sign_write_info, 1);
> +		if (!remote_sign_info)
> +			return false;
> +		att->remote_info = remote_sign_info;
> +	}
> +
> +	remote_sign_info->sign_cnt = remote_sign_cnt;
> +	memcpy(remote_sign_info->csrk, remote_key, 16);
> +
> +	return true;
> +}
> +
> +bool bt_att_get_local_csrk(struct bt_att *att, uint8_t local_key[16],
> +					uint32_t *local_sign_cnt)

Since function name is self explaining just use key and sign_cnt for parameters
names. Also this should be indented more right. Same for remote_csrk variant.

> +{
> +	struct sign_write_info *local_sign_info;

I'm not sure how this local variable helps. Just use att->local_info directly.
Same for remote_csrk variant.

> +
> +	if (!att)
> +		return false;
> +
> +	if (!att->local_info)
> +		return false;
> +
> +	local_sign_info = att->local_info;
> +
> +	memcpy(local_key, local_sign_info->csrk, 16);
> +	*local_sign_cnt = local_sign_info->sign_cnt;
> +
> +	return true;
> +}
> +
> +bool bt_att_get_remote_csrk(struct bt_att *att, uint8_t remote_key[16],
> +					uint32_t *remote_sign_cnt)
> +{
> +	struct sign_write_info *remote_sign_info;
> +
> +	if (!att)
> +		return false;
> +
> +	if (!att->remote_info)
> +		return false;
> +
> +	remote_sign_info = att->remote_info;
> +
> +	memcpy(remote_key, remote_sign_info->csrk, 16);
> +	*remote_sign_cnt = remote_sign_info->sign_cnt;
> +
> +	return true;
> +}
> diff --git a/src/shared/att.h b/src/shared/att.h
> index 1063021..d232d22 100644
> --- a/src/shared/att.h
> +++ b/src/shared/att.h
> @@ -76,3 +76,15 @@ 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, uint32_t local_sign_cnt,
> +						uint8_t local_key[16]);
> +
> +bool bt_att_set_remote_csrk(struct bt_att *att, uint32_t remote_sign_cnt,
> +						uint8_t remote_key[16]);
> +
> +bool bt_att_get_local_csrk(struct bt_att *att, uint8_t local_key[16],
> +					uint32_t *local_sign_cnt);
> +
> +bool bt_att_get_remote_csrk(struct bt_att *att, uint8_t remote_key[16],
> +					uint32_t *remote_sign_cnt);

Indentation should be up to the right. 

-- 
Best regards, 
Szymon Janc

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

[Gu Chaojie]

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

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 8689368..0d406f5 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -1987,19 +1987,20 @@ 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)
+		return bt_att_send(client->att, BT_ATT_OP_SIGNED_WRITE_CMD,
+					pdu, sizeof(pdu), NULL, NULL, NULL);
+
 	return bt_att_send(client->att, BT_ATT_OP_WRITE_CMD, pdu, sizeof(pdu),
 							NULL, NULL, NULL);
 }
@@ -2480,3 +2481,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,
+				uint32_t local_sign_cnt,
+				uint8_t local_key[16])
+{
+	if (!client)
+		return false;
+
+	return bt_att_set_local_csrk(client->att, local_sign_cnt, local_key);
+}
diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h
index adccfc5..5429576 100644
--- a/src/shared/gatt-client.h
+++ b/src/shared/gatt-client.h
@@ -171,3 +171,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,
+				uint32_t local_sign_cnt,
+				uint8_t local_key[16]);
-- 
1.9.1

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

[Szymon Janc]

Hi Gu Chaojie,

There should be space after ":" in commit prefix.

On Friday 07 of November 2014 17:04:32 Gu Chaojie wrote:
> ---
>  src/shared/gatt-client.c | 21 ++++++++++++++++-----
>  src/shared/gatt-client.h |  4 ++++
>  2 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
> index 8689368..0d406f5 100644
> --- a/src/shared/gatt-client.c
> +++ b/src/shared/gatt-client.c
> @@ -1987,19 +1987,20 @@ 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)
> +{

Code style fixes should be in separate patch.

>  	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)
> +		return bt_att_send(client->att, BT_ATT_OP_SIGNED_WRITE_CMD,
> +					pdu, sizeof(pdu), NULL, NULL, NULL);
> +
>  	return bt_att_send(client->att, BT_ATT_OP_WRITE_CMD, pdu, sizeof(pdu),
>  							NULL, NULL, NULL);
>  }
> @@ -2480,3 +2481,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,
> +				uint32_t local_sign_cnt,
> +				uint8_t local_key[16])
> +{
> +	if (!client)
> +		return false;
> +
> +	return bt_att_set_local_csrk(client->att, local_sign_cnt, local_key);
> +}
> diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h
> index adccfc5..5429576 100644
> --- a/src/shared/gatt-client.h
> +++ b/src/shared/gatt-client.h
> @@ -171,3 +171,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,
> +				uint32_t local_sign_cnt,
> +				uint8_t local_key[16]);

just name those sign_cnt and key. 

-- 
Best regards, 
Szymon Janc

[PATCH v5 3/3] tools/btgatt-client:Add signed write with CSRK option

[Gu Chaojie]

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

diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
index 7a1204f..f175cf2 100644
--- a/tools/btgatt-client.c
+++ b/tools/btgatt-client.c
@@ -505,12 +505,14 @@ 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-s, --signed-write\tsigned write command\n"
 		"e.g.:\n"
 		"\twrite-value 0x0001 00 01 00\n");
 }
 
 static struct option write_value_options[] = {
 	{ "without-response",	0, 0, 'w' },
+	{ "signed-write",	0, 0, 's' },
 	{ }
 };
 
@@ -534,6 +536,7 @@ 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;
 
 	if (!bt_gatt_client_is_ready(cli->gatt)) {
 		printf("GATT client not initialized\n");
@@ -548,12 +551,15 @@ 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, "+ws", write_value_options,
 								NULL)) != -1) {
 		switch (opt) {
 		case 'w':
 			without_response = true;
 			break;
+		case 's':
+			signed_write = true;
+			break;
 		default:
 			write_value_usage();
 			return;
@@ -607,7 +613,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;
@@ -858,6 +864,63 @@ static void cmd_unregister_notify(struct client *cli, char *cmd_str)
 	printf("Unregistered notify handler with id: %u\n", id);
 }
 
+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 set_csrk_usage(void)
+{
+	printf("Usage: set-csrk [options]\nOptions:\n"
+		"\t -c, --csrk <csrk>\tCSRK\n"
+		"e.g.:\n"
+		"\tset-csrk -c D8515948451FEA320DC05A2E88308188\n");
+}
+
+static void cmd_set_csrk(struct client *cli, char *cmd_str)
+{
+	char *argv[3];
+	int argc = 0;
+	uint8_t csrk[16];
+
+	memset(csrk, 0, 16);
+
+	if (!bt_gatt_client_is_ready(cli->gatt)) {
+		printf("GATT client not initialized\n");
+		return;
+	}
+
+	if (!parse_args(cmd_str, 2, argv, &argc)) {
+		set_csrk_usage();
+		return;
+	}
+
+	if (argc != 2) {
+		set_csrk_usage();
+		return;
+	}
+
+	if (!strcmp(argv[0], "-c") || !strcmp(argv[0], "--csrk")) {
+		if (convert_csrk_key(argv[1], csrk))
+			bt_gatt_client_set_local_csrk(cli->gatt, 0, csrk);
+
+	} else
+		set_csrk_usage();
+}
+
 static void cmd_help(struct client *cli, char *cmd_str);
 
 typedef void (*command_func_t)(struct client *cli, char *cmd_str);
@@ -881,6 +944,8 @@ static struct {
 			"\tSubscribe to not/ind from a characteristic" },
 	{ "unregister-notify", cmd_unregister_notify,
 						"Unregister a not/ind session"},
+	{ "set-csrk", cmd_set_csrk,
+			"\tSet CSRK for signed write command"},
 	{ }
 };
 
-- 
1.9.1

Re: [PATCH v5 3/3] tools/btgatt-client:Add signed write with CSRK option

[Szymon Janc]

Hi Gu Chaojie,

On Friday 07 of November 2014 17:04:33 Gu Chaojie wrote:
> ---
>  tools/btgatt-client.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 67 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
> index 7a1204f..f175cf2 100644
> --- a/tools/btgatt-client.c
> +++ b/tools/btgatt-client.c
> @@ -505,12 +505,14 @@ 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-s, --signed-write\tsigned write command\n"

Keep same capitalization style.

>  		"e.g.:\n"
>  		"\twrite-value 0x0001 00 01 00\n");
>  }
>  
>  static struct option write_value_options[] = {
>  	{ "without-response",	0, 0, 'w' },
> +	{ "signed-write",	0, 0, 's' },
>  	{ }
>  };
>  
> @@ -534,6 +536,7 @@ 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;
>  
>  	if (!bt_gatt_client_is_ready(cli->gatt)) {
>  		printf("GATT client not initialized\n");
> @@ -548,12 +551,15 @@ 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, "+ws", write_value_options,
>  								NULL)) != -1) {
>  		switch (opt) {
>  		case 'w':
>  			without_response = true;
>  			break;
> +		case 's':
> +			signed_write = true;
> +			break;
>  		default:
>  			write_value_usage();
>  			return;
> @@ -607,7 +613,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;
> @@ -858,6 +864,63 @@ static void cmd_unregister_notify(struct client *cli, char *cmd_str)
>  	printf("Unregistered notify handler with id: %u\n", id);
>  }
>  
> +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);
> +		}
> +	}

Else is not needed since you return in if.

> +
> +	return true;
> +}
> +
> +static void set_csrk_usage(void)
> +{
> +	printf("Usage: set-csrk [options]\nOptions:\n"
> +		"\t -c, --csrk <csrk>\tCSRK\n"
> +		"e.g.:\n"
> +		"\tset-csrk -c D8515948451FEA320DC05A2E88308188\n");
> +}
> +
> +static void cmd_set_csrk(struct client *cli, char *cmd_str)
> +{
> +	char *argv[3];
> +	int argc = 0;
> +	uint8_t csrk[16];
> +
> +	memset(csrk, 0, 16);

This memset is not really needed.

> +
> +	if (!bt_gatt_client_is_ready(cli->gatt)) {
> +		printf("GATT client not initialized\n");
> +		return;
> +	}
> +
> +	if (!parse_args(cmd_str, 2, argv, &argc)) {
> +		set_csrk_usage();
> +		return;
> +	}
> +
> +	if (argc != 2) {
> +		set_csrk_usage();
> +		return;
> +	}
> +
> +	if (!strcmp(argv[0], "-c") || !strcmp(argv[0], "--csrk")) {

csrk[] could be local to this scope.

> +		if (convert_csrk_key(argv[1], csrk))
> +			bt_gatt_client_set_local_csrk(cli->gatt, 0, csrk);
> +
> +	} else
> +		set_csrk_usage();

Braces should be present on both if-else if one of then needs it.
Please refer to doc/coding-style.txt.

> +}
> +
>  static void cmd_help(struct client *cli, char *cmd_str);
>  
>  typedef void (*command_func_t)(struct client *cli, char *cmd_str);
> @@ -881,6 +944,8 @@ static struct {
>  			"\tSubscribe to not/ind from a characteristic" },
>  	{ "unregister-notify", cmd_unregister_notify,
>  						"Unregister a not/ind session"},
> +	{ "set-csrk", cmd_set_csrk,
> +			"\tSet CSRK for signed write command"},
>  	{ }
>  };
>  
> 

-- 
Best regards, 
Szymon Janc

Re: [PATCH v5 0/3] support signed write command

[Arman Uguray]

Hi all,

> On Fri, Nov 7, 2014 at 1:04 AM, Gu Chaojie <chao.jie.gu@intel.com> wrote:
> * This patch set give user dedicated command to set CSRK option and seperate from signed write command implementation in the previous patch
> * Remove the valid_csrk flag, use struct signed_write_info make implementation more clear and simple
> * signed_counter will initialize when CSRK be set, remove signed_counter initialization in bt_att_new procedure.
>
> Gu Chaojie (3):
>   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 with CSRK option
>
>  src/shared/att-types.h   |   3 +
>  src/shared/att.c         | 146 ++++++++++++++++++++++++++++++++++++++++++++++-
>  src/shared/att.h         |  12 ++++
>  src/shared/gatt-client.c |  21 +++++--
>  src/shared/gatt-client.h |   4 ++
>  tools/btgatt-client.c    |  69 +++++++++++++++++++++-
>  6 files changed, 245 insertions(+), 10 deletions(-)
>
> --
> 1.9.1
>
> --
> 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

Sending a gentle ping on these patches, since they haven't received a
lot of attention for a couple of months. What's the status of Signed
Writes here? These will be more relevant for Android now than they
will initially be for desktop.

Thanks,
Arman

Re: [PATCH v5 0/3] support signed write command

[Luiz Augusto von Dentz]

Hi Gu,

On Mon, Dec 15, 2014 at 6:41 PM, Arman Uguray <armansito@chromium.org> wrote:
> Hi all,
>
>> On Fri, Nov 7, 2014 at 1:04 AM, Gu Chaojie <chao.jie.gu@intel.com> wrote:
>> * This patch set give user dedicated command to set CSRK option and seperate from signed write command implementation in the previous patch
>> * Remove the valid_csrk flag, use struct signed_write_info make implementation more clear and simple
>> * signed_counter will initialize when CSRK be set, remove signed_counter initialization in bt_att_new procedure.
>>
>> Gu Chaojie (3):
>>   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 with CSRK option
>>
>>  src/shared/att-types.h   |   3 +
>>  src/shared/att.c         | 146 ++++++++++++++++++++++++++++++++++++++++++++++-
>>  src/shared/att.h         |  12 ++++
>>  src/shared/gatt-client.c |  21 +++++--
>>  src/shared/gatt-client.h |   4 ++
>>  tools/btgatt-client.c    |  69 +++++++++++++++++++++-
>>  6 files changed, 245 insertions(+), 10 deletions(-)
>>
>> --
>> 1.9.1
>>
>> --
>> 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
>
> Sending a gentle ping on these patches, since they haven't received a
> lot of attention for a couple of months. What's the status of Signed
> Writes here? These will be more relevant for Android now than they
> will initially be for desktop.

Do you have any new patches addressing the comments from Szymon?


-- 
Luiz Augusto von Dentz

RE: [PATCH v5 0/3] support signed write command

[Gu, Chao Jie]

SGkgTHVpeiwNCglUaGlzIHBhdGNoIGhhcyBiZWVuIHN1Ym1pdHRlZCBmb3IgYSBsb25nIHRpbWUu
IFdoZW4gSSB3cml0aW5nIHRoZSBzaWduZWQgd3JpdGUgZm9yIEdBVFQgQ2xpZW50LCB0aGUgR0FU
VCBTZXJ2ZXIgaXMgbm90IHJlYWR5IHlldC4gU28gdGhpcyBwYXRjaCBpcyBqdXN0IGZvciBHQVRU
IENsaWVudCBzaWduZWQgY29tbWFuZC4gSSBhbSBub3Qgc3VyZSBBcm1hbiBpbXBsZW1lbnRlZCBT
aWduZWQgd3JpdGUgY29tbWFuZCBpbiBHQVRUIFNlcnZlciBvciBub3QuDQoNClRoYW5rcw0KQ2hh
b2ppZQ0KDQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IEx1aXogQXVndXN0
byB2b24gRGVudHogW21haWx0bzpsdWl6LmRlbnR6QGdtYWlsLmNvbV0NCj4gU2VudDogVHVlc2Rh
eSwgRmVicnVhcnkgMTcsIDIwMTUgOToyNCBQTQ0KPiBUbzogQXJtYW4gVWd1cmF5DQo+IENjOiBH
dSwgQ2hhbyBKaWU7IEJsdWVaIGRldmVsb3BtZW50DQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggdjUg
MC8zXSBzdXBwb3J0IHNpZ25lZCB3cml0ZSBjb21tYW5kDQo+IA0KPiBIaSBHdSwNCj4gDQo+IE9u
IE1vbiwgRGVjIDE1LCAyMDE0IGF0IDY6NDEgUE0sIEFybWFuIFVndXJheSA8YXJtYW5zaXRvQGNo
cm9taXVtLm9yZz4gd3JvdGU6DQo+ID4gSGkgYWxsLA0KPiA+DQo+ID4+IE9uIEZyaSwgTm92IDcs
IDIwMTQgYXQgMTowNCBBTSwgR3UgQ2hhb2ppZSA8Y2hhby5qaWUuZ3VAaW50ZWwuY29tPiB3cm90
ZToNCj4gPj4gKiBUaGlzIHBhdGNoIHNldCBnaXZlIHVzZXIgZGVkaWNhdGVkIGNvbW1hbmQgdG8g
c2V0IENTUksgb3B0aW9uIGFuZA0KPiA+PiBzZXBlcmF0ZSBmcm9tIHNpZ25lZCB3cml0ZSBjb21t
YW5kIGltcGxlbWVudGF0aW9uIGluIHRoZSBwcmV2aW91cw0KPiA+PiBwYXRjaA0KPiA+PiAqIFJl
bW92ZSB0aGUgdmFsaWRfY3NyayBmbGFnLCB1c2Ugc3RydWN0IHNpZ25lZF93cml0ZV9pbmZvIG1h
a2UNCj4gPj4gaW1wbGVtZW50YXRpb24gbW9yZSBjbGVhciBhbmQgc2ltcGxlDQo+ID4+ICogc2ln
bmVkX2NvdW50ZXIgd2lsbCBpbml0aWFsaXplIHdoZW4gQ1NSSyBiZSBzZXQsIHJlbW92ZSBzaWdu
ZWRfY291bnRlcg0KPiBpbml0aWFsaXphdGlvbiBpbiBidF9hdHRfbmV3IHByb2NlZHVyZS4NCj4g
Pj4NCj4gPj4gR3UgQ2hhb2ppZSAoMyk6DQo+ID4+ICAgc2hhcmVkL2F0dC5jOkFkZCBzaWduZWQg
Y29tbWFuZCBvdXRnb2luZyBhbmQgQ1NSSyBmdW5jdGlvbg0KPiA+PiAgIHNoYXJlZC9nYXR0LWNs
aWVudDpBZGQgQ1NSSyBwYXJ0IHRvIHN1cHBvcnQgc2lnbmVkIHdyaXRlDQo+ID4+ICAgdG9vbHMv
YnRnYXR0LWNsaWVudDpBZGQgc2lnbmVkIHdyaXRlIHdpdGggQ1NSSyBvcHRpb24NCj4gPj4NCj4g
Pj4gIHNyYy9zaGFyZWQvYXR0LXR5cGVzLmggICB8ICAgMyArDQo+ID4+ICBzcmMvc2hhcmVkL2F0
dC5jICAgICAgICAgfCAxNDYNCj4gKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysr
KysrKysrKysrKy0NCj4gPj4gIHNyYy9zaGFyZWQvYXR0LmggICAgICAgICB8ICAxMiArKysrDQo+
ID4+ICBzcmMvc2hhcmVkL2dhdHQtY2xpZW50LmMgfCAgMjEgKysrKystLQ0KPiA+PiAgc3JjL3No
YXJlZC9nYXR0LWNsaWVudC5oIHwgICA0ICsrDQo+ID4+ICB0b29scy9idGdhdHQtY2xpZW50LmMg
ICAgfCAgNjkgKysrKysrKysrKysrKysrKysrKysrLQ0KPiA+PiAgNiBmaWxlcyBjaGFuZ2VkLCAy
NDUgaW5zZXJ0aW9ucygrKSwgMTAgZGVsZXRpb25zKC0pDQo+ID4+DQo+ID4+IC0tDQo+ID4+IDEu
OS4xDQo+ID4+DQo+ID4+IC0tDQo+ID4+IFRvIHVuc3Vic2NyaWJlIGZyb20gdGhpcyBsaXN0OiBz
ZW5kIHRoZSBsaW5lICJ1bnN1YnNjcmliZQ0KPiA+PiBsaW51eC1ibHVldG9vdGgiIGluIHRoZSBi
b2R5IG9mIGEgbWVzc2FnZSB0bw0KPiA+PiBtYWpvcmRvbW9Admdlci5rZXJuZWwub3JnIE1vcmUg
bWFqb3Jkb21vIGluZm8gYXQNCj4gPj4gaHR0cDovL3ZnZXIua2VybmVsLm9yZy9tYWpvcmRvbW8t
aW5mby5odG1sDQo+ID4NCj4gPiBTZW5kaW5nIGEgZ2VudGxlIHBpbmcgb24gdGhlc2UgcGF0Y2hl
cywgc2luY2UgdGhleSBoYXZlbid0IHJlY2VpdmVkIGENCj4gPiBsb3Qgb2YgYXR0ZW50aW9uIGZv
ciBhIGNvdXBsZSBvZiBtb250aHMuIFdoYXQncyB0aGUgc3RhdHVzIG9mIFNpZ25lZA0KPiA+IFdy
aXRlcyBoZXJlPyBUaGVzZSB3aWxsIGJlIG1vcmUgcmVsZXZhbnQgZm9yIEFuZHJvaWQgbm93IHRo
YW4gdGhleQ0KPiA+IHdpbGwgaW5pdGlhbGx5IGJlIGZvciBkZXNrdG9wLg0KPiANCj4gRG8geW91
IGhhdmUgYW55IG5ldyBwYXRjaGVzIGFkZHJlc3NpbmcgdGhlIGNvbW1lbnRzIGZyb20gU3p5bW9u
Pw0KPiANCj4gDQo+IC0tDQo+IEx1aXogQXVndXN0byB2b24gRGVudHoNCg==

Re: [PATCH v5 0/3] support signed write command

[Arman Uguray]

Hi Chaojie,

> On Thu, Feb 19, 2015 at 7:33 PM, Gu, Chao Jie <chao.jie.gu@intel.com> wrote:
> Hi Luiz,
>         This patch has been submitted for a long time. When I writing the signed write for GATT Client, the GATT Server is not ready yet. So this patch is just for GATT Client signed command. I am not sure Arman implemented Signed write command in GATT Server or not.
>
> Thanks
> Chaojie
>

Please don't top-post on your responses to the list.

I believe when we originally discussed the signed write support we
decided that bt_att needs to handle the signing and CSRK validation.
So as long as that support is not in place, bt_gatt_server can't
support signed writes either.

>> -----Original Message-----
>> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
>> Sent: Tuesday, February 17, 2015 9:24 PM
>> To: Arman Uguray
>> Cc: Gu, Chao Jie; BlueZ development
>> Subject: Re: [PATCH v5 0/3] support signed write command
>>
>> Hi Gu,
>>
>> On Mon, Dec 15, 2014 at 6:41 PM, Arman Uguray <armansito@chromium.org> wrote:
>> > Hi all,
>> >
>> >> On Fri, Nov 7, 2014 at 1:04 AM, Gu Chaojie <chao.jie.gu@intel.com> wrote:
>> >> * This patch set give user dedicated command to set CSRK option and
>> >> seperate from signed write command implementation in the previous
>> >> patch
>> >> * Remove the valid_csrk flag, use struct signed_write_info make
>> >> implementation more clear and simple
>> >> * signed_counter will initialize when CSRK be set, remove signed_counter
>> initialization in bt_att_new procedure.
>> >>
>> >> Gu Chaojie (3):
>> >>   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 with CSRK option
>> >>
>> >>  src/shared/att-types.h   |   3 +
>> >>  src/shared/att.c         | 146
>> ++++++++++++++++++++++++++++++++++++++++++++++-
>> >>  src/shared/att.h         |  12 ++++
>> >>  src/shared/gatt-client.c |  21 +++++--
>> >>  src/shared/gatt-client.h |   4 ++
>> >>  tools/btgatt-client.c    |  69 +++++++++++++++++++++-
>> >>  6 files changed, 245 insertions(+), 10 deletions(-)
>> >>
>> >> --
>> >> 1.9.1
>> >>
>> >> --
>> >> 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
>> >
>> > Sending a gentle ping on these patches, since they haven't received a
>> > lot of attention for a couple of months. What's the status of Signed
>> > Writes here? These will be more relevant for Android now than they
>> > will initially be for desktop.
>>
>> Do you have any new patches addressing the comments from Szymon?
>>
>>
>> --
>> Luiz Augusto von Dentz

Arman

[PATCH v5 0/3] support signed write command

[Gu Chaojie]

*This patch set give user dedicated command to set CSRK option and seperate from
 signed write command implementation in previous patchset.
*Remove the valid_csrk flag, use struct signed_write_info make implementaion more
 clear and simple.
*signed_counter will initialize automatically when CSRK setup, remove initialization
 in bt_att_new procedure. 

Gu Chaojie (3):
  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 with CSRK option

 src/shared/att-types.h   |   3 +
 src/shared/att.c         | 146 ++++++++++++++++++++++++++++++++++++++++++++++-
 src/shared/att.h         |  12 ++++
 src/shared/gatt-client.c |  21 +++++--
 src/shared/gatt-client.h |   4 ++
 tools/btgatt-client.c    |  69 +++++++++++++++++++++-
 6 files changed, 245 insertions(+), 10 deletions(-)

-- 
1.9.1

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

[Gu Chaojie]

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

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 8689368..0d406f5 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -1987,19 +1987,20 @@ 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)
+		return bt_att_send(client->att, BT_ATT_OP_SIGNED_WRITE_CMD,
+					pdu, sizeof(pdu), NULL, NULL, NULL);
+
 	return bt_att_send(client->att, BT_ATT_OP_WRITE_CMD, pdu, sizeof(pdu),
 							NULL, NULL, NULL);
 }
@@ -2480,3 +2481,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,
+				uint32_t local_sign_cnt,
+				uint8_t local_key[16])
+{
+	if (!client)
+		return false;
+
+	return bt_att_set_local_csrk(client->att, local_sign_cnt, local_key);
+}
diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h
index adccfc5..5429576 100644
--- a/src/shared/gatt-client.h
+++ b/src/shared/gatt-client.h
@@ -171,3 +171,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,
+				uint32_t local_sign_cnt,
+				uint8_t local_key[16]);
-- 
1.9.1