Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-1269-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <741ff02f-596c-4c50-b809-d5fd39b77244@collabora.com>
Date: Tue, 23 Jan 2024 16:00:24 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH BlueZ 2/2] device: Update local and remote CSRK on
 management event
Content-Language: en-US
To: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc: linux-bluetooth@vger.kernel.org
References: <20240123121519.523735-1-frederic.danis@collabora.com>
 <20240123121519.523735-2-frederic.danis@collabora.com>
 <CABBYNZKMry5Vh5Xu5t7ykoWOo8zjqjVfS+zfYk1fxDomHRpDQg@mail.gmail.com>
From: =?UTF-8?Q?Fr=C3=A9d=C3=A9ric_Danis?= <frederic.danis@collabora.com>
In-Reply-To: <CABBYNZKMry5Vh5Xu5t7ykoWOo8zjqjVfS+zfYk1fxDomHRpDQg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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