Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211101140544.Bluez.1.I515833d2764b8ec2ac2bb1f87313de80ebb497cd@changeid>
 <CABBYNZJg=Acx_BLyq=RzbkvfWOj-Rik3t7B1Z-XZqemo2LMykQ@mail.gmail.com>
In-Reply-To: <CABBYNZJg=Acx_BLyq=RzbkvfWOj-Rik3t7B1Z-XZqemo2LMykQ@mail.gmail.com>
From:   Archie Pusaka <apusaka@google.com>
Date:   Tue, 2 Nov 2021 17:10:14 +0800
Message-ID: <CAJQfnxHP0dRzahhcK2S6sN-e1ssk1S2BZB3Nzs+SCTXufBVfGA@mail.gmail.com>
Subject: Re: [Bluez PATCH 1/3] adapter: Use PeripheralLongTermKey to store LTK
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     linux-bluetooth <linux-bluetooth@vger.kernel.org>,
        Marcel Holtmann <marcel@holtmann.org>,
        CrosBT Upstreaming <chromeos-bluetooth-upstreaming@chromium.org>,
        Archie Pusaka <apusaka@chromium.org>,
        Sonny Sasaka <sonnysasaka@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Luiz,


On Tue, 2 Nov 2021 at 14:17, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Sun, Oct 31, 2021 at 11:06 PM Archie Pusaka <apusaka@google.com> wrote:
> >
> > From: Archie Pusaka <apusaka@chromium.org>
> >
> > Introducing PeripheralLongTermKey group for storing LTK info to
> > replace the less inclusive term. Currently we still need to write/read
> > from both to ensure smooth transition, but later we should deprecate
> > the old term.
> >
> > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
> > ---
> >
> >  src/adapter.c | 41 ++++++++++++++++++++++++++++++++---------
> >  1 file changed, 32 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/adapter.c b/src/adapter.c
> > index d0d38621b8..6b12c9e793 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> > @@ -3868,7 +3868,11 @@ static struct smp_ltk_info *get_peripheral_ltk_info(GKeyFile *key_file,
> >
> >         DBG("%s", peer);
> >
> > -       ltk = get_ltk(key_file, peer, bdaddr_type, "SlaveLongTermKey");
> > +       /* Read from both entries. Later we should deprecate Slave. */
> > +       ltk = get_ltk(key_file, peer, bdaddr_type, "PeripheralLongTermKey");
> > +       if (!ltk)
> > +               ltk = get_ltk(key_file, peer, bdaddr_type, "SlaveLongTermKey");
>
> If you find the old name being used you better replace it with the new
> one and erase the old.
>
> >         if (ltk)
> >                 ltk->central = false;
> >
> > @@ -8415,13 +8419,12 @@ static void new_link_key_callback(uint16_t index, uint16_t length,
> >         bonding_complete(adapter, &addr->bdaddr, addr->type, 0);
> >  }
> >
> > -static void store_longtermkey(struct btd_adapter *adapter, const bdaddr_t *peer,
> > +static void store_ltk_group(struct btd_adapter *adapter, const bdaddr_t *peer,
> >                                 uint8_t bdaddr_type, const unsigned char *key,
> > -                               uint8_t central, uint8_t authenticated,
> > +                               const char *group, uint8_t authenticated,
> >                                 uint8_t enc_size, uint16_t ediv,
> >                                 uint64_t rand)
> >  {
> > -       const char *group = central ? "LongTermKey" : "SlaveLongTermKey";
> >         char device_addr[18];
> >         char filename[PATH_MAX];
> >         GKeyFile *key_file;
> > @@ -8431,11 +8434,6 @@ static void store_longtermkey(struct btd_adapter *adapter, const bdaddr_t *peer,
> >         char *str;
> >         int i;
> >
> > -       if (central != 0x00 && central != 0x01) {
> > -               error("Unsupported LTK type %u", central);
> > -               return;
> > -       }
> > -
> >         ba2str(peer, device_addr);
> >
> >         snprintf(filename, PATH_MAX, STORAGEDIR "/%s/%s/info",
> > @@ -8475,6 +8473,31 @@ static void store_longtermkey(struct btd_adapter *adapter, const bdaddr_t *peer,
> >         g_key_file_free(key_file);
> >  }
> >
> > +static void store_longtermkey(struct btd_adapter *adapter, const bdaddr_t *peer,
> > +                               uint8_t bdaddr_type, const unsigned char *key,
> > +                               uint8_t central, uint8_t authenticated,
> > +                               uint8_t enc_size, uint16_t ediv,
> > +                               uint64_t rand)
> > +{
> > +       if (central != 0x00 && central != 0x01) {
> > +               error("Unsupported LTK type %u", central);
> > +               return;
> > +       }
> > +
> > +       if (central) {
> > +               store_ltk_group(adapter, peer, bdaddr_type, key, "LongTermKey",
> > +                               authenticated, enc_size, ediv, rand);
> > +       } else {
> > +               /* Store duplicates for now. Later we should deprecate Slave. */
> > +               store_ltk_group(adapter, peer, bdaddr_type, key,
> > +                               "PeripheralLongTermKey", authenticated,
> > +                               enc_size, ediv, rand);
> > +               store_ltk_group(adapter, peer, bdaddr_type, key,
> > +                               "SlaveLongTermKey", authenticated,
> > +                               enc_size, ediv, rand);
>
> In not following why you want to keep duplicate entries?
>
Based on the discussion in the other thread with Marcel, we don't want
to immediately remove the old entry since this would cause friction
when people are switching around between the newer and older version
of BlueZ.
Instead, for some time we would keep both entries alive, and only
after that we remove the old entry.

> > +       }
> > +}
> > +
> >  static void new_long_term_key_callback(uint16_t index, uint16_t length,
> >                                         const void *param, void *user_data)
> >  {
> > --
> > 2.33.1.1089.g2158813163f-goog
> >
>
>
> --
> Luiz Augusto von Dentz

Thanks,
Archie