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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx>


Kind regards,

Paul