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> <CAJQfnxHP0dRzahhcK2S6sN-e1ssk1S2BZB3Nzs+SCTXufBVfGA@mail.gmail.com>
In-Reply-To: <CAJQfnxHP0dRzahhcK2S6sN-e1ssk1S2BZB3Nzs+SCTXufBVfGA@mail.gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 2 Nov 2021 15:39:38 -0700
Message-ID: <CABBYNZK5xdvTsoBCHdEDE86s6UtxKOGLmO7oO5Pn1swY23YWOA@mail.gmail.com>
Subject: Re: [Bluez PATCH 1/3] adapter: Use PeripheralLongTermKey to store LTK
To:     Archie Pusaka <apusaka@google.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 Archie,

On Tue, Nov 2, 2021 at 2:10 AM Archie Pusaka <apusaka@google.com> wrote:
>
> 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.

I guess the problem would be downgrading wouldn't work since we would
have removed the old value once upgrading, in that case I would be
fine keeping it but Id leave a comment since we should probably remove
that as soon as we have a few releases.

> 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


-- 
Luiz Augusto von Dentz