Bluetooth: btintel: Add infrastructure to read controller information

-----Original Message-----
From: Dan Carpenter <[email protected]>
Sent: Friday, September 18, 2020 3:09 PM
To: [email protected]
Cc: [email protected]
Subject: [bug report] Bluetooth: btintel: Add infrastructure to read controller information

Hello Kiran K,

The patch 57375beef71a: "Bluetooth: btintel: Add infrastructure to read controller information" from Sep 14, 2020, leads to the following static checker warning:

	drivers/bluetooth/btintel.c:490 btintel_read_version_tlv()
	error: 'tlv->len' from user is not capped properly

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_variant
   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 the 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 is possible.

If skb->len is less than sizeof(*tlv) then it will read beyond the end of the skb.

while (skb->len >= sizeof(struct intel_tlv)) {

But struct intel_tlv is variable length so it's more complicated than just testing while we need aditional tests below.

   435                  struct intel_tlv *tlv;
   436
   437                  tlv = (struct intel_tlv *)skb->data;

if (struct_size(tlv->len, val, tvl) > skb->len)

	return -EINVAL;

The length has to be at least 1.

if (tvl->len < 1)

	return -EINVAL;

   438                  switch (tlv->type) {
   439                  case INTEL_TLV_CNVI_TOP:

Ever test which is reads more than 1 byte has to have a check:

if (tvl->len < sizeof(u32))

	return -EINVAL;

   440                          version->cnvi_top = get_unaligned_le32(tlv->val);
   441                          break;
   442                  case INTEL_TLV_CNVR_TOP:

Here too, etc.

   443                          version->cnvr_top = get_unaligned_le32(tlv->val);
   444                          break;
   445                  case INTEL_TLV_CNVI_BT:
   446                          version->cnvi_bt = get_unaligned_le32(tlv->val);
   447                          break;
   448                  case INTEL_TLV_CNVR_BT:
   449                          version->cnvr_bt = get_unaligned_le32(tlv->val);
   450                          break;
   451                  case INTEL_TLV_DEV_REV_ID:
   452                          version->dev_rev_id = get_unaligned_le16(tlv->val);
   453                          break;
   454                  case INTEL_TLV_IMAGE_TYPE:
   455                          version->img_type = tlv->val[0];
   456                          break;
   457                  case INTEL_TLV_TIME_STAMP:

if (tvl->len < sizeof(u16))

	return -EINVAL;

   458                          version->timestamp = get_unaligned_le16(tlv->val);
   459                          break;
   460                  case INTEL_TLV_BUILD_TYPE:
   461                          version->build_type = tlv->val[0];
   462                          break;
   463                  case INTEL_TLV_BUILD_NUM:
   464                          version->build_num = get_unaligned_le32(tlv->val);
   465                          break;
   466                  case INTEL_TLV_SECURE_BOOT:
   467                          version->secure_boot = tlv->val[0];
   468                          break;
   469                  case INTEL_TLV_OTP_LOCK:
   470                          version->otp_lock = tlv->val[0];
   471                          break;
   472                  case INTEL_TLV_API_LOCK:
   473                          version->api_lock = tlv->val[0];
   474                          break;
   475                  case INTEL_TLV_DEBUG_LOCK:
   476                          version->debug_lock = tlv->val[0];
   477                          break;
   478                  case INTEL_TLV_MIN_FW:

if (tvl->len < 3)

	return -EINVAL;

   479                          version->min_fw_build_nn = tlv->val[0];
   480                          version->min_fw_build_cw = tlv->val[1];
   481                          version->min_fw_build_yy = tlv->val[2];
   482                          break;
   483                  case INTEL_TLV_LIMITED_CCE:
   484                          version->limited_cce = tlv->val[0];
   485                          break;
   486                  case INTEL_TLV_SBE_TYPE:
   487                          version->sbe_type = tlv->val[0];
   488                          break;
   489                  case INTEL_TLV_OTP_BDADDR:
   490                          memcpy(&version->otp_bd_addr, tlv->val, tlv->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 in -

Quoted 1 line

otp_bd_addr?

	if (tlv->len != sizeof(version->otp_bd_addr))
		return -EINVAL;

   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;

regards,
dan carpenter