RE: [PATCH v3 1/3] Bluetooth: btintel: validate version TLV value lengths

[K, Kiran]

Hi Luiz,

>Subject: [PATCH v3 1/3] Bluetooth: btintel: validate version TLV value lengths
>
>btintel_parse_version_tlv() verifies that a complete TLV is present in the
>response, but it does not ensure that the value is long enough for the specific
>TLV type. A short value can therefore cause an out-of-bounds read through
>get_unaligned_le16(), get_unaligned_le32(), or memcpy().
>
>Reject values shorter than the minimum required by each known TLV type.
>Also reject responses that do not contain the Command Complete Status field.
>
>Fixes: 57375beef71a ("Bluetooth: btintel: Add infrastructure to read controller
>information")
>Reviewed-by: Ali Ahmet Memis <ali@xxxxxxxxxxxxxx>
>Signed-off-by: Laxman Acharya Padhya <acharyalaxman8848@xxxxxxxxx>
>---
> drivers/bluetooth/btintel.c | 37
>++++++++++++++++++++++++++++++++++++-
> 1 file changed, 36 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c index
>cbeb27033..998c99b17 100644
>--- a/drivers/bluetooth/btintel.c
>+++ b/drivers/bluetooth/btintel.c
>@@ -573,12 +573,44 @@ int btintel_version_info_tlv(struct hci_dev *hdev,  }
>EXPORT_SYMBOL_GPL(btintel_version_info_tlv);
>
>+static u8 btintel_version_tlv_min_len(u8 type) {
>+	switch (type) {
>+	case INTEL_TLV_CNVI_TOP:
>+	case INTEL_TLV_CNVR_TOP:
>+	case INTEL_TLV_CNVI_BT:
>+	case INTEL_TLV_CNVR_BT:
>+	case INTEL_TLV_BUILD_NUM:
>+	case INTEL_TLV_GIT_SHA1:
>+		return sizeof(u32);
>+	case INTEL_TLV_DEV_REV_ID:
>+	case INTEL_TLV_TIME_STAMP:
>+		return sizeof(u16);
>+	case INTEL_TLV_IMAGE_TYPE:
>+	case INTEL_TLV_BUILD_TYPE:
>+	case INTEL_TLV_SECURE_BOOT:
>+	case INTEL_TLV_OTP_LOCK:
>+	case INTEL_TLV_API_LOCK:
>+	case INTEL_TLV_DEBUG_LOCK:
>+	case INTEL_TLV_LIMITED_CCE:
>+	case INTEL_TLV_SBE_TYPE:
>+		return sizeof(u8);
>+	case INTEL_TLV_MIN_FW:
>+		return 3;
>+	case INTEL_TLV_OTP_BDADDR:
>+		return sizeof(bdaddr_t);
>+	default:
>+		return 0;
>+	}
>+}
>+
> int btintel_parse_version_tlv(struct hci_dev *hdev,
> 			      struct intel_version_tlv *version,
> 			      struct sk_buff *skb)
> {
> 	/* Consume Command Complete Status field */
>-	skb_pull(skb, 1);
>+	if (!skb_pull(skb, 1))
>+		return -EINVAL;
>
> 	/* Event parameters contain multiple TLVs. Read each of them
> 	 * and only keep the required data. Also, it use existing legacy @@ -
>598,6 +630,9 @@ int btintel_parse_version_tlv(struct hci_dev *hdev,
> 		if (skb->len < tlv->len + sizeof(*tlv))
> 			return -EINVAL;
>
>+		if (tlv->len < btintel_version_tlv_min_len(tlv->type))
>+			return -EINVAL;
>+
> 		switch (tlv->type) {
> 		case INTEL_TLV_CNVI_TOP:
> 			version->cnvi_top = get_unaligned_le32(tlv->val);
>--
>2.51.2

Tested-by: Kiran K <kiran.k@xxxxxxxxx>

Thanks,
Kiran