diff mbox series

[v2,2/4] wifi: libertas: only add RSN/WPA IE in lbs_add_wpa_tlv

Message ID 20230108013016.222494-3-doug@schmorgal.com
State Superseded
Headers show
Series wifi: libertas: IE handling fixes | expand

Commit Message

Doug Brown Jan. 8, 2023, 1:30 a.m. UTC
The existing code only converts the first IE to a TLV, but it returns a
value that takes the length of all IEs into account. When there is more
than one IE (which happens with modern wpa_supplicant versions for
example), the returned length is too long and extra junk TLVs get sent
to the firmware, resulting in an association failure.

Fix this by finding the first RSN or WPA IE and only adding that. This
has the extra benefit of working properly if the RSN/WPA IE isn't the
first one in the IE buffer.

While we're at it, clean up the code to use the available structs like
the other lbs_add_* functions instead of directly manipulating the TLV
buffer.

Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 drivers/net/wireless/marvell/libertas/cfg.c | 28 +++++++++++++--------
 1 file changed, 18 insertions(+), 10 deletions(-)

Comments

Simon Horman Jan. 13, 2023, 9:52 a.m. UTC | #1
On Sat, Jan 07, 2023 at 05:30:14PM -0800, Doug Brown wrote:
> The existing code only converts the first IE to a TLV, but it returns a
> value that takes the length of all IEs into account. When there is more
> than one IE (which happens with modern wpa_supplicant versions for
> example), the returned length is too long and extra junk TLVs get sent
> to the firmware, resulting in an association failure.
> 
> Fix this by finding the first RSN or WPA IE and only adding that. This
> has the extra benefit of working properly if the RSN/WPA IE isn't the
> first one in the IE buffer.
> 
> While we're at it, clean up the code to use the available structs like
> the other lbs_add_* functions instead of directly manipulating the TLV
> buffer.
> 
> Signed-off-by: Doug Brown <doug@schmorgal.com>
> ---
>  drivers/net/wireless/marvell/libertas/cfg.c | 28 +++++++++++++--------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/libertas/cfg.c b/drivers/net/wireless/marvell/libertas/cfg.c
> index 3e065cbb0af9..5cd78fefbe4c 100644
> --- a/drivers/net/wireless/marvell/libertas/cfg.c
> +++ b/drivers/net/wireless/marvell/libertas/cfg.c
> @@ -416,10 +416,20 @@ static int lbs_add_cf_param_tlv(u8 *tlv)
>  
>  static int lbs_add_wpa_tlv(u8 *tlv, const u8 *ie, u8 ie_len)
>  {
> -	size_t tlv_len;
> +	struct mrvl_ie_data *wpatlv = (struct mrvl_ie_data *)tlv;
> +	const struct element *wpaie;
> +
> +	/* Find the first RSN or WPA IE to use */
> +	wpaie = cfg80211_find_elem(WLAN_EID_RSN, ie, ie_len);
> +	if (!wpaie)
> +		wpaie = cfg80211_find_vendor_elem(WLAN_OUI_MICROSOFT,
> +						  WLAN_OUI_TYPE_MICROSOFT_WPA,
> +						  ie, ie_len);
> +	if (!wpaie || wpaie->datalen > 128)
> +		return 0;
>  
>  	/*
> -	 * We need just convert an IE to an TLV. IEs use u8 for the header,
> +	 * Convert the found IE to a TLV. IEs use u8 for the header,
>  	 *   u8      type
>  	 *   u8      len
>  	 *   u8[]    data
> @@ -428,14 +438,12 @@ static int lbs_add_wpa_tlv(u8 *tlv, const u8 *ie, u8 ie_len)
>  	 *   __le16  len
>  	 *   u8[]    data
>  	 */
> -	*tlv++ = *ie++;
> -	*tlv++ = 0;
> -	tlv_len = *tlv++ = *ie++;
> -	*tlv++ = 0;
> -	while (tlv_len--)
> -		*tlv++ = *ie++;
> -	/* the TLV is two bytes larger than the IE */
> -	return ie_len + 2;
> +	wpatlv->header.type = wpaie->id;
> +	wpatlv->header.len = wpaie->datalen;

Hi Doug,

For correctness should type and len be converted to little endian,
f.e. using cpu_to_le16() ?

Likewise in patch 4/4.

> +	memcpy(wpatlv->data, wpaie->data, wpaie->datalen);
> +
> +	/* Return the total number of bytes added to the TLV buffer */
> +	return sizeof(struct mrvl_ie_header) + wpaie->datalen;
>  }
>  
>  /*
> -- 
> 2.34.1
>
Doug Brown Jan. 14, 2023, 12:25 a.m. UTC | #2
Hi Simon,

On 1/13/2023 1:52 AM, Simon Horman wrote:
> On Sat, Jan 07, 2023 at 05:30:14PM -0800, Doug Brown wrote:
>> The existing code only converts the first IE to a TLV, but it returns a
>> value that takes the length of all IEs into account. When there is more
>> than one IE (which happens with modern wpa_supplicant versions for
>> example), the returned length is too long and extra junk TLVs get sent
>> to the firmware, resulting in an association failure.
>>
>> Fix this by finding the first RSN or WPA IE and only adding that. This
>> has the extra benefit of working properly if the RSN/WPA IE isn't the
>> first one in the IE buffer.
>>
>> While we're at it, clean up the code to use the available structs like
>> the other lbs_add_* functions instead of directly manipulating the TLV
>> buffer.
>>
>> Signed-off-by: Doug Brown <doug@schmorgal.com>
>> ---
>>   drivers/net/wireless/marvell/libertas/cfg.c | 28 +++++++++++++--------
>>   1 file changed, 18 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/wireless/marvell/libertas/cfg.c b/drivers/net/wireless/marvell/libertas/cfg.c
>> index 3e065cbb0af9..5cd78fefbe4c 100644
>> --- a/drivers/net/wireless/marvell/libertas/cfg.c
>> +++ b/drivers/net/wireless/marvell/libertas/cfg.c
>> @@ -416,10 +416,20 @@ static int lbs_add_cf_param_tlv(u8 *tlv)
>>   
>>   static int lbs_add_wpa_tlv(u8 *tlv, const u8 *ie, u8 ie_len)
>>   {
>> -	size_t tlv_len;
>> +	struct mrvl_ie_data *wpatlv = (struct mrvl_ie_data *)tlv;
>> +	const struct element *wpaie;
>> +
>> +	/* Find the first RSN or WPA IE to use */
>> +	wpaie = cfg80211_find_elem(WLAN_EID_RSN, ie, ie_len);
>> +	if (!wpaie)
>> +		wpaie = cfg80211_find_vendor_elem(WLAN_OUI_MICROSOFT,
>> +						  WLAN_OUI_TYPE_MICROSOFT_WPA,
>> +						  ie, ie_len);
>> +	if (!wpaie || wpaie->datalen > 128)
>> +		return 0;
>>   
>>   	/*
>> -	 * We need just convert an IE to an TLV. IEs use u8 for the header,
>> +	 * Convert the found IE to a TLV. IEs use u8 for the header,
>>   	 *   u8      type
>>   	 *   u8      len
>>   	 *   u8[]    data
>> @@ -428,14 +438,12 @@ static int lbs_add_wpa_tlv(u8 *tlv, const u8 *ie, u8 ie_len)
>>   	 *   __le16  len
>>   	 *   u8[]    data
>>   	 */
>> -	*tlv++ = *ie++;
>> -	*tlv++ = 0;
>> -	tlv_len = *tlv++ = *ie++;
>> -	*tlv++ = 0;
>> -	while (tlv_len--)
>> -		*tlv++ = *ie++;
>> -	/* the TLV is two bytes larger than the IE */
>> -	return ie_len + 2;
>> +	wpatlv->header.type = wpaie->id;
>> +	wpatlv->header.len = wpaie->datalen;
> 
> Hi Doug,
> 
> For correctness should type and len be converted to little endian,
> f.e. using cpu_to_le16() ?
> 
> Likewise in patch 4/4.

