linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1] Bluetooth: btintel: Fix issue reported by static analysis tool
@ 2020-09-22  4:00 Kiran K
  2020-09-22  5:16 ` Luiz Augusto von Dentz
  0 siblings, 1 reply; 4+ messages in thread
From: Kiran K @ 2020-09-22  4:00 UTC (permalink / raw)
  To: linux-bluetooth
  Cc: chethan.tumkur.narayan, ravishankar.srivatsa, dan.carpenter,
	kiraank, Kiran K

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


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v1] Bluetooth: btintel: Fix issue reported by static analysis tool
  2020-09-22  4:00 [PATCH v1] Bluetooth: btintel: Fix issue reported by static analysis tool Kiran K
@ 2020-09-22  5:16 ` Luiz Augusto von Dentz
  2020-09-22  6:02   ` K, Kiran
  0 siblings, 1 reply; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2020-09-22  5:16 UTC (permalink / raw)
  To: Kiran K
  Cc: linux-bluetooth, Chethan T N, ravishankar.srivatsa,
	dan.carpenter, Kiran K

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
>


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH v1] Bluetooth: btintel: Fix issue reported by static analysis tool
  2020-09-22  5:16 ` Luiz Augusto von Dentz
@ 2020-09-22  6:02   ` K, Kiran
  2020-09-25 16:39     ` Marcel Holtmann
  0 siblings, 1 reply; 4+ messages in thread
From: K, Kiran @ 2020-09-22  6:02 UTC (permalink / raw)
  To: Luiz Augusto von Dentz, Kiran K
  Cc: linux-bluetooth, Tumkur Narayan, Chethan, Srivatsa, Ravishankar,
	dan.carpenter

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v1] Bluetooth: btintel: Fix issue reported by static analysis tool
  2020-09-22  6:02   ` K, Kiran
@ 2020-09-25 16:39     ` Marcel Holtmann
  0 siblings, 0 replies; 4+ messages in thread
From: Marcel Holtmann @ 2020-09-25 16:39 UTC (permalink / raw)
  To: K, Kiran
  Cc: Luiz Augusto von Dentz, Kiran K, linux-bluetooth, Tumkur Narayan,
	Chethan, Srivatsa, Ravishankar, dan.carpenter

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-09-25 16:39 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-22  4:00 [PATCH v1] Bluetooth: btintel: Fix issue reported by static analysis tool Kiran K
2020-09-22  5:16 ` Luiz Augusto von Dentz
2020-09-22  6:02   ` K, Kiran
2020-09-25 16:39     ` Marcel Holtmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).