| Message ID | 1600747219-11626-1-git-send-email-kiran.k@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] Bluetooth: btintel: Fix issue reported by static analysis tool | expand |
Hi Kiran, On Mon, Sep 21, 2020 at 9:03 PM Kiran K <kiraank@gmail.com> 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@intel.com> > --- > 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 >
Hi Luiz, > -----Original Message----- > From: Luiz Augusto von Dentz <luiz.dentz@gmail.com> > Sent: Tuesday, September 22, 2020 10:47 AM > To: Kiran K <kiraank@gmail.com> > Cc: linux-bluetooth@vger.kernel.org; Tumkur Narayan, Chethan > <chethan.tumkur.narayan@intel.com>; Srivatsa, Ravishankar > <ravishankar.srivatsa@intel.com>; dan.carpenter@oracle.com; K, Kiran > <kiran.k@intel.com> > Subject: Re: [PATCH v1] Bluetooth: btintel: Fix issue reported by static > analysis tool > > Hi Kiran, > > On Mon, Sep 21, 2020 at 9:03 PM Kiran K <kiraank@gmail.com> 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@intel.com> > > --- > > 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 Agree that the types are fixed here. But if due to some reason if controller is not honoring the same, then driver might end up reading unwanted data. The check is more about driver being defensive rather than believing what comes on wire. > we cannot extend the types later on since it would not be backward > compatible if we maintain such strict checks. I didn’t get this part. Could you please be more specific ? > > > 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 Thanks, Kiran
Hi Kiran, >>> 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@intel.com> >>> --- >>> 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 > > Agree that the types are fixed here. But if due to some reason if controller is not honoring the same, then driver might end up reading unwanted data. The check is more about driver being defensive rather than believing what comes on wire. > >> we cannot extend the types later on since it would not be backward >> compatible if we maintain such strict checks. > > I didn’t get this part. Could you please be more specific ? please change this that you check the size of the TLV data part against the expected size of the field and only then assign it. Regards Marcel
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; 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);
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@intel.com> --- drivers/bluetooth/btintel.c | 43 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-)