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
