Hi,
On Thu, Oct 9, 2014 at 12:38 PM, Gu, Chao Jie <chao.jie.gu@xxxxxxxxx> wrote:
> Hi Luiz,
>> >
>> >> > @@ -77,6 +78,16 @@ struct bt_att {
>> >> > bt_att_debug_func_t debug_callback;
>> >> > bt_att_destroy_func_t debug_destroy;
>> >> > void *debug_data;
>> >> > +
>> >> > + struct bt_crypto *crypto;
>> >> > +
>> >> > + bool valid_local_csrk;
>> >> > + uint8_t local_csrk[16];
>> >> > + uint32_t local_sign_cnt;
>> >> > +
>> >> > + bool valid_remote_csrk;
>> >> > + uint8_t remote_csrk[16];
>> >> > + uint32_t remote_sign_cnt;
>> >>
>> >> Maybe it is better to have pointers to a structs so you can free the
>> >> data once it becomes invalid and it also removes the necessity of
>> >> extra flags just to tell if the data is still valid since you can check if the pointer is
>> not NULL then it must be valid.
>> >
>> > This is good idea to remove the extra flags. However, we define the
>> > struct to do this which would result in some problem. Now we
>> > initialize the local_sign_cnt in the bt_att_new function and only add
>> > local_sign_cnt value when signed write command outgoing. Meanwile, we
>> > set local CSRK in the bt_gatt_client_set_local_csrk function when there is valid
>> CSRK provided. This two Parameter set at different time now.
>>
>> Well I guess this was your design, it doesn't have to be this way, in fact I believe
>> there is no point of initializing the counter if there is no valid CSRK because one
>> depend on the other, also perhaps you have a bug in bt_gatt_client_set_local_csrk
>> since I believe every time you set CSRK you should reset the counter otherwise you
>> may be carrying the counters from the past CSRK which probably won't work.
>
> You said condition resuilt in bug which I want to avoid, because we use this command line
> By btgatt-client tool
> Write-value -w -c <CSRK> <value_handle> <value>
> If user write signed command twice, it would call bt_gatt_client_set_local_csrk twice.
> So it will restore sign_cnt, that's why I initialize the sign_cnt in bt_att_new to avoid this bug.
Considering what the spec says about SignCounter and that we do in
fact store the counter persistently I believe the counter is valid as
long as CSRK is valid, which means it is not per connection and
initializing it on bt_att_new is probably wrong since the counter will
probably be out of sync. We should either create a dedicate command to
set CSRK and the SignCounter or make write-value to take the counter
as well otherwise it wont work except with a fresh CSRK and
SignCounter which perhaps is the case of btgatt-client but not if we
want to reuse in the daemons.
> In fact, the only way is that we should tell user set CSRK once if CSRK did not change to
> resolve this problem.
>
>> Also don't to have to pass the counter when setting the key, the counter probably
>> need to be restored on every connection so bt_gatt_client_set_local_csrk and
>> bt_att_set_remote_csrk probably needs to take the counter and instead of having a
>> valid flag you can probably reset by setting NULL as key or an invalid counter if one
>> exists.
>
> If we define the new struct naming signed_info to store csrk and sign_cnt, what's your point?
>
>>
>> > If we have pointers to a struct including local_csrk and
>> > local_signd_cnt, we initialize the local_sign_cnt meanwhile the struct should not
>> be NULL because of incuding it. In fact, local_csrk is not valid at this time.
>> > If we initialize the local_signed_cnt when CSRK is valid , it would be
>> > risk to change local_signed_cnt by user calling bt_gatt_client_set_local_csrk.
>> >> > };
>> >> >
>> >> > enum att_op_type {
>> >> > @@ -176,6 +187,8 @@ struct att_send_op {
>> >> > bt_att_response_func_t callback;
>> >> > bt_att_destroy_func_t destroy;
>> >> > void *user_data;
>> >> > +
>> >> > + struct bt_att *att;
>> >> > };
>> >> >
>> >> > static void destroy_att_send_op(void *data) @@ -277,6 +290,10 @@
>> >> > static bool encode_pdu(struct att_send_op *op, const void *pdu,
>> >> > uint16_t length,
>> >> > uint16_t mtu) {
>> >> > uint16_t pdu_len = 1;
>> >> > + struct bt_att *att = op->att;
>> >> > +
>> >> > + if (op->opcode & ATT_OP_SIGNED_MASK)
>> >> > + pdu_len += BT_ATT_SIGNATURE_LEN;
>> >> >
>> >> > if (length && pdu)
>> >> > pdu_len += length;
>> >> > @@ -293,17 +310,32 @@ static bool encode_pdu(struct att_send_op
>> >> > *op, const
>> >> void *pdu,
>> >> > if (pdu_len > 1)
>> >> > memcpy(op->pdu + 1, pdu, length);
>> >> >
>> >> > + if (op->opcode & ATT_OP_SIGNED_MASK) {
>> >> > + if (bt_crypto_sign_att(att->crypto,
>> >> > + att->local_csrk,
>> >> > + op->pdu,
>> >> > + 1 + length,
>> >> > + att->local_sign_cnt,
>> >> > + &((uint8_t *) op->pdu)[1 +
>> >> length]))
>> >> > + att->local_sign_cnt++;
>> >> > + else
>> >> > + return false;
>> >> > + }
>> >> > +
>> >>
>> >> You can probably bail out early if you check !(op->opcode &
>> >> ATT_OP_SIGNED_MASK), also perhaps it is a good idea to start defining
>> >> structs for the PDUs to simplify the access.
>> >>
>> > If we bail out this checking early , maybe we have to need two
>> > different encode_pdu function to implement it such as naming new
>> encode_signed_pdu function to do this.
>>
>> But you return true anyway after this code, no idea why you need a new function for
>> that, all I was suggesting is the following:
>>
>> if (!(op->opcode & ATT_OP_SIGNED_MASK))
>> return true;
>>
>> if (!bt_crypto_sign_att...)
>> return false;
>>
>> att->local_sign_cnt++;
>>
>> return true;
>>
>
> I got your meaning now. I have returned false when bt_crypto_sign_att. Failed
>
> + else
> + return false;
>
> Your proposal is good so as to make code more clearer. I would accept that.
>
> Thanks
> Chaojie Gu
--
Luiz Augusto von Dentz
--
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