Thank you for reviewing my changes. You are absolutely right -- this
would be broken on big-endian systems. That was a major oversight on my
part. Not sure how I missed it because all of the other functions do it
correctly. Nice catch! I'll fix it in the next version of the series.

> 
>> +	memcpy(wpatlv->data, wpaie->data, wpaie->datalen);
>> +
>> +	/* Return the total number of bytes added to the TLV buffer */
>> +	return sizeof(struct mrvl_ie_header) + wpaie->datalen;
>>   }
>>   
>>   /*
>> -- 
>> 2.34.1
>>
diff mbox series

Patch

diff --git a/drivers/net/wireless/marvell/libertas/cfg.c b/drivers/net/wireless/marvell/libertas/cfg.c
index 3e065cbb0af9..5cd78fefbe4c 100644
--- a/drivers/net/wireless/marvell/libertas/cfg.c
+++ b/drivers/net/wireless/marvell/libertas/cfg.c
@@ -416,10 +416,20 @@  static int lbs_add_cf_param_tlv(u8 *tlv)
 
 static int lbs_add_wpa_tlv(u8 *tlv, const u8 *ie, u8 ie_len)
 {
-	size_t tlv_len;
+	struct mrvl_ie_data *wpatlv = (struct mrvl_ie_data *)tlv;
+	const struct element *wpaie;
+
+	/* Find the first RSN or WPA IE to use */
+	wpaie = cfg80211_find_elem(WLAN_EID_RSN, ie, ie_len);
+	if (!wpaie)
+		wpaie = cfg80211_find_vendor_elem(WLAN_OUI_MICROSOFT,
+						  WLAN_OUI_TYPE_MICROSOFT_WPA,
+						  ie, ie_len);
+	if (!wpaie || wpaie->datalen > 128)
+		return 0;
 
 	/*
-	 * We need just convert an IE to an TLV. IEs use u8 for the header,
+	 * Convert the found IE to a TLV. IEs use u8 for the header,
 	 *   u8      type
 	 *   u8      len
 	 *   u8[]    data
@@ -428,14 +438,12 @@  static int lbs_add_wpa_tlv(u8 *tlv, const u8 *ie, u8 ie_len)
 	 *   __le16  len
 	 *   u8[]    data
 	 */
-	*tlv++ = *ie++;
-	*tlv++ = 0;
-	tlv_len = *tlv++ = *ie++;
-	*tlv++ = 0;
-	while (tlv_len--)
-		*tlv++ = *ie++;
-	/* the TLV is two bytes larger than the IE */
-	return ie_len + 2;
+	wpatlv->header.type = wpaie->id;
+	wpatlv->header.len = wpaie->datalen;
+	memcpy(wpatlv->data, wpaie->data, wpaie->datalen);
+
+	/* Return the total number of bytes added to the TLV buffer */
+	return sizeof(struct mrvl_ie_header) + wpaie->datalen;
 }
 
 /*