Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-1271-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240123121519.523735-1-frederic.danis@collabora.com>
 <20240123121519.523735-2-frederic.danis@collabora.com> <CABBYNZKMry5Vh5Xu5t7ykoWOo8zjqjVfS+zfYk1fxDomHRpDQg@mail.gmail.com>
 <741ff02f-596c-4c50-b809-d5fd39b77244@collabora.com>
In-Reply-To: <741ff02f-596c-4c50-b809-d5fd39b77244@collabora.com>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Tue, 23 Jan 2024 10:14:06 -0500
Message-ID: <CABBYNZ+bmanaepVN4Xq1kiOqjuZJ07mtKcFBqjVsefw-PwYkew@mail.gmail.com>
Subject: Re: [PATCH BlueZ 2/2] device: Update local and remote CSRK on
 management event
To: =?UTF-8?B?RnLDqWTDqXJpYyBEYW5pcw==?= <frederic.danis@collabora.com>
Cc: linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Fr=C3=A9d=C3=A9ric,

On Tue, Jan 23, 2024 at 10:00=E2=80=AFAM Fr=C3=A9d=C3=A9ric Danis
<frederic.danis@collabora.com> wrote:
>
> Hi Luiz,
>
> On 23/01/2024 14:53, Luiz Augusto von Dentz wrote:
> > Hi Fr=C3=A9d=C3=A9ric,
> >
> > On Tue, Jan 23, 2024 at 7:15=E2=80=AFAM Fr=C3=A9d=C3=A9ric Danis
> > <frederic.danis@collabora.com> wrote:
> >> The local and remote CSRK keys are only loaded from storage during sta=
rt.
> >>
> >> 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, ui=
nt16_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 *devi=
ce, 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 =3D 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 =3D 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 =3D 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, con=
st 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?

Don't we have the CSRK stored at adapter level though? Or we have to
generate a pair of local/remote CSRK for each device, if we do I'm not
seeing when we store the local CSRK.

> >>   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 bd=
addr_type,
> >> --
> >> 2.34.1
> >>
> >>
> >
>
> --
> Fr=C3=A9d=C3=A9ric Danis
> Senior Software Engineer
>
> Collabora Ltd.
> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, United K=
ingdom
> Registered in England & Wales, no. 5513718
>


--=20
Luiz Augusto von Dentz