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 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.
> > ---
> > 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?
> 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.
> > +{
> > + 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).
Great. I will follow your advice.
>
> > @@ -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).
>
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 = 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?
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 purpose 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 = 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.
You mean remove mgmt_destroy function and just directly use mgmt_unref , right?
But I still cannot catch your meaning about this making code harder to follow
Best Regards
Chaojie Gu
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
