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;
From:   "K, Kiran" <kiran.k@intel.com>
To:     Marcel Holtmann <marcel@holtmann.org>
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
Thread-Topic: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with
 garbage value
Thread-Index: AQHXuaBi4PrY9W41v0ul9TVX3EMhsqvFq4mAgACWNICAAM0YAIAIgV0AgAELt7A=
Date:   Wed, 13 Oct 2021 07:55:48 +0000
Message-ID: <DM8PR11MB55736DA45A8E0F5CBE390F8CF5B79@DM8PR11MB5573.namprd11.prod.outlook.com>
References: <20211005042613.9946-1-kiran.k@intel.com>
 <79B0A1CF-17D0-473C-A321-39E1BC291A89@holtmann.org>
 <CABBYNZL9f1j6sEcNm6B9ONg6yV=Pt=msuOd3Urb-0d-5vmm7qw@mail.gmail.com>
 <DM8PR11MB557322FB90F730BDE853E742F5B19@DM8PR11MB5573.namprd11.prod.outlook.com>
 <23272CA9-FEE5-4E66-942F-E66FA1E513ED@holtmann.org>
In-Reply-To: <23272CA9-FEE5-4E66-942F-E66FA1E513ED@holtmann.org>
Accept-Language: en-US
Content-Language: en-US
dlp-reaction: no-action
dlp-version: 11.6.200.16
dlp-product: dlpe-windows
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?ODFELdvrw8Ia4pQOjsutIcR5o73HcH0yaibztbSiC/qgWqn16vDkYVyYd2F3?=
 =?us-ascii?Q?TZG33IzROz/GULS3KVOL2eePGXPJlX83U9FNl53V3ivGiUUDGUphZQY1vGXO?=
 =?us-ascii?Q?MpajWTarGxU7Ijy49ADs0dWOHX0xBosOCIUgUaZrFjve5PgxJE5DK97GOeng?=
 =?us-ascii?Q?kd4ATRjjqAgepdXwn7ZpzmqPO2/tWWc/yL74UdGTkHYO6XUGckXhVGLke+Ha?=
 =?us-ascii?Q?jqRXHORjsEiGyhIdZDapfwbUht4HEAyZ2t4U3NnprRFnwnNS7o1WojxsrGxE?=
 =?us-ascii?Q?lodvLQ54epyRAyoKIy+s2qjEaz+N0I3ZDyn0g30XHQQtpXhFFta6WkuQ42nH?=
 =?us-ascii?Q?BvqRtjjSGAm4eUja9GfV1ePlu6++Nx3AJWeebm9M9StXD1cVg9kLPjV/3Sme?=
 =?us-ascii?Q?g8pt+yvY9RPHopmpTtMuhRq+Umw415qr1bP93QucbDxbU+D8U30CFGC7h0dp?=
 =?us-ascii?Q?v59W3OoqgUwviWzhIJ+sTJd/dpVPoS+BPHgru2ijJZBonKM6rAWQgGOkXwmQ?=
 =?us-ascii?Q?HBWnQn5mCbR+fkazq+zjKHsntzWRApuKtP4lKBTZWzxggaKLLsPuYtBO/p6N?=
 =?us-ascii?Q?PVd43nNeKSj9XkzRwd9TI/axWiNvNJtX2MVhH9+T/Im34tOhKcIdd5uZS7hU?=
 =?us-ascii?Q?p/t3mgHUPuCKGoSrei54w364H7kinMEYYqkPen18pohpyQLNoEbLOOWpm5Hj?=
 =?us-ascii?Q?/U+0VQ5g3Pfw2TQTJSiRzqeGtrUEgQ+uOOKHPrmhP10+e1L0qe29XXIcRq6G?=
 =?us-ascii?Q?VU5UoG3Z72Q8eA8aPgGZx8YP9nQZE5gmxTA5znHPaXVOnt17PA34gP0GZ/BB?=
 =?us-ascii?Q?QgBtobvIVIDm8gcO42gkgRCHH53umvnafrX+QfVLnhh0rZBZwE0g+RHxfAN6?=
 =?us-ascii?Q?jxS5InxKEeNUh5iUmkShp6VxSxTFmRcgp7Qc9d9XQKc+iRUmfJNp9LkZgrjR?=
 =?us-ascii?Q?NC5zD/arsNDCVr6snSdHiaxFAwvZZ5V5/N3Mw6HU1STB0B+6eJgO/p12ir8L?=
 =?us-ascii?Q?+hyV6DKf91iF+3G4mGaW9TuOFpCTSp8FXgz6oRwamsaLcOsrv4ZvO/MSAJmr?=
 =?us-ascii?Q?qCH38hKfVcqV3K+VvXEXZDLeALCTC9E0BBD8uSXhI00syk0H7ALG+OJU/esw?=
 =?us-ascii?Q?vFG1YRW/h13wfYunXcZv98geZx7cc7Icie/Zo/9Sb8prJNCiArdgwMFRh29m?=
 =?us-ascii?Q?BIZY28X8k0M87l/4PrQPatzYpWPxVU7LNX1DPM+E7Kuf0TReHMaXJJKkeZh2?=
 =?us-ascii?Q?xJTcybLVu9SHS7W1TI1oYG/lNWdTF+4vXyqX5ZiXH1Q3q15FYv97Ecfw+fdx?=
 =?us-ascii?Q?4mt5ZllQKuiQFV/0duSicNQd?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM8PR11MB5573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ab1e13b5-6774-4ce4-4f58-08d98e1ede92
X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Oct 2021 07:55:48.3013
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: HIGbyXsJgzVT6DYaqhVnjg9mmP04hzJDpz3UUj8Iakdpgdq5toQqYa/ZKMZ48gXSPBgMTBtoaFjfFc2Nx2hd8A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM8PR11MB5574
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-bluetooth.vger.kernel.org>
X-Mailing-List: linux-bluetooth@vger.kernel.org

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
>=20
> Hi Kiran,
>=20
> >>>> 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 =3D=3D 0x03) {
> >>>>              btintel_clear_flag(hdev, INTEL_BOOTLOADER);
> >>>>              btintel_check_bdaddr(hdev);
> >>>> -     }
> >>>> -
> >>>> -     /* If the OTP has no valid Bluetooth device address, then ther=
e 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. Dev=
ice
> >>>> +              * 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 configure=
d");
> >>>> +                     set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirk=
s);
> >>>> +             }
> >>>>      }
> >>>>
> >>>>      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.
>=20
> since that is not obviously clear, the takeaway from my review should hav=
e
> 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.

>=20
> Regards
>=20
> Marcel