RE: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with garbage value

["K, Kiran" <kiran.k@intel.com>]

Hi Marcel,

> -----Original Message-----
> From: Marcel Holtmann <marcel@holtmann.org>
> Sent: Tuesday, October 12, 2021 9:27 PM
> To: K, Kiran <kiran.k@intel.com>
> Cc: Luiz Augusto von Dentz <luiz.dentz@gmail.com>; linux-bluetooth <linux-
> bluetooth@vger.kernel.org>; Srivatsa, Ravishankar
> <ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@intel.com>; An, Tedd <tedd.an@intel.com>
> Subject: Re: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with
> garbage value
> 
> 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.
> >
> > Yes. If not memset, then garbage value is compared against BDADDR_ANY.
> 
> since that is not obviously clear, the takeaway from my review should have
> been that you either describe this properly in the commit message or you
> add a comment.

I will add a comment and send an updated patch. Thanks.

> 
> Regards
> 
> Marcel