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
--
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
