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