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

Hi Marcel, Luiz,

-----Original Message-----
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Sent: Wednesday, October 6, 2021 11:19 PM
To: Marcel Holtmann <marcel@holtmann.org>
Cc: K, Kiran <redacted>; linux-bluetooth <linux-
bluetooth@vger.kernel.org>; Srivatsa, Ravishankar
[off-list ref]; Tumkur Narayan, Chethan
[off-list ref]; An, Tedd [off-list ref]
Subject: Re: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with
garbage value

Hi Marcel,

On Wed, Oct 6, 2021 at 1:52 AM Marcel Holtmann [off-list ref]
wrote:

quoted

Hi Kiran,

quoted

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.

quoted

quoted

Signed-off-by: Kiran K <redacted>
---
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,

quoted

quoted

      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)

quoted

quoted

              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.

quoted

Regards

Marcel

--
Luiz Augusto von Dentz