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: <20211005042613.9946-1-kiran.k@intel.com> <79B0A1CF-17D0-473C-A321-39E1BC291A89@holtmann.org>
In-Reply-To: <79B0A1CF-17D0-473C-A321-39E1BC291A89@holtmann.org>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Wed, 6 Oct 2021 10:49:23 -0700
Message-ID: <CABBYNZL9f1j6sEcNm6B9ONg6yV=Pt=msuOd3Urb-0d-5vmm7qw@mail.gmail.com>
Subject: Re: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with
 garbage value
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     Kiran K <kiran.k@intel.com>,
        linux-bluetooth <linux-bluetooth@vger.kernel.org>,
        Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>,
        Chethan T N <chethan.tumkur.narayan@intel.com>,
        "An, Tedd" <tedd.an@intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Marcel,

On Wed, Oct 6, 2021 at 1:52 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Kiran,
>
> > Intel Read Verision(TLV) data is parsed into a local structure variable
> > and it contains a field for bd address. Bd address is returned only in
> > bootloader mode and hence bd address in TLV structure needs to be validated
> > only if controller is present in boot loader mode.
> >
> > Signed-off-by: Kiran K <kiran.k@intel.com>
> > ---
> > drivers/bluetooth/btintel.c | 19 +++++++++++--------
> > 1 file changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> > index 9359bff47296..d1703cc99705 100644
> > --- a/drivers/bluetooth/btintel.c
> > +++ b/drivers/bluetooth/btintel.c
> > @@ -2081,14 +2081,16 @@ static int btintel_prepare_fw_download_tlv(struct hci_dev *hdev,
> >       if (ver->img_type == 0x03) {
> >               btintel_clear_flag(hdev, INTEL_BOOTLOADER);
> >               btintel_check_bdaddr(hdev);
> > -     }
> > -
> > -     /* If the OTP has no valid Bluetooth device address, then there will
> > -      * also be no valid address for the operational firmware.
> > -      */
> > -     if (!bacmp(&ver->otp_bd_addr, BDADDR_ANY)) {
> > -             bt_dev_info(hdev, "No device address configured");
> > -             set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
> > +     } else {
> > +             /*
> > +              * Check for valid bd address in boot loader mode. Device
> > +              * will be marked as unconfigured if empty bd address is
> > +              * found.
> > +              */
> > +             if (!bacmp(&ver->otp_bd_addr, BDADDR_ANY)) {
> > +                     bt_dev_info(hdev, "No device address configured");
> > +                     set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
> > +             }
> >       }
> >
> >       btintel_get_fw_name_tlv(ver, fwname, sizeof(fwname), "sfi");
> > @@ -2466,6 +2468,7 @@ static int btintel_setup_combined(struct hci_dev *hdev)
> >               goto exit_error;
> >       }
> >
> > +     memset(&ver_tlv, 0, sizeof(ver_tlv));
>
> this change is not described in the commit message. Why is that now out of a sudden needed?

I guess this is just to make sure the ver_tlv is initialized so its
otp_bd_addr be set to all zeros (BDADDR_ANY) otherwise the code above
doesn't work as it attempts to compare to BDADDR_ANY.

> Regards
>
> Marcel
>


-- 
Luiz Augusto von Dentz