Add bt_att_set_csrk for btgatt-client tool. However this patch hook into
shared/mgmt, so btgatt-client now should be running by the root user.
---
src/shared/att-types.h | 3 ++
src/shared/att.c | 49 ++++++++++++++++--
src/shared/att.h | 2 +
src/shared/gatt-client.c | 11 ++--
tools/btgatt-client.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 186 insertions(+), 8 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..aadef9e 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,11 @@ struct bt_att {
bt_att_debug_func_t debug_callback;
bt_att_destroy_func_t debug_destroy;
void *debug_data;
+
+ struct bt_crypto *crypto;
+
+ uint8_t local_csrk[16];
+ uint32_t local_sign_cnt;
};
enum att_op_type {
@@ -176,6 +182,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 +285,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 +305,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 +361,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);
@@ -707,6 +735,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 +773,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 +964,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 +1149,13 @@ bool bt_att_unregister_all(struct bt_att *att)
return true;
}
+
+void bt_att_set_csrk(struct bt_att *att,
+ const uint8_t *val,
+ uint32_t local_sign_cnt)
+{
+ memcpy(att->local_csrk, val, 16);
+ att->local_sign_cnt = local_sign_cnt;
+}
+
+
diff --git a/src/shared/att.h b/src/shared/att.h
index 1063021..b76e5f5 100644
--- a/src/shared/att.h
+++ b/src/shared/att.h
@@ -76,3 +76,5 @@ 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);
+
+void bt_att_set_csrk(struct bt_att *att, const uint8_t *val, uint32_t local_sign_cnt);
diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 1a157ec..b1619c6 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -973,19 +973,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);
}
diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
index d1395b2..fdf8485 100644
--- a/tools/btgatt-client.c
+++ b/tools/btgatt-client.c
@@ -38,11 +38,14 @@
#include <bluetooth/hci_lib.h>
#include <bluetooth/l2cap.h>
#include "lib/uuid.h"
+#include "lib/bluetooth.h"
+#include "lib/mgmt.h"
#include "monitor/mainloop.h"
#include "src/shared/util.h"
#include "src/shared/att.h"
#include "src/shared/gatt-client.h"
+#include "src/shared/mgmt.h"
#define ATT_CID 4
@@ -59,6 +62,8 @@
#define COLOR_BOLDWHITE "\x1B[1;37m"
static bool verbose = false;
+static struct mgmt *mgmt_if = NULL;
+static uint16_t adapter_index = MGMT_INDEX_NONE;
struct client {
int fd;
@@ -92,6 +97,115 @@ static void gatt_debug_cb(const char *str, void *user_data)
PRLOG(COLOR_GREEN "%s%s\n" COLOR_OFF, prefix, str);
}
+static void new_csrk_callback(uint16_t index, uint16_t length,
+ const void *param, void *user_data)
+{
+ const struct mgmt_ev_new_csrk *ev = param;
+ struct bt_att *att = user_data;
+
+ if (length < sizeof(*ev)) {
+ fprintf(stderr, "Too small csrk event (%u bytes)\n", length);
+ return;
+ }
+
+ switch (ev->key.master) {
+ case 0x00:
+ bt_att_set_csrk(att, ev->key.val, 0);
+ break;
+ case 0x01:
+ break;
+ default:
+ fprintf(stderr,
+ "Unknown CSRK key type 02%02x\n", ev->key.master);
+ return;
+ }
+}
+
+static void mgmt_index_added_event(uint16_t index, uint16_t length,
+ const void *param, void *user_data)
+{
+ struct bt_att *att = user_data;
+
+ printf("index %u\n", index);
+
+ if (adapter_index != MGMT_INDEX_NONE) {
+ fprintf(stderr, "skip event for index %u\n", index);
+ return;
+ }
+
+ adapter_index = index;
+
+ mgmt_register(mgmt_if, MGMT_EV_NEW_CSRK, adapter_index,
+ new_csrk_callback, att, NULL);
+}
+
+static void mgmt_index_removed_event(uint16_t index, uint16_t length,
+ const void *param, void *user_data)
+{
+ if (index != adapter_index)
+ return;
+
+ perror("Adapter was removed. Exiting.\n");
+ raise(SIGTERM);
+}
+
+static void read_version_complete(uint8_t status, uint16_t length,
+ const void *param, void *user_data)
+{
+ const struct mgmt_rp_read_version *rp = param;
+ uint8_t mgmt_version, mgmt_revision;
+ struct bt_att *att = user_data;
+
+ if (status) {
+ fprintf(stderr,
+ "Failed to read version information: %s (0x%02x)\n",
+ mgmt_errstr(status), status);
+ return;
+ }
+
+ if (length < sizeof(*rp)) {
+ perror("Wrong size response\n");
+ return;
+ }
+
+ mgmt_version = rp->version;
+ mgmt_revision = btohs(rp->revision);
+
+ printf("Bluetooth management interface %u.%u initialized\n",
+ mgmt_version, mgmt_revision);
+
+ if (MGMT_VERSION(mgmt_version, mgmt_revision) < MGMT_VERSION(1, 3)) {
+ perror("Version 1.3 later of management interface required\n");
+ return;
+ }
+
+ mgmt_register(mgmt_if, MGMT_EV_INDEX_ADDED, MGMT_INDEX_NONE,
+ mgmt_index_added_event, att, NULL);
+ mgmt_register(mgmt_if, MGMT_EV_INDEX_REMOVED, MGMT_INDEX_NONE,
+ mgmt_index_removed_event, NULL, NULL);
+}
+
+static bool mgmt_create(struct bt_att *att)
+{
+ mgmt_if = mgmt_new_default();
+ if (!mgmt_if) {
+ perror("Failed to access management interface\n");
+ return false;
+ }
+
+ if (mgmt_send(mgmt_if, MGMT_OP_READ_VERSION, MGMT_INDEX_NONE, 0, NULL,
+ read_version_complete, att, NULL) == 0) {
+ perror("Error sending READ_VERSION mgmt command\n");
+
+ mgmt_unref(mgmt_if);
+ mgmt_if = NULL;
+
+ return false;
+ }
+
+ return true;
+}
+
static void ready_cb(bool success, uint8_t att_ecode, void *user_data);
static struct client *client_create(int fd, uint16_t mtu)
@@ -136,6 +250,13 @@ static struct client *client_create(int fd, uint16_t mtu)
return NULL;
}
+ if (!mgmt_create(att)) {
+ fprintf(stderr, "Failed to create management interface\n");
+ bt_att_unref(att);
+ free(cli);
+ return NULL;
+ }
+
if (verbose) {
bt_att_set_debug(att, att_debug_cb, "att: ", NULL);
bt_gatt_client_set_debug(cli->gatt, gatt_debug_cb, "gatt: ",
@@ -155,6 +276,12 @@ static void client_destroy(struct client *cli)
bt_gatt_client_unref(cli->gatt);
}
+static void mgmt_destroy(struct mgmt *mgmt_if)
+{
+ mgmt_unref(mgmt_if);
+ mgmt_if = NULL;
+}
+
static void print_uuid(const uint8_t uuid[16])
{
char uuid_str[MAX_LEN_UUID_STR];
@@ -1029,5 +1156,7 @@ int main(int argc, char *argv[])
client_destroy(cli);
+ mgmt_destroy(mgmt_if);
+
return EXIT_SUCCESS;
}
--
1.7.10.4
Hi Marcel,
> >>> Add bt_att_set_csrk for btgatt-client tool. However this patch hook
> >>> into shared/mgmt, so btgatt-client now should be running by the root =
user.
> >>
> >> I guess this could be made optional.
> >
> > You said this could be made optional , what's you point ? I am not
> > very clear about That.
>=20
> just provide a command line option to btgatt-client to provide the CSRK t=
o be used.
> There is no need to attach to mgmt since that is only seeing keys during =
pairing
> anyway.
>
You means that let user get CSRK from DEVICE_FILE which stored CSRK value w=
hen pairing,
and put this CSRK into command line option by himself , right?
=20
> >>> +void bt_att_set_csrk(struct bt_att *att,
> >>> + const uint8_t *val,
> >>> + uint32_t local_sign_cnt)
> >>
> >> This should allow to set local and/or remote csrk. Either we have two
> >> functions or flag identifying which key is provided.
> >
> > This is good idea. But this patch for client side, there is no need to
> > use remote_csrk, So would it be better to add remote csrk part when we =
need
> this?
>=20
> I want to function. One to set the local CSRK and one for setting the rem=
ote CSRK.
> However I have no idea why we are bothering with the sign counter. Just r=
eset it
> when creating bt_att.
>=20
That's fine . I will implement it.
Best Regards
Chaojie Gu
> >>> Add bt_att_set_csrk for btgatt-client tool. However this patch hook
> >>> into shared/mgmt, so btgatt-client now should be running by the root user.
> >>
> >> I guess this could be made optional.
> >
> > You said this could be made optional , what's you point ? I am not
> > very clear about That.
>
> just provide a command line option to btgatt-client to provide the CSRK to be used.
> There is no need to attach to mgmt since that is only seeing keys during pairing
> anyway.
You means that let user get CSRK from DEVICE_FILE which stored CSRK value
when pairing, and put this CSRK into command line option by himself , right?
> >>> +
> >>> +void bt_att_set_csrk(struct bt_att *att,
> >>> + const uint8_t *val,
> >>> + uint32_t local_sign_cnt)
> >>
> >> This should allow to set local and/or remote csrk. Either we have two
> >> functions or flag identifying which key is provided.
> >
> > This is good idea. But this patch for client side, there is no need to
> > use remote_csrk, So would it be better to add remote csrk part when we need
> this?
>
> I want to function. One to set the local CSRK and one for setting the remote CSRK.
> However I have no idea why we are bothering with the sign counter. Just reset it
> when creating bt_att.
>
That's fine . I will implement it.
Best Regards
Chaojie Gu
Hi Marcel,
On Mon, Sep 22, 2014 at 11:06 AM, Marcel Holtmann <[email protected]> wrote:
> Hi Chao Jie,
>
>>>> Add bt_att_set_csrk for btgatt-client tool. However this patch hook
>>>> into shared/mgmt, so btgatt-client now should be running by the root user.
>>>
>>> I guess this could be made optional.
>>
>> You said this could be made optional , what's you point ? I am not very clear about
>> That.
>
> just provide a command line option to btgatt-client to provide the CSRK to be used. There is no need to attach to mgmt since that is only seeing keys during pairing anyway.
>
>>
>>>> ---
>>>> src/shared/att-types.h | 3 ++
>>>> src/shared/att.c | 49 ++++++++++++++++--
>>>> src/shared/att.h | 2 +
>>>> src/shared/gatt-client.c | 11 ++--
>>>> tools/btgatt-client.c | 129
>>>> ++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 186
>>>> insertions(+), 8 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..aadef9e 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,11 @@ struct bt_att {
>>>> bt_att_debug_func_t debug_callback;
>>>> bt_att_destroy_func_t debug_destroy;
>>>> void *debug_data;
>>>> +
>>>> + struct bt_crypto *crypto;
>>>> +
>>>> + uint8_t local_csrk[16];
>>>> + uint32_t local_sign_cnt;
>>>> };
>>>>
>>>> enum att_op_type {
>>>> @@ -176,6 +182,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 +285,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 +305,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) {
>>>
>>> This should be used only if CSRK was provided. So we should have local_csrk_valid or
>>> similar flag (similar to what we currently have in android code). Check should be
>>> either here or we could also bail out sooner in
>>> bt_gatt_client_write_without_response().
>>
>> I think if we add loca_csrk_valid flag, it would be better to put in here. Since csrk
>> Information in the bt_att structure, it should be hidden the details from upper layer.
>>
>>>
>>>> + 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 +361,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);
>>>> @@ -707,6 +735,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 +773,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 +964,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 +1149,13 @@ bool bt_att_unregister_all(struct bt_att *att)
>>>>
>>>> return true;
>>>> }
>>>> +
>>>> +void bt_att_set_csrk(struct bt_att *att,
>>>> + const uint8_t *val,
>>>> + uint32_t local_sign_cnt)
>>>
>>> This should allow to set local and/or remote csrk. Either we have two functions or
>>> flag identifying which key is provided.
>>
>> This is good idea. But this patch for client side, there is no need to use remote_csrk,
>> So would it be better to add remote csrk part when we need this?
>
> I want to function. One to set the local CSRK and one for setting the remote CSRK. However I have no idea why we are bothering with the sign counter. Just reset it when creating bt_att.
Eventually we need signed counter to be stored. Spec 4.1 Part H, 2.4.5
Signing Algorithm says:
" The SignCounter shall be initialized to zero when CSRK is generated and
incremented for every message that is signed with a given CSRK."
" The device performing verification should store the last verified
SignCounter in
the security database and compare it with a received SignCounter to prevent
replay attacks. If the received SignCounter is greater than the stored value
then the message has not been seen by the local device before and the security
database can be updated."
As far as I understand it is valid as long as given CSRK is used.
>
>>
>>> Also there should be a way to notify user of bt_att about incremented values of
>>> counters.
>>
>> This suggestion is also the same with previous one. I have thought about add
>> function like bt_att_get_csrk for user. However , for btgatt-client tool, there is no
>> need to use it right now, so I didn't add it.
>
> So unless it needs to be remember persistent across connects, then I would not bother with the sign counter at all. If it needs to be remembered, then yes, we something to tell the owner of bt_att that it got updated.
>
> Regards
>
> Marcel
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
\Lukasz
Hi Chao Jie,
>>> Add bt_att_set_csrk for btgatt-client tool. However this patch hook
>>> into shared/mgmt, so btgatt-client now should be running by the root user.
>>
>> I guess this could be made optional.
>
> You said this could be made optional , what's you point ? I am not very clear about
> That.
just provide a command line option to btgatt-client to provide the CSRK to be used. There is no need to attach to mgmt since that is only seeing keys during pairing anyway.
>
>>> ---
>>> src/shared/att-types.h | 3 ++
>>> src/shared/att.c | 49 ++++++++++++++++--
>>> src/shared/att.h | 2 +
>>> src/shared/gatt-client.c | 11 ++--
>>> tools/btgatt-client.c | 129
>>> ++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 186
>>> insertions(+), 8 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..aadef9e 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,11 @@ struct bt_att {
>>> bt_att_debug_func_t debug_callback;
>>> bt_att_destroy_func_t debug_destroy;
>>> void *debug_data;
>>> +
>>> + struct bt_crypto *crypto;
>>> +
>>> + uint8_t local_csrk[16];
>>> + uint32_t local_sign_cnt;
>>> };
>>>
>>> enum att_op_type {
>>> @@ -176,6 +182,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 +285,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 +305,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) {
>>
>> This should be used only if CSRK was provided. So we should have local_csrk_valid or
>> similar flag (similar to what we currently have in android code). Check should be
>> either here or we could also bail out sooner in
>> bt_gatt_client_write_without_response().
>
> I think if we add loca_csrk_valid flag, it would be better to put in here. Since csrk
> Information in the bt_att structure, it should be hidden the details from upper layer.
>
>>
>>> + 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 +361,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);
>>> @@ -707,6 +735,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 +773,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 +964,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 +1149,13 @@ bool bt_att_unregister_all(struct bt_att *att)
>>>
>>> return true;
>>> }
>>> +
>>> +void bt_att_set_csrk(struct bt_att *att,
>>> + const uint8_t *val,
>>> + uint32_t local_sign_cnt)
>>
>> This should allow to set local and/or remote csrk. Either we have two functions or
>> flag identifying which key is provided.
>
> This is good idea. But this patch for client side, there is no need to use remote_csrk,
> So would it be better to add remote csrk part when we need this?
I want to function. One to set the local CSRK and one for setting the remote CSRK. However I have no idea why we are bothering with the sign counter. Just reset it when creating bt_att.
>
>> Also there should be a way to notify user of bt_att about incremented values of
>> counters.
>
> This suggestion is also the same with previous one. I have thought about add
> function like bt_att_get_csrk for user. However , for btgatt-client tool, there is no
> need to use it right now, so I didn't add it.
So unless it needs to be remember persistent across connects, then I would not bother with the sign counter at all. If it needs to be remembered, then yes, we something to tell the owner of bt_att that it got updated.
Regards
Marcel
Hi Szymon,
> > Add bt_att_set_csrk for btgatt-client tool. However this patch hook
> > into shared/mgmt, so btgatt-client now should be running by the root us=
er.
>=20
> I guess this could be made optional.
You said this could be made optional , what's you point ? I am not very cle=
ar about
That.
=20
> > ---
> > src/shared/att-types.h | 3 ++
> > src/shared/att.c | 49 ++++++++++++++++--
> > src/shared/att.h | 2 +
> > src/shared/gatt-client.c | 11 ++--
> > tools/btgatt-client.c | 129
> > ++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 186
> > insertions(+), 8 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..aadef9e 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,11 @@ struct bt_att {
> > bt_att_debug_func_t debug_callback;
> > bt_att_destroy_func_t debug_destroy;
> > void *debug_data;
> > +
> > + struct bt_crypto *crypto;
> > +
> > + uint8_t local_csrk[16];
> > + uint32_t local_sign_cnt;
> > };
> >
> > enum att_op_type {
> > @@ -176,6 +182,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 +285,10 @@
> > static bool encode_pdu(struct att_send_op *op, const void *pdu,
> > uint16_t length, uint16_t mtu) {
> > uint16_t pdu_len =3D 1;
> > + struct bt_att *att =3D op->att;
> > +
> > + if (op->opcode & ATT_OP_SIGNED_MASK)
> > + pdu_len +=3D BT_ATT_SIGNATURE_LEN;
> >
> > if (length && pdu)
> > pdu_len +=3D length;
> > @@ -293,17 +305,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) {
>=20
> This should be used only if CSRK was provided. So we should have local_cs=
rk_valid or
> similar flag (similar to what we currently have in android code). Check s=
hould be
> either here or we could also bail out sooner in
> bt_gatt_client_write_without_response().
I think if we add loca_csrk_valid flag, it would be better to put in here. =
Since csrk
Information in the bt_att structure, it should be hidden the details from u=
pper layer.
>=20
> > + 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 vo=
id
> > *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 =3D att->mtu;
> >
> > if (length && !pdu)
> > return NULL;
> > @@ -334,6 +361,7 @@ static struct att_send_op
> > *create_att_send_op(uint8_t opcode, const void *pdu, op->callback =3D c=
allback;
> > op->destroy =3D destroy;
> > op->user_data =3D user_data;
> > + op->att =3D att;
> >
> > if (!encode_pdu(op, pdu, length, mtu)) {
> > free(op);
> > @@ -707,6 +735,10 @@ struct bt_att *bt_att_new(int fd)
> > if (!att->io)
> > goto fail;
> >
> > + att->crypto =3D bt_crypto_new();
> > + if (!att->crypto)
> > + goto fail;
> > +
> > att->req_queue =3D queue_new();
> > if (!att->req_queue)
> > goto fail;
> > @@ -741,6 +773,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 +964,7 @@ unsigned int bt_att_send(struct bt_att *att,
> > uint8_t opcode, if (!att || !att->io)
> > return 0;
> >
> > - op =3D create_att_send_op(opcode, pdu, length, att->mtu, callback,
> > + op =3D create_att_send_op(opcode, pdu, length, att, callback,
> > user_data, destroy);
> > if (!op)
> > return 0;
> > @@ -1116,3 +1149,13 @@ bool bt_att_unregister_all(struct bt_att *att)
> >
> > return true;
> > }
> > +
> > +void bt_att_set_csrk(struct bt_att *att,
> > + const uint8_t *val,
> > + uint32_t local_sign_cnt)
>=20
> This should allow to set local and/or remote csrk. Either we have two fun=
ctions or
> flag identifying which key is provided.
This is good idea. But this patch for client side, there is no need to use =
remote_csrk,
So would it be better to add remote csrk part when we need this?=20
> Also there should be a way to notify user of bt_att about incremented val=
ues of
> counters.
This suggestion is also the same with previous one. I have thought about ad=
d
function like bt_att_get_csrk for user. However , for btgatt-client tool, t=
here is no
need to use it right now, so I didn't add it.
> > +{
> > + memcpy(att->local_csrk, val, 16);
> > + att->local_sign_cnt =3D local_sign_cnt; }
> > +
> > +
> > diff --git a/src/shared/att.h b/src/shared/att.h index
> > 1063021..b76e5f5 100644
> > --- a/src/shared/att.h
> > +++ b/src/shared/att.h
> > @@ -76,3 +76,5 @@ 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);
> > +
> > +void bt_att_set_csrk(struct bt_att *att, const uint8_t *val, uint32_t
> > local_sign_cnt); diff --git a/src/shared/gatt-client.c
> > b/src/shared/gatt-client.c index 1a157ec..b1619c6 100644
> > --- a/src/shared/gatt-client.c
> > +++ b/src/shared/gatt-client.c
> > @@ -973,19 +973,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);
> > }
> > diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
> > index d1395b2..fdf8485 100644
> > --- a/tools/btgatt-client.c
> > +++ b/tools/btgatt-client.c
>=20
> It would be nice if you do not mix src/shared/ and tools/ changes in sing=
le
> patch (makes history and review easier).
Great. I will follow your advice.
>=20
> > @@ -38,11 +38,14 @@
> > #include <bluetooth/hci_lib.h>
> > #include <bluetooth/l2cap.h>
> > #include "lib/uuid.h"
> > +#include "lib/bluetooth.h"
> > +#include "lib/mgmt.h"
> >
> > #include "monitor/mainloop.h"
> > #include "src/shared/util.h"
> > #include "src/shared/att.h"
> > #include "src/shared/gatt-client.h"
> > +#include "src/shared/mgmt.h"
> >
> > #define ATT_CID 4
> >
> > @@ -59,6 +62,8 @@
> > #define COLOR_BOLDWHITE "\x1B[1;37m"
> >
> > static bool verbose =3D false;
> > +static struct mgmt *mgmt_if =3D NULL;
> > +static uint16_t adapter_index =3D MGMT_INDEX_NONE;
> >
> > struct client {
> > int fd;
> > @@ -92,6 +97,115 @@ static void gatt_debug_cb(const char *str, void
> > *user_data) PRLOG(COLOR_GREEN "%s%s\n" COLOR_OFF, prefix, str);
> > }
> >
> > +static void new_csrk_callback(uint16_t index, uint16_t length,
> > + const void *param, void *user_data)
> > +{
> > + const struct mgmt_ev_new_csrk *ev =3D param;
> > + struct bt_att *att =3D user_data;
> > +
> > + if (length < sizeof(*ev)) {
> > + fprintf(stderr, "Too small csrk event (%u bytes)\n", length);
> > + return;
> > + }
> > +
> > + switch (ev->key.master) {
> > + case 0x00:
> > + bt_att_set_csrk(att, ev->key.val, 0);
> > + break;
> > + case 0x01:
> > + break;
> > + default:
> > + fprintf(stderr,
> > + "Unknown CSRK key type 02%02x\n", ev->key.master);
> > + return;
> > + }
> > +}
> > +
> > +static void mgmt_index_added_event(uint16_t index, uint16_t length,
> > + const void *param, void *user_data)
> > +{
> > + struct bt_att *att =3D user_data;
> > +
> > + printf("index %u\n", index);
> > +
> > + if (adapter_index !=3D MGMT_INDEX_NONE) {
> > + fprintf(stderr, "skip event for index %u\n", index);
> > + return;
> > + }
> > + adapter_index =3D index;
> > +
> > + mgmt_register(mgmt_if, MGMT_EV_NEW_CSRK, adapter_index,
> > + new_csrk_callback, att, NULL);
> > +}
> > +
> > +static void mgmt_index_removed_event(uint16_t index, uint16_t length,
> > + const void *param, void *user_data)
> > +{
> > + if (index !=3D adapter_index)
> > + return;
> > +
> > + perror("Adapter was removed. Exiting.\n");
>=20
> Using perror() is not correct here (and in other places as well). It will
> print error message with description of (most likely bogus) errno value. =
It
> should be used only for printing error message after failed syscall (or i=
f
> function uses errno for error reporting).
>=20
Mistake . I will modify it.
> > + raise(SIGTERM);
> > +}
> > +
> > +static void read_version_complete(uint8_t status, uint16_t length,
> > + const void *param, void *user_data)
> > +{
> > + const struct mgmt_rp_read_version *rp =3D param;
> > + uint8_t mgmt_version, mgmt_revision;
> > + struct bt_att *att =3D user_data;
> > +
> > + if (status) {
> > + fprintf(stderr,
> > + "Failed to read version information: %s (0x%02x)\n",
> > + mgmt_errstr(status), status);
> > + return;
> > + }
> > +
> > + if (length < sizeof(*rp)) {
> > + perror("Wrong size response\n");
>=20
> Ditto.
>=20
> > + return;
> > + }
> > +
> > + mgmt_version =3D rp->version;
> > + mgmt_revision =3D btohs(rp->revision);
> > +
> > + printf("Bluetooth management interface %u.%u initialized\n",
> > + mgmt_version, mgmt_revision);
> > +
> > + if (MGMT_VERSION(mgmt_version, mgmt_revision) < MGMT_VERSION(1, 3)) {
> > + perror("Version 1.3 later of management interface required\n");
> > + return;
>=20
> Why 1.3?
I follow the bluetoothd daemon implementation, Maybe it lower than 1.3,
Would some mgmt API would not compatible with kernel? It' s for safe purpo=
se here.
> > + }
> > +
> > + mgmt_register(mgmt_if, MGMT_EV_INDEX_ADDED, MGMT_INDEX_NONE,
> > + mgmt_index_added_event, att, NULL);
> > + mgmt_register(mgmt_if, MGMT_EV_INDEX_REMOVED,
> MGMT_INDEX_NONE,
> > + mgmt_index_removed_event, NULL, NULL);
> > +}
> > +
> > +static bool mgmt_create(struct bt_att *att)
> > +{
> > + mgmt_if =3D mgmt_new_default();
> > + if (!mgmt_if) {
> > + perror("Failed to access management interface\n");
> > + return false;
> > + }
> > +
> > + if (mgmt_send(mgmt_if, MGMT_OP_READ_VERSION, MGMT_INDEX_NONE, 0,
> NULL,
> > + read_version_complete, att, NULL) =3D=3D 0) {
> > + perror("Error sending READ_VERSION mgmt command\n");
> > +
> > + mgmt_unref(mgmt_if);
> > + mgmt_if =3D NULL;
> > +
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > static void ready_cb(bool success, uint8_t att_ecode, void *user_data)=
;
> >
> > static struct client *client_create(int fd, uint16_t mtu)
> > @@ -136,6 +250,13 @@ static struct client *client_create(int fd, uint16=
_t
> > mtu) return NULL;
> > }
> >
> > + if (!mgmt_create(att)) {
> > + fprintf(stderr, "Failed to create management interface\n");
> > + bt_att_unref(att);
> > + free(cli);
> > + return NULL;
> > + }
> > +
> > if (verbose) {
> > bt_att_set_debug(att, att_debug_cb, "att: ", NULL);
> > bt_gatt_client_set_debug(cli->gatt, gatt_debug_cb, "gatt: ",
> > @@ -155,6 +276,12 @@ static void client_destroy(struct client *cli)
> > bt_gatt_client_unref(cli->gatt);
> > }
> >
> > +static void mgmt_destroy(struct mgmt *mgmt_if)
> > +{
> > + mgmt_unref(mgmt_if);
> > + mgmt_if =3D NULL;
>=20
> You are shadowing mgmt_if here so it will not NULL static (it doesn't rea=
lly
> introduce a bug since tool is exiting at that time but makes code harder =
to
> follow). I'd just opencode this.
You mean remove mgmt_destroy function and just directly use mgmt_unref , ri=
ght?
But I still cannot catch your meaning about this making code harder to foll=
ow
Best Regards
Chaojie Gu
Hi Chaojie,
> Add bt_att_set_csrk for btgatt-client tool. However this patch hook into
> shared/mgmt, so btgatt-client now should be running by the root user.
As Szymon said, please split this patch into two: one for gatt-client
and one for the tool. Please use a proper commit message title for
each of them, prefixed with "shared/gatt-client:" and
"tools/btgatt_client:" respectively, with descriptive commit messages
for each patch.
It's also helpful if you add the -v flag when running git format-patch
to assign a version number to each iteration of the patch set. Makes
the reviews easier to keep track of. There are many examples of this
in the mailing list that you can refer to.
Thanks,
Arman
Hi Gu Chaojie,
On Wednesday 17 of September 2014 20:53:15 Gu Chaojie wrote:
> Add bt_att_set_csrk for btgatt-client tool. However this patch hook into
> shared/mgmt, so btgatt-client now should be running by the root user.
I guess this could be made optional.
> ---
> src/shared/att-types.h | 3 ++
> src/shared/att.c | 49 ++++++++++++++++--
> src/shared/att.h | 2 +
> src/shared/gatt-client.c | 11 ++--
> tools/btgatt-client.c | 129
> ++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 186
> insertions(+), 8 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..aadef9e 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,11 @@ struct bt_att {
> bt_att_debug_func_t debug_callback;
> bt_att_destroy_func_t debug_destroy;
> void *debug_data;
> +
> + struct bt_crypto *crypto;
> +
> + uint8_t local_csrk[16];
> + uint32_t local_sign_cnt;
> };
>
> enum att_op_type {
> @@ -176,6 +182,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 +285,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 +305,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) {
This should be used only if CSRK was provided. So we should have
local_csrk_valid or similar flag (similar to what we currently have in android
code). Check should be either here or we could also bail out sooner in
bt_gatt_client_write_without_response().
> + 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 +361,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);
> @@ -707,6 +735,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 +773,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 +964,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 +1149,13 @@ bool bt_att_unregister_all(struct bt_att *att)
>
> return true;
> }
> +
> +void bt_att_set_csrk(struct bt_att *att,
> + const uint8_t *val,
> + uint32_t local_sign_cnt)
This should allow to set local and/or remote csrk. Either we have two
functions or flag identifying which key is provided.
Also there should be a way to notify user of bt_att about incremented values
of counters.
> +{
> + memcpy(att->local_csrk, val, 16);
> + att->local_sign_cnt = local_sign_cnt;
> +}
> +
> +
> diff --git a/src/shared/att.h b/src/shared/att.h
> index 1063021..b76e5f5 100644
> --- a/src/shared/att.h
> +++ b/src/shared/att.h
> @@ -76,3 +76,5 @@ 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);
> +
> +void bt_att_set_csrk(struct bt_att *att, const uint8_t *val, uint32_t
> local_sign_cnt); diff --git a/src/shared/gatt-client.c
> b/src/shared/gatt-client.c
> index 1a157ec..b1619c6 100644
> --- a/src/shared/gatt-client.c
> +++ b/src/shared/gatt-client.c
> @@ -973,19 +973,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);
> }
> diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
> index d1395b2..fdf8485 100644
> --- a/tools/btgatt-client.c
> +++ b/tools/btgatt-client.c
It would be nice if you do not mix src/shared/ and tools/ changes in single
patch (makes history and review easier).
> @@ -38,11 +38,14 @@
> #include <bluetooth/hci_lib.h>
> #include <bluetooth/l2cap.h>
> #include "lib/uuid.h"
> +#include "lib/bluetooth.h"
> +#include "lib/mgmt.h"
>
> #include "monitor/mainloop.h"
> #include "src/shared/util.h"
> #include "src/shared/att.h"
> #include "src/shared/gatt-client.h"
> +#include "src/shared/mgmt.h"
>
> #define ATT_CID 4
>
> @@ -59,6 +62,8 @@
> #define COLOR_BOLDWHITE "\x1B[1;37m"
>
> static bool verbose = false;
> +static struct mgmt *mgmt_if = NULL;
> +static uint16_t adapter_index = MGMT_INDEX_NONE;
>
> struct client {
> int fd;
> @@ -92,6 +97,115 @@ static void gatt_debug_cb(const char *str, void
> *user_data) PRLOG(COLOR_GREEN "%s%s\n" COLOR_OFF, prefix, str);
> }
>
> +static void new_csrk_callback(uint16_t index, uint16_t length,
> + const void *param, void *user_data)
> +{
> + const struct mgmt_ev_new_csrk *ev = param;
> + struct bt_att *att = user_data;
> +
> + if (length < sizeof(*ev)) {
> + fprintf(stderr, "Too small csrk event (%u bytes)\n", length);
> + return;
> + }
> +
> + switch (ev->key.master) {
> + case 0x00:
> + bt_att_set_csrk(att, ev->key.val, 0);
> + break;
> + case 0x01:
> + break;
> + default:
> + fprintf(stderr,
> + "Unknown CSRK key type 02%02x\n", ev->key.master);
> + return;
> + }
> +}
> +
> +static void mgmt_index_added_event(uint16_t index, uint16_t length,
> + const void *param, void *user_data)
> +{
> + struct bt_att *att = user_data;
> +
> + printf("index %u\n", index);
> +
> + if (adapter_index != MGMT_INDEX_NONE) {
> + fprintf(stderr, "skip event for index %u\n", index);
> + return;
> + }
> + adapter_index = index;
> +
> + mgmt_register(mgmt_if, MGMT_EV_NEW_CSRK, adapter_index,
> + new_csrk_callback, att, NULL);
> +}
> +
> +static void mgmt_index_removed_event(uint16_t index, uint16_t length,
> + const void *param, void *user_data)
> +{
> + if (index != adapter_index)
> + return;
> +
> + perror("Adapter was removed. Exiting.\n");
Using perror() is not correct here (and in other places as well). It will
print error message with description of (most likely bogus) errno value. It
should be used only for printing error message after failed syscall (or if
function uses errno for error reporting).
> + raise(SIGTERM);
> +}
> +
> +static void read_version_complete(uint8_t status, uint16_t length,
> + const void *param, void *user_data)
> +{
> + const struct mgmt_rp_read_version *rp = param;
> + uint8_t mgmt_version, mgmt_revision;
> + struct bt_att *att = user_data;
> +
> + if (status) {
> + fprintf(stderr,
> + "Failed to read version information: %s (0x%02x)\n",
> + mgmt_errstr(status), status);
> + return;
> + }
> +
> + if (length < sizeof(*rp)) {
> + perror("Wrong size response\n");
Ditto.
> + return;
> + }
> +
> + mgmt_version = rp->version;
> + mgmt_revision = btohs(rp->revision);
> +
> + printf("Bluetooth management interface %u.%u initialized\n",
> + mgmt_version, mgmt_revision);
> +
> + if (MGMT_VERSION(mgmt_version, mgmt_revision) < MGMT_VERSION(1, 3)) {
> + perror("Version 1.3 later of management interface required\n");
> + return;
Why 1.3?
> + }
> +
> + mgmt_register(mgmt_if, MGMT_EV_INDEX_ADDED, MGMT_INDEX_NONE,
> + mgmt_index_added_event, att, NULL);
> + mgmt_register(mgmt_if, MGMT_EV_INDEX_REMOVED, MGMT_INDEX_NONE,
> + mgmt_index_removed_event, NULL, NULL);
> +}
> +
> +static bool mgmt_create(struct bt_att *att)
> +{
> + mgmt_if = mgmt_new_default();
> + if (!mgmt_if) {
> + perror("Failed to access management interface\n");
> + return false;
> + }
> +
> + if (mgmt_send(mgmt_if, MGMT_OP_READ_VERSION, MGMT_INDEX_NONE, 0, NULL,
> + read_version_complete, att, NULL) == 0) {
> + perror("Error sending READ_VERSION mgmt command\n");
> +
> + mgmt_unref(mgmt_if);
> + mgmt_if = NULL;
> +
> + return false;
> + }
> +
> + return true;
> +}
> +
> static void ready_cb(bool success, uint8_t att_ecode, void *user_data);
>
> static struct client *client_create(int fd, uint16_t mtu)
> @@ -136,6 +250,13 @@ static struct client *client_create(int fd, uint16_t
> mtu) return NULL;
> }
>
> + if (!mgmt_create(att)) {
> + fprintf(stderr, "Failed to create management interface\n");
> + bt_att_unref(att);
> + free(cli);
> + return NULL;
> + }
> +
> if (verbose) {
> bt_att_set_debug(att, att_debug_cb, "att: ", NULL);
> bt_gatt_client_set_debug(cli->gatt, gatt_debug_cb, "gatt: ",
> @@ -155,6 +276,12 @@ static void client_destroy(struct client *cli)
> bt_gatt_client_unref(cli->gatt);
> }
>
> +static void mgmt_destroy(struct mgmt *mgmt_if)
> +{
> + mgmt_unref(mgmt_if);
> + mgmt_if = NULL;
You are shadowing mgmt_if here so it will not NULL static (it doesn't really
introduce a bug since tool is exiting at that time but makes code harder to
follow). I'd just opencode this.
> +}
> +
> static void print_uuid(const uint8_t uuid[16])
> {
> char uuid_str[MAX_LEN_UUID_STR];
> @@ -1029,5 +1156,7 @@ int main(int argc, char *argv[])
>
> client_destroy(cli);
>
> + mgmt_destroy(mgmt_if);
> +
> return EXIT_SUCCESS;
> }
--
BR
Szymon Janc