Re: [PATCH BlueZ 2/2] device: Update local and remote CSRK on management event

["Frédéric Danis" <frederic.danis@collabora.com>]

Hi Luiz,

On 23/01/2024 14:53, Luiz Augusto von Dentz wrote:
> Hi Frédéric,
>
> On Tue, Jan 23, 2024 at 7:15 AM Frédéric Danis
> <frederic.danis@collabora.com> wrote:
>> The local and remote CSRK keys are only loaded from storage during start.
>>
>> Those keys should be updated on MGMT_EV_NEW_CSRK event to be able to
>> perform signed write for GAP/SEC/CSIGN/BV-02-C.
>> ---
>>   src/adapter.c |  2 ++
>>   src/device.c  | 16 ++++++++++++++++
>>   src/device.h  |  2 ++
>>   3 files changed, 20 insertions(+)
>>
>> diff --git a/src/adapter.c b/src/adapter.c
>> index 022390f0d..fb71ef83e 100644
>> --- a/src/adapter.c
>> +++ b/src/adapter.c
>> @@ -8882,6 +8882,8 @@ static void new_csrk_callback(uint16_t index, uint16_t length,
>>                  return;
>>          }
>>
>> +       device_set_csrk(device, key->val, key->type & 0x01);
>> +
>>          if (!ev->store_hint)
>>                  return;
>>
>> diff --git a/src/device.c b/src/device.c
>> index 17bcfbc49..34f64ca5b 100644
>> --- a/src/device.c
>> +++ b/src/device.c
>> @@ -1955,6 +1955,22 @@ bool btd_device_get_ltk(struct btd_device *device, uint8_t key[16],
>>          return true;
>>   }
>>
>> +void device_set_csrk(struct btd_device *device, const uint8_t val[16],
>> +                               bool remote)
>> +{
>> +       if (remote) {
>> +               g_free(device->remote_csrk);
>> +               device->remote_csrk = g_new0(struct csrk_info, 1);
>> +               memcpy(device->remote_csrk->key, val,
>> +                      sizeof(device->remote_csrk->key));
>> +       } else {
>> +               g_free(device->local_csrk);
>> +               device->local_csrk = g_new0(struct csrk_info, 1);
>> +               memcpy(device->local_csrk->key, val,
>> +                      sizeof(device->local_csrk->key));
>> +       }
>> +}
>> +
>>   static bool match_sirk(const void *data, const void *match_data)
>>   {
>>          const struct sirk_info *sirk = data;
>> diff --git a/src/device.h b/src/device.h
>> index 8bb38669d..d00c002c3 100644
>> --- a/src/device.h
>> +++ b/src/device.h
>> @@ -134,6 +134,8 @@ void device_set_ltk(struct btd_device *device, const uint8_t val[16],
>>                                  bool central, uint8_t enc_size);
>>   bool btd_device_get_ltk(struct btd_device *device, uint8_t val[16],
>>                                  bool *central, uint8_t *enc_size);
>> +void device_set_csrk(struct btd_device *device, const uint8_t val[16],
>> +                               bool remote);
> Looks like there is only one use of this function and it is always set
> for the remote, actually the fact that this is on the device object
> already means it is for the remote so I wonder if we really need to
> store the local as well?

As device is able to store and load both keys from storage I think it 
could be better to keep them in sync, no?

>>   bool btd_device_add_set(struct btd_device *device, bool encrypted,
>>                                  uint8_t sirk[16], uint8_t size, uint8_t rank);
>>   void device_store_svc_chng_ccc(struct btd_device *device, uint8_t bdaddr_type,
>> --
>> 2.34.1
>>
>>
>

-- 
Frédéric Danis
Senior Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, United Kingdom
Registered in England & Wales, no. 5513718