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;
IronPort-SDR: 2Zn0NidZr0NStf0tudzIDywcWEAAhHiKYjxF555Ql0OuiVlIJA3LyMhIuxqWHtPoN7HID0spP+
 6eTPy6lrMPwg==
IronPort-SDR: Lv9dkA8TM4MP09WVuCpgMZDpZzrunECkqyGpZNixutWK0o38IEv+DiI7dU/e85WFvPIVNu3SiO
 dgUPtXPksyTA==
From:   "K, Kiran" <kiran.k@intel.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>,
        "kiraank@gmail.com" <kiraank@gmail.com>
CC:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        "Srivatsa, Ravishankar" <ravishankar.srivatsa@intel.com>,
        "Tumkur Narayan, Chethan" <chethan.tumkur.narayan@intel.com>
Subject: RE: [bug report] Bluetooth: btintel: Add infrastructure to read
 controller information
Thread-Topic: [bug report] Bluetooth: btintel: Add infrastructure to read
 controller information
Thread-Index: AQHWjZ/gVkmoNz+emESKM63nVsqm26lya27g
Date:   Mon, 21 Sep 2020 03:03:54 +0000
Message-ID: <BYAPR11MB3141AF55929E6AE9CC0225DDF53A0@BYAPR11MB3141.namprd11.prod.outlook.com>
References: <20200918093902.GA882111@mwanda>
In-Reply-To: <20200918093902.GA882111@mwanda>
Accept-Language: en-US
Content-Language: en-US
dlp-reaction: no-action
dlp-version: 11.5.1.3
dlp-product: dlpe-windows
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 8c1c491e-6d32-4faa-63cb-08d85ddaf992
X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Sep 2020 03:03:54.1880
 (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: FWIfAMO69FQTh0tvi7hBVrMPPO6ZJnIPdvzzdFf2YvNXvXRDp36j7Ur1GvpHNrhoEoI0P/y/2wjM0MSoAyBQ3g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR11MB3494
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-bluetooth.vger.kernel.org>
X-Mailing-List: linux-bluetooth@vger.kernel.org

Hi Dan,

Thanks for the comments.  For some reason, the static analysis tool I am us=
ing didn't report these issues. I will submit a patch soon to address these=
 issues.

Regards,
Kiran

> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: Friday, September 18, 2020 3:09 PM
> To: kiraank@gmail.com
> Cc: linux-bluetooth@vger.kernel.org
> Subject: [bug report] Bluetooth: btintel: Add infrastructure to read cont=
roller
> information
>=20
> Hello Kiran K,
>=20
> The patch 57375beef71a: "Bluetooth: btintel: Add infrastructure to read
> controller information" from Sep 14, 2020, leads to the following static
> checker warning:
>=20
> 	drivers/bluetooth/btintel.c:490 btintel_read_version_tlv()
> 	error: 'tlv->len' from user is not capped properly
>=20
> drivers/bluetooth/btintel.c
>    426          /* Consume Command Complete Status field */
>    427          skb_pull(skb, 1);
>    428
>    429          /* Event parameters contatin multiple TLVs. Read each of =
them
>    430           * and only keep the required data. Also, it use existing=
 legacy
>    431           * version field like hw_platform, hw_variant, and fw_var=
iant
>    432           * to keep the existing setup flow
>    433           */
>    434          while (skb->len) {
>                        ^^^^^^^^
> I feel like these days we are trying to not trust firmware...  Smatch is
> complaining because it distrusts all skb->data information, but unless th=
e
> devs at Google have a way to connect a fuzzer to this then trusting is
> probably harmless.  Anyway, the rest of this email assumes that fuzzing i=
s
> possible.
>=20
> If skb->len is less than sizeof(*tlv) then it will read beyond the end of=
 the skb.
>=20
> while (skb->len >=3D sizeof(struct intel_tlv)) {
>=20
> But struct intel_tlv is variable length so it's more complicated than jus=
t testing
> while we need aditional tests below.
>=20
>    435                  struct intel_tlv *tlv;
>    436
>    437                  tlv =3D (struct intel_tlv *)skb->data;
>=20
> if (struct_size(tlv->len, val, tvl) > skb->len)
> 	return -EINVAL;
>=20
> The length has to be at least 1.
>=20
> if (tvl->len < 1)
> 	return -EINVAL;
>=20
>    438                  switch (tlv->type) {
>    439                  case INTEL_TLV_CNVI_TOP:
>=20
> Ever test which is reads more than 1 byte has to have a check:
>=20
> if (tvl->len < sizeof(u32))
> 	return -EINVAL;
>=20
>    440                          version->cnvi_top =3D get_unaligned_le32(=
tlv->val);
>    441                          break;
>    442                  case INTEL_TLV_CNVR_TOP:
>=20
> Here too, etc.
>=20
>    443                          version->cnvr_top =3D get_unaligned_le32(=
tlv->val);
>    444                          break;
>    445                  case INTEL_TLV_CNVI_BT:
>    446                          version->cnvi_bt =3D get_unaligned_le32(t=
lv->val);
>    447                          break;
>    448                  case INTEL_TLV_CNVR_BT:
>    449                          version->cnvr_bt =3D get_unaligned_le32(t=
lv->val);
>    450                          break;
>    451                  case INTEL_TLV_DEV_REV_ID:
>    452                          version->dev_rev_id =3D get_unaligned_le1=
6(tlv->val);
>    453                          break;
>    454                  case INTEL_TLV_IMAGE_TYPE:
>    455                          version->img_type =3D tlv->val[0];
>    456                          break;
>    457                  case INTEL_TLV_TIME_STAMP:
>=20
> if (tvl->len < sizeof(u16))
> 	return -EINVAL;
>=20
>    458                          version->timestamp =3D get_unaligned_le16=
(tlv->val);
>    459                          break;
>    460                  case INTEL_TLV_BUILD_TYPE:
>    461                          version->build_type =3D tlv->val[0];
>    462                          break;
>    463                  case INTEL_TLV_BUILD_NUM:
>    464                          version->build_num =3D get_unaligned_le32=
(tlv->val);
>    465                          break;
>    466                  case INTEL_TLV_SECURE_BOOT:
>    467                          version->secure_boot =3D tlv->val[0];
>    468                          break;
>    469                  case INTEL_TLV_OTP_LOCK:
>    470                          version->otp_lock =3D tlv->val[0];
>    471                          break;
>    472                  case INTEL_TLV_API_LOCK:
>    473                          version->api_lock =3D tlv->val[0];
>    474                          break;
>    475                  case INTEL_TLV_DEBUG_LOCK:
>    476                          version->debug_lock =3D tlv->val[0];
>    477                          break;
>    478                  case INTEL_TLV_MIN_FW:
>=20
> if (tvl->len < 3)
> 	return -EINVAL;
>=20
>    479                          version->min_fw_build_nn =3D tlv->val[0];
>    480                          version->min_fw_build_cw =3D tlv->val[1];
>    481                          version->min_fw_build_yy =3D tlv->val[2];
>    482                          break;
>    483                  case INTEL_TLV_LIMITED_CCE:
>    484                          version->limited_cce =3D tlv->val[0];
>    485                          break;
>    486                  case INTEL_TLV_SBE_TYPE:
>    487                          version->sbe_type =3D tlv->val[0];
>    488                          break;
>    489                  case INTEL_TLV_OTP_BDADDR:
>    490                          memcpy(&version->otp_bd_addr, tlv->val, t=
lv->len);
>                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^=
^^^^^^^^
> tlv->len comes from the network and it's 0-255.  If it's more than 6
> then this will corrupt memory.  There is no caller for this function yet =
in linux-
> next so if tvl->len is less than 6 will that leave uninitialized memory i=
n -
> >otp_bd_addr?
>=20
> 	if (tlv->len !=3D sizeof(version->otp_bd_addr))
> 		return -EINVAL;
>=20
>    491                          break;
>    492                  default:
>    493                          /* Ignore rest of information */
>    494                          break;
>    495                  }
>    496                  /* consume the current tlv and move to next*/
>    497                  skb_pull(skb, tlv->len + sizeof(*tlv));
>    498          }
>    499
>    500          kfree_skb(skb);
>    501          return 0;
>=20
> regards,
> dan carpenter