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;
Date:   Fri, 18 Sep 2020 12:39:02 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     kiraank@gmail.com
Cc:     linux-bluetooth@vger.kernel.org
Subject: [bug report] Bluetooth: btintel: Add infrastructure to read
 controller information
Message-ID: <20200918093902.GA882111@mwanda>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Precedence: bulk

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 ->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