Hi, arman,
> > This patch for signing outgoing, csrk and cryto stored in bt_att.
> > the verifying incoming ATT PDU implementation introduce the
> > shared/gatt-server which combined with shared/gatt-db.c, so need more
> > disccussion.
> >
> > Signed-off-by: Gu Chaojie <chao.jie.gu@xxxxxxxxx>
>
> I don't think include "Signed-off-by" in commit messages in BlueZ.
>
I will follow bluez rules.
> > +/* Len of signature in write signed packet */
> > +#define BT_ATT_SIGNATURE_LEN 12
>
> Can you spell out "Length of signature" in the comment instead of just "Len"?
No problem.
>
> > + bool valid_remote_csrk;
> > + uint8_t remote_csrk[16];
> > + uint32_t remote_sign_cnt;
> Can we add these later if they aren't being used yet?
>
I agree with that.
> > + struct bt_att *att = NULL;
> > +
> > + if (op->opcode == BT_ATT_OP_SIGNED_WRITE_CMD) {
>
> Actually, instead of checking explicitly for BT_ATT_OP_SIGNED_WRITE_CMD, it
> might be better to check if the opcode has the "signed" bit set in general. (You can
> use ATT_OP_SIGNED_MASK, which is defined in this file).
>
>
> > + pdu_len += BT_ATT_SIGNATURE_LEN;
> > + att = op->user_data;
>
> I don't think that this code here works. op->user_data refers to user data provided in
> bt_att_send and not the bt_att structure. Instead, I would just add a "struct bt_att
> *att" field to struct att_send_op and remove the local variable "att".
In my implementation , op->user_data in the signed write command code path refer to
the bt_att structure because struct bt_att provided in bt_att_send parameter user_data.
So it could be work.
But your proposal make more sense if extend the att_send_op struct to store struct bt_att *att
Because user_data should be provided for callback , current implementation style is ugly indeed.
> > + }
> >
> > if (length && pdu)
> > pdu_len += length;
> > @@ -293,6 +310,16 @@ static bool encode_pdu(struct att_send_op *op, const
> void *pdu,
> > if (pdu_len > 1)
> > memcpy(op->pdu + 1, pdu, length);
> >
> > + if (att) {
> > + if (!bt_crypto_sign_att(att->crypto,
> > + att->local_csrk,
> > + op->pdu,
> > + 1 + length,
> > + att->local_sign_cnt,
> > + &((uint8_t *) op->pdu)[1 +
> length]))
> > + return false;
> > + }
>
> You shouldn't have to check if "att" is NULL. "att" should always be present if struct
> att_send_op has a field for it.
This problem would be fixed by extending struct att_send_op.
> I would also remove the nested if statement and just return the return value of
> bt_crypto_sign_att inline.
>
> Other than that, I don't see how the CSRK is being provided to the bt_att structure.
> Shouldn't there be a bt_att_set_csrk of sorts?
>
This point is my major purpose for this patch discussion. We can see in android/gatt.c,
Csrk and sign counter get from struct device in new_csrk_callback by mgmt_register
event from kernel. And we can get att->csrk from struct device. However, in
btgatt-client tool, there is no struct device and no event notification handler.
So if we have to implemetnt bt_att_set_csrk, how can we get csrk ?
And for the lifecycle of sign_counter, it should be updated to store in files after signed write.
Btgatt-client need to create special file to do that?
Best Regards
Chaojie Gu
��.n��������+%������w��{.n�����{��� �^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
