Hi Kiran, On Mon, Sep 21, 2020 at 9:03 PM Kiran K <kiraank@xxxxxxxxx> wrote: > > Smatch tool reported the below issue: > > drivers/bluetooth/btintel.c:490 btintel_read_version_tlv() > error: 'tlv->len' from user is not capped properly > > Additional details in the below link > https://www.spinics.net/lists/linux-bluetooth/msg87786.html > > Signed-off-by: Kiran K <kiran.k@xxxxxxxxx> > --- > drivers/bluetooth/btintel.c | 43 ++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 42 insertions(+), 1 deletion(-) > > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c > index 88ce5f0..47f2b3d 100644 > --- a/drivers/bluetooth/btintel.c > +++ b/drivers/bluetooth/btintel.c > @@ -431,62 +431,99 @@ int btintel_read_version_tlv(struct hci_dev *hdev, struct intel_version_tlv *ver > * version field like hw_platform, hw_variant, and fw_variant > * to keep the existing setup flow > */ > - while (skb->len) { > + while (skb->len >= sizeof(struct intel_tlv)) { > struct intel_tlv *tlv; > > tlv = (struct intel_tlv *)skb->data; > + if (struct_size(tlv, val, tlv->len) > skb->len) > + goto failed; > + > switch (tlv->type) { > case INTEL_TLV_CNVI_TOP: > + if (tlv->len < sizeof(u32)) > + goto failed; > version->cnvi_top = get_unaligned_le32(tlv->val); > break; > case INTEL_TLV_CNVR_TOP: > + if (tlv->len < sizeof(u32)) > + goto failed; > version->cnvr_top = get_unaligned_le32(tlv->val); > break; > case INTEL_TLV_CNVI_BT: > + if (tlv->len < sizeof(u32)) > + goto failed; > version->cnvi_bt = get_unaligned_le32(tlv->val); > break; > case INTEL_TLV_CNVR_BT: > + if (tlv->len < sizeof(u32)) > + goto failed; > version->cnvr_bt = get_unaligned_le32(tlv->val); > break; > case INTEL_TLV_DEV_REV_ID: > + if (tlv->len < sizeof(u16)) > + goto failed; > version->dev_rev_id = get_unaligned_le16(tlv->val); > break; > case INTEL_TLV_IMAGE_TYPE: > + if (tlv->len < sizeof(u8)) > + goto failed; > version->img_type = tlv->val[0]; > break; > case INTEL_TLV_TIME_STAMP: > + if (tlv->len < sizeof(u16)) > + goto failed; > version->timestamp = get_unaligned_le16(tlv->val); > break; > case INTEL_TLV_BUILD_TYPE: > + if (tlv->len < sizeof(u8)) > + goto failed; > version->build_type = tlv->val[0]; > break; > case INTEL_TLV_BUILD_NUM: > + if (tlv->len < sizeof(u32)) > + goto failed; > version->build_num = get_unaligned_le32(tlv->val); > break; > case INTEL_TLV_SECURE_BOOT: > + if (tlv->len < sizeof(u8)) > + goto failed; > version->secure_boot = tlv->val[0]; > break; > case INTEL_TLV_OTP_LOCK: > + if (tlv->len < sizeof(u8)) > + goto failed; > version->otp_lock = tlv->val[0]; > break; > case INTEL_TLV_API_LOCK: > + if (tlv->len < sizeof(u8)) > + goto failed; > version->api_lock = tlv->val[0]; > break; > case INTEL_TLV_DEBUG_LOCK: > + if (tlv->len < sizeof(u8)) > + goto failed; > version->debug_lock = tlv->val[0]; > break; > case INTEL_TLV_MIN_FW: > + if (tlv->len < 3) > + goto failed; > version->min_fw_build_nn = tlv->val[0]; > version->min_fw_build_cw = tlv->val[1]; > version->min_fw_build_yy = tlv->val[2]; > break; > case INTEL_TLV_LIMITED_CCE: > + if (tlv->len < sizeof(u8)) > + goto failed; > version->limited_cce = tlv->val[0]; > break; > case INTEL_TLV_SBE_TYPE: > + if (tlv->len < sizeof(u8)) > + goto failed; > version->sbe_type = tlv->val[0]; > break; > case INTEL_TLV_OTP_BDADDR: > + if (tlv->len != sizeof(version->otp_bd_addr)) > + goto failed; Do we really want to fail here? The advantage of using a TLV is that we can skip if the type is not understood or is malformed but with this checks the length becomes useless since the types will always have a fixed value, also we cannot extend the types later on since it would not be backward compatible if we maintain such strict checks. > memcpy(&version->otp_bd_addr, tlv->val, tlv->len); > break; > default: > @@ -499,6 +536,10 @@ int btintel_read_version_tlv(struct hci_dev *hdev, struct intel_version_tlv *ver > > kfree_skb(skb); > return 0; > + > +failed: > + kfree_skb(skb); > + return -EINVAL; > } > EXPORT_SYMBOL_GPL(btintel_read_version_tlv); > > -- > 2.7.4 > -- Luiz Augusto von Dentz
