[PATCH v2] Bluetooth: MGMT: validate advertising TLV before type checks

[PATCH v2] Bluetooth: MGMT: validate advertising TLV before type checks

[Zhang Cen]

tlv_data_is_valid() reads each advertising data field length from
data[i], then inspects data[i + 1] for managed EIR types before
checking that the current field still fits inside the supplied buffer.

A malformed field whose length byte is the last byte of the buffer can
therefore make the parser read one byte past the advertising data.

Move the existing element-length check before any type-octet inspection
so each non-empty element is proven to contain its type byte before the
parser looks at data[i + 1].

KASAN reported an out-of-bounds read in tlv_data_is_valid(), reached
through add_advertising() and hci_mgmt_cmd().

Fixes: 2bb36870e8cb ("Bluetooth: Unify advertising instance flags check")
Signed-off-by: Zhang Cen <rollkingzzc@gmail.com>
---
v2:
- Drop the MGMT_OP_ADD_EXT_ADV_DATA hunk; bluetooth-next already
  validates that command length.
- Keep only the tlv_data_is_valid() element-ordering fix.
- Add a Fixes tag and avoid the raw KASAN headline that triggered
  checkpatch.

 net/bluetooth/mgmt.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index de5bd6b637b20..027b266ccc747 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -8638,6 +8638,12 @@ static bool tlv_data_is_valid(struct hci_dev *hdev, u32 adv_flags, u8 *data,
 		if (!cur_len)
 			continue;
 
+		/* If the current field length would exceed the total data
+		 * length, then it's invalid.
+		 */
+		if (i + cur_len >= len)
+			return false;
+
 		if (data[i + 1] == EIR_FLAGS &&
 		    (!is_adv_data || flags_managed(adv_flags)))
 			return false;
@@ -8654,12 +8660,6 @@ static bool tlv_data_is_valid(struct hci_dev *hdev, u32 adv_flags, u8 *data,
 		if (data[i + 1] == EIR_APPEARANCE &&
 		    appearance_managed(adv_flags))
 			return false;
-
-		/* If the current field length would exceed the total data
-		 * length, then it's invalid.
-		 */
-		if (i + cur_len >= len)
-			return false;
 	}
 
 	return true;
-- 
2.43.0

Re: [PATCH v2] Bluetooth: MGMT: validate advertising TLV before type checks

[Paul Menzel]

Dear Cen,


Thank you for your patch.

Am 28.05.26 um 09:57 schrieb Zhang, Cen:
> tlv_data_is_valid() reads each advertising data field length from
> data[i], then inspects data[i + 1] for managed EIR types before
> checking that the current field still fits inside the supplied buffer.
> 
> A malformed field whose length byte is the last byte of the buffer can
> therefore make the parser read one byte past the advertising data.
> 
> Move the existing element-length check before any type-octet inspection
> so each non-empty element is proven to contain its type byte before the
> parser looks at data[i + 1].
> 
> KASAN reported an out-of-bounds read in tlv_data_is_valid(), reached
> through add_advertising() and hci_mgmt_cmd().

I’d add this after the second paragraph, and also add an excerpt, so 
people also seeing the trace can find the patch/commit more easily.

> Fixes: 2bb36870e8cb ("Bluetooth: Unify advertising instance flags check")
> Signed-off-by: Zhang Cen <rollkingzzc@gmail.com>
> ---
> v2:
> - Drop the MGMT_OP_ADD_EXT_ADV_DATA hunk; bluetooth-next already
>    validates that command length.
> - Keep only the tlv_data_is_valid() element-ordering fix.
> - Add a Fixes tag and avoid the raw KASAN headline that triggered
>    checkpatch.
> 
>   net/bluetooth/mgmt.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index de5bd6b637b20..027b266ccc747 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -8638,6 +8638,12 @@ static bool tlv_data_is_valid(struct hci_dev *hdev, u32 adv_flags, u8 *data,
>   		if (!cur_len)
>   			continue;
>   
> +		/* If the current field length would exceed the total data
> +		 * length, then it's invalid.
> +		 */
> +		if (i + cur_len >= len)
> +			return false;

Does Linux log something in this case?

> +
>   		if (data[i + 1] == EIR_FLAGS &&
>   		    (!is_adv_data || flags_managed(adv_flags)))
>   			return false;
> @@ -8654,12 +8660,6 @@ static bool tlv_data_is_valid(struct hci_dev *hdev, u32 adv_flags, u8 *data,
>   		if (data[i + 1] == EIR_APPEARANCE &&
>   		    appearance_managed(adv_flags))
>   			return false;
> -
> -		/* If the current field length would exceed the total data
> -		 * length, then it's invalid.
> -		 */
> -		if (i + cur_len >= len)
> -			return false;
>   	}
>   
>   	return true;

The diff looks good. Feel free to add:

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

Re: [PATCH v2] Bluetooth: MGMT: validate advertising TLV before type checks

[Cen Zhang]

Hi Paul,

Thanks for the review.

> Does Linux log something in this case?
>
There is no printk or other kernel log for this case. The malformed
advertising data makes tlv_data_is_valid() return false, and
add_advertising() then reports MGMT_STATUS_INVALID_PARAMS through the
Bluetooth management command status response. Keeping this quiet matches
the surrounding validation paths and avoids user-triggerable log noise.

> The diff looks good. Feel free to add:
>
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>

I will send v3 with the KASAN excerpt moved earlier in the commit
message, and with your Reviewed-by tag added.

Thanks,
Zhang Cen