Hi Dan,
Thanks for the comments. For some reason, the static analysis tool I am using didn't report these issues. I will submit a patch soon to address these issues.
Regards,
Kiran
> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Sent: Friday, September 18, 2020 3:09 PM
> To: kiraank@xxxxxxxxx
> Cc: linux-bluetooth@xxxxxxxxxxxxxxx
> 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 -
> >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
