Re: [PATCH 21/27] qtnfmac: extend "IE set" TLV to include frame type info

[Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>]

> -		if (tlv_full_len > payload_len) {
> -			pr_warn("VIF%u.%u: malformed TLV 0x%.2X; LEN: %u\n",
> -				mac->macid, vif->vifid, tlv_type,
> -				tlv_value_len);
> +		if (tlv_full_len > payload_len)
>  			return -EINVAL;
> -		}

Why drop this sanity check ?

>  		if (tlv_type == QTN_TLV_ID_IE_SET) {
> -			sinfo.assoc_req_ies = tlv->val;
> -			sinfo.assoc_req_ies_len = tlv_value_len;
> +			const struct qlink_tlv_ie_set *ie_set;
> +			unsigned int ie_len;
> +
> +			if (payload_len < sizeof(*ie_set))
> +				return -EINVAL;
> +
> +			ie_set = (const struct qlink_tlv_ie_set *)tlv;
> +			ie_len = tlv_value_len -
> +				(sizeof(*ie_set) - sizeof(ie_set->hdr));
> +
> +			if (ie_set->type == QLINK_IE_SET_ASSOC_REQ && ie_len) {
> +				sinfo.assoc_req_ies = ie_set->ie_data;
> +				sinfo.assoc_req_ies_len = ie_len;
> +			}
>  		}

Does it make sense to keep QTN_TLV_ID_IE_SET here at all ?
Maybe replace it completely by qlink_tlv_ie_set with
QLINK_IE_SET_ASSOC_REQ type ? Also see the comment below
for the similar snippet in qtnf_event_handle_scan_results.

...

> -		if (tlv_full_len > payload_len) {
> -			pr_warn("VIF%u.%u: malformed TLV 0x%.2X; LEN: %u\n",
> -				vif->mac->macid, vif->vifid, tlv_type,
> -				tlv_value_len);
> +		if (tlv_full_len > payload_len)
>  			return -EINVAL;
> -		}

ditto

...

>  		if (tlv_type == QTN_TLV_ID_IE_SET) {
> -			ies = tlv->val;
> -			ies_len = tlv_value_len;
> +			const struct qlink_tlv_ie_set *ie_set;
> +			unsigned int ie_len;
> +
> +			if (payload_len < sizeof(*ie_set))
> +				return -EINVAL;
> +
> +			ie_set = (const struct qlink_tlv_ie_set *)tlv;
> +			ie_len = tlv_value_len -
> +				(sizeof(*ie_set) - sizeof(ie_set->hdr));
> +
> +			if (ie_len) {
> +				ies = ie_set->ie_data;
> +				ies_len = ie_len;
> +			}
>  		}
>  	}

Two points here. First, it looks like there is a problem here inherited
from the existing implementation. We go through payload, but in fact we
pass to cfg80211_inform_bss only the last QTN_TLV_ID_IE_SET element.
Second, it looks like QTN_TLV_ID_IE_SET here should be treated in
the same way as in qtnf_event_handle_sta_assoc, to avoid confusion.
In other words, either we use only QTN_TLV_ID_IE_SET in both cases,
or switch to specific qlink_tlv_ie_set elements.

Thoughts ? Comments ?

Regards,
Sergey