[PATCH v3 0/4] wifi: libertas: IE handling fixes

[PATCH v3 0/4] wifi: libertas: IE handling fixes

[Doug Brown]

This series implements two fixes for the libertas driver that restore
compatibility with modern wpa_supplicant versions, and adds (or at least
improves) support for WPS in the process.

1) Better handling of the RSN/WPA IE in association requests:
   The previous logic was always just grabbing the first one, and didn't
   handle multiple IEs properly, which wpa_supplicant adds nowadays.

2) Support for IEs in scan requests:
   Modern wpa_supplicant always adds an "extended capabilities" IE,
   which violates max_scan_ie_len in this driver. Go ahead and allow
   scan IEs, and handle WPS based on the info that Dan provided.

These changes have been tested on a Marvell PXA168-based device with a
Marvell 88W8686 Wi-Fi chipset. I've confirmed that with these changes
applied, modern wpa_supplicant versions connect properly and WPS also
works correctly (tested with "wpa_cli -i wlan0 wps_pbc any").

Changes since V2:
- Add missing cpu_to_le16 as suggested by Simon Horman

Changes since V1 (which was a single patch linked here [1]):

- Switch to cfg80211_find_*_elem when looking for specific IEs,
  resulting in cleaner/safer code.
- Use mrvl_ie_data struct for cleaner manipulation of TLV buffer, and
  fix capitalization of the "data" member to avoid checkpatch warnings.
- Implement idea suggested by Dan to change max_scan_ie_len to be
  nonzero and enable WPS support in probe requests while we're at it.
- Remove "Fixes:" tag; I'm not sure if it's still appropriate or not
  with it depending on the capitalization fix.
- Clarify comments.

[1] https://lore.kernel.org/all/20230102234714.169831-1-doug@schmorgal.com/

Doug Brown (4):
  wifi: libertas: fix capitalization in mrvl_ie_data struct
  wifi: libertas: only add RSN/WPA IE in lbs_add_wpa_tlv
  wifi: libertas: add new TLV type for WPS enrollee IE
  wifi: libertas: add support for WPS enrollee IE in probe requests

 drivers/net/wireless/marvell/libertas/cfg.c   | 76 +++++++++++++++----
 drivers/net/wireless/marvell/libertas/types.h |  3 +-
 2 files changed, 65 insertions(+), 14 deletions(-)

-- 
2.34.1

[PATCH v3 1/4] wifi: libertas: fix capitalization in mrvl_ie_data struct

[Doug Brown]

This struct is currently unused, but it will be used in future patches.
Fix the code style to not use camel case.

Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 drivers/net/wireless/marvell/libertas/types.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/libertas/types.h b/drivers/net/wireless/marvell/libertas/types.h
index cd4ceb6f885d..398e3272e85f 100644
--- a/drivers/net/wireless/marvell/libertas/types.h
+++ b/drivers/net/wireless/marvell/libertas/types.h
@@ -105,7 +105,7 @@ struct mrvl_ie_header {
 
 struct mrvl_ie_data {
 	struct mrvl_ie_header header;
-	u8 Data[1];
+	u8 data[1];
 } __packed;
 
 struct mrvl_ie_rates_param_set {
-- 
2.34.1

[PATCH v3 2/4] wifi: libertas: only add RSN/WPA IE in lbs_add_wpa_tlv

[Doug Brown]

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..3f35dc7a1d7d 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 = cpu_to_le16(wpaie->id);
+	wpatlv->header.len = cpu_to_le16(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;
 }
 
 /*
-- 
2.34.1

[PATCH v3 3/4] wifi: libertas: add new TLV type for WPS enrollee IE

[Doug Brown]

Add a define for the TLV type that will be used to add WPS enrollee
information to probe requests.

Suggested-by: Dan Williams <dcbw@redhat.com>
Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 drivers/net/wireless/marvell/libertas/types.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/marvell/libertas/types.h b/drivers/net/wireless/marvell/libertas/types.h
index 398e3272e85f..39deb2b8bc82 100644
--- a/drivers/net/wireless/marvell/libertas/types.h
+++ b/drivers/net/wireless/marvell/libertas/types.h
@@ -93,6 +93,7 @@ union ieee_phy_param_set {
 #define TLV_TYPE_TSFTIMESTAMP	    (PROPRIETARY_TLV_BASE_ID + 19)
 #define TLV_TYPE_RSSI_HIGH          (PROPRIETARY_TLV_BASE_ID + 22)
 #define TLV_TYPE_SNR_HIGH           (PROPRIETARY_TLV_BASE_ID + 23)
+#define TLV_TYPE_WPS_ENROLLEE       (PROPRIETARY_TLV_BASE_ID + 27)
 #define TLV_TYPE_AUTH_TYPE          (PROPRIETARY_TLV_BASE_ID + 31)
 #define TLV_TYPE_MESH_ID            (PROPRIETARY_TLV_BASE_ID + 37)
 #define TLV_TYPE_OLD_MESH_ID        (PROPRIETARY_TLV_BASE_ID + 291)
-- 
2.34.1

[PATCH v3 4/4] wifi: libertas: add support for WPS enrollee IE in probe requests

[Doug Brown]

Add compatibility with WPS by passing on WPS enrollee information in
probe requests. Ignore other IEs supplied in the scan request. This also
has the added benefit of restoring compatibility with newer
wpa_supplicant versions that always add scan IEs. Previously, with
max_scan_ie_len set to 0, scans would always fail.

Suggested-by: Dan Williams <dcbw@redhat.com>
Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 drivers/net/wireless/marvell/libertas/cfg.c | 48 +++++++++++++++++++--
 1 file changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas/cfg.c b/drivers/net/wireless/marvell/libertas/cfg.c
index 3f35dc7a1d7d..b700c213d10c 100644
--- a/drivers/net/wireless/marvell/libertas/cfg.c
+++ b/drivers/net/wireless/marvell/libertas/cfg.c
@@ -446,6 +446,41 @@ static int lbs_add_wpa_tlv(u8 *tlv, const u8 *ie, u8 ie_len)
 	return sizeof(struct mrvl_ie_header) + wpaie->datalen;
 }
 
+/* Add WPS enrollee TLV
+ */
+#define LBS_MAX_WPS_ENROLLEE_TLV_SIZE		\
+	(sizeof(struct mrvl_ie_header)		\
+	 + 256)
+
+static int lbs_add_wps_enrollee_tlv(u8 *tlv, const u8 *ie, size_t ie_len)
+{
+	struct mrvl_ie_data *wpstlv = (struct mrvl_ie_data *)tlv;
+	const struct element *wpsie;
+
+	/* Look for a WPS IE and add it to the probe request */
+	wpsie = cfg80211_find_vendor_elem(WLAN_OUI_MICROSOFT,
+					  WLAN_OUI_TYPE_MICROSOFT_WPS,
+					  ie, ie_len);
+	if (!wpsie)
+		return 0;
+
+	/* Convert the WPS IE to a TLV. The IE looks like this:
+	 *   u8      type (WLAN_EID_VENDOR_SPECIFIC)
+	 *   u8      len
+	 *   u8[]    data
+	 * but the TLV will look like this instead:
+	 *   __le16  type (TLV_TYPE_WPS_ENROLLEE)
+	 *   __le16  len
+	 *   u8[]    data
+	 */
+	wpstlv->header.type = cpu_to_le16(TLV_TYPE_WPS_ENROLLEE);
+	wpstlv->header.len = cpu_to_le16(wpsie->datalen);
+	memcpy(wpstlv->data, wpsie->data, wpsie->datalen);
+
+	/* Return the total number of bytes added to the TLV buffer */
+	return sizeof(struct mrvl_ie_header) + wpsie->datalen;
+}
+
 /*
  * Set Channel
  */
@@ -672,14 +707,15 @@ static int lbs_ret_scan(struct lbs_private *priv, unsigned long dummy,
 
 
 /*
- * Our scan command contains a TLV, consting of a SSID TLV, a channel list
- * TLV and a rates TLV. Determine the maximum size of them:
+ * Our scan command contains a TLV, consisting of a SSID TLV, a channel list
+ * TLV, a rates TLV, and an optional WPS IE. Determine the maximum size of them:
  */
 #define LBS_SCAN_MAX_CMD_SIZE			\
 	(sizeof(struct cmd_ds_802_11_scan)	\
 	 + LBS_MAX_SSID_TLV_SIZE		\
 	 + LBS_MAX_CHANNEL_LIST_TLV_SIZE	\
-	 + LBS_MAX_RATES_TLV_SIZE)
+	 + LBS_MAX_RATES_TLV_SIZE		\
+	 + LBS_MAX_WPS_ENROLLEE_TLV_SIZE)
 
 /*
  * Assumes priv->scan_req is initialized and valid
@@ -728,6 +764,11 @@ static void lbs_scan_worker(struct work_struct *work)
 	/* add rates TLV */
 	tlv += lbs_add_supported_rates_tlv(tlv);
 
+	/* add optional WPS enrollee TLV */
+	if (priv->scan_req->ie && priv->scan_req->ie_len)
+		tlv += lbs_add_wps_enrollee_tlv(tlv, priv->scan_req->ie,
+						priv->scan_req->ie_len);
+
 	if (priv->scan_channel < priv->scan_req->n_channels) {
 		cancel_delayed_work(&priv->scan_work);
 		if (netif_running(priv->dev))
@@ -2114,6 +2155,7 @@ int lbs_cfg_register(struct lbs_private *priv)
 	int ret;
 
 	wdev->wiphy->max_scan_ssids = 1;
+	wdev->wiphy->max_scan_ie_len = 256;
 	wdev->wiphy->signal_type = CFG80211_SIGNAL_TYPE_MBM;
 
 	wdev->wiphy->interface_modes =
-- 
2.34.1

RE: [PATCH v3 1/4] wifi: libertas: fix capitalization in mrvl_ie_data struct

[Ping-Ke Shih]

> -----Original Message-----
> From: Doug Brown <doug@schmorgal.com>
> Sent: Tuesday, January 17, 2023 4:21 AM
> To: Kalle Valo <kvalo@kernel.org>; David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>
> Cc: Dan Williams <dcbw@redhat.com>; Simon Horman <simon.horman@corigine.com>;
> libertas-dev@lists.infradead.org; linux-wireless@vger.kernel.org; netdev@vger.kernel.org; Doug Brown
> <doug@schmorgal.com>
> Subject: [PATCH v3 1/4] wifi: libertas: fix capitalization in mrvl_ie_data struct
> 
> This struct is currently unused, but it will be used in future patches.
> Fix the code style to not use camel case.
> 
> Signed-off-by: Doug Brown <doug@schmorgal.com>
> ---
>  drivers/net/wireless/marvell/libertas/types.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/marvell/libertas/types.h
> b/drivers/net/wireless/marvell/libertas/types.h
> index cd4ceb6f885d..398e3272e85f 100644
> --- a/drivers/net/wireless/marvell/libertas/types.h
> +++ b/drivers/net/wireless/marvell/libertas/types.h
> @@ -105,7 +105,7 @@ struct mrvl_ie_header {
> 
>  struct mrvl_ie_data {
>  	struct mrvl_ie_header header;
> -	u8 Data[1];
> +	u8 data[1];

data[]. see [1]

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays

>  } __packed;
> 
>  struct mrvl_ie_rates_param_set {
> --
> 2.34.1

Re: [PATCH v3 1/4] wifi: libertas: fix capitalization in mrvl_ie_data struct

[Doug Brown]

On 1/16/2023 7:39 PM, Ping-Ke Shih wrote:
> 
> 
>> -----Original Message-----
>> From: Doug Brown <doug@schmorgal.com>
>> Sent: Tuesday, January 17, 2023 4:21 AM
>> To: Kalle Valo <kvalo@kernel.org>; David S. Miller <davem@davemloft.net>; Eric Dumazet
>> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>
>> Cc: Dan Williams <dcbw@redhat.com>; Simon Horman <simon.horman@corigine.com>;
>> libertas-dev@lists.infradead.org; linux-wireless@vger.kernel.org; netdev@vger.kernel.org; Doug Brown
>> <doug@schmorgal.com>
>> Subject: [PATCH v3 1/4] wifi: libertas: fix capitalization in mrvl_ie_data struct
>>
>> This struct is currently unused, but it will be used in future patches.
>> Fix the code style to not use camel case.
>>
>> Signed-off-by: Doug Brown <doug@schmorgal.com>
>> ---
>>   drivers/net/wireless/marvell/libertas/types.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/marvell/libertas/types.h
>> b/drivers/net/wireless/marvell/libertas/types.h
>> index cd4ceb6f885d..398e3272e85f 100644
>> --- a/drivers/net/wireless/marvell/libertas/types.h
>> +++ b/drivers/net/wireless/marvell/libertas/types.h
>> @@ -105,7 +105,7 @@ struct mrvl_ie_header {
>>
>>   struct mrvl_ie_data {
>>   	struct mrvl_ie_header header;
>> -	u8 Data[1];
>> +	u8 data[1];
> 
> data[]. see [1]
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays

Hi Ping-Ke,

Thanks for the link. There are several other cases of this same syntax
for flexible trailing arrays in this file, so I will update this patch
in the next version of the series to fix them all.

> 
>>   } __packed;
>>
>>   struct mrvl_ie_rates_param_set {
>> --
>> 2.34.1
> 

Re: [PATCH v3 2/4] wifi: libertas: only add RSN/WPA IE in lbs_add_wpa_tlv

[Simon Horman]

On Mon, Jan 16, 2023 at 12:21:24PM -0800, Doug Brown wrote:
> [You don't often get email from doug@schmorgal.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> 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..3f35dc7a1d7d 100644
> --- a/drivers/net/wireless/marvell/libertas/cfg.c
> +++ b/drivers/net/wireless/marvell/libertas/cfg.c

...

> @@ -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 = cpu_to_le16(wpaie->id);
> +       wpatlv->header.len = cpu_to_le16(wpaie->datalen);
> +       memcpy(wpatlv->data, wpaie->data, wpaie->datalen);

Hi Doug,

Thanks for fixing the endiness issues with cpu_to_le16()
This part looks good to me now. Likewise for patch 4/4.

One suggestion I have, which is probably taking things to far,
is a helper for what seems to be repeated code-pattern.
But I don't feel strongly about that.

> +
> +       /* Return the total number of bytes added to the TLV buffer */
> +       return sizeof(struct mrvl_ie_header) + wpaie->datalen;
>  }
> 
>  /*
> --
> 2.34.1
> 

Re: [PATCH v3 4/4] wifi: libertas: add support for WPS enrollee IE in probe requests

[Dan Williams]

On Mon, 2023-01-16 at 12:21 -0800, Doug Brown wrote:
> Add compatibility with WPS by passing on WPS enrollee information in
> probe requests. Ignore other IEs supplied in the scan request. This
> also
> has the added benefit of restoring compatibility with newer
> wpa_supplicant versions that always add scan IEs. Previously, with
> max_scan_ie_len set to 0, scans would always fail.
> 
> Suggested-by: Dan Williams <dcbw@redhat.com>
> Signed-off-by: Doug Brown <doug@schmorgal.com>

Reviewed-by: Dan Williams <dcbw@redhat.com>

(don't know if I can/should ack anything anymore, given I haven't
touched the driver in like 4 years...)

> ---
>  drivers/net/wireless/marvell/libertas/cfg.c | 48
> +++++++++++++++++++--
>  1 file changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/libertas/cfg.c
> b/drivers/net/wireless/marvell/libertas/cfg.c
> index 3f35dc7a1d7d..b700c213d10c 100644
> --- a/drivers/net/wireless/marvell/libertas/cfg.c
> +++ b/drivers/net/wireless/marvell/libertas/cfg.c
> @@ -446,6 +446,41 @@ static int lbs_add_wpa_tlv(u8 *tlv, const u8
> *ie, u8 ie_len)
>         return sizeof(struct mrvl_ie_header) + wpaie->datalen;
>  }
>  
> +/* Add WPS enrollee TLV
> + */
> +#define LBS_MAX_WPS_ENROLLEE_TLV_SIZE          \
> +       (sizeof(struct mrvl_ie_header)          \
> +        + 256)
> +
> +static int lbs_add_wps_enrollee_tlv(u8 *tlv, const u8 *ie, size_t
> ie_len)
> +{
> +       struct mrvl_ie_data *wpstlv = (struct mrvl_ie_data *)tlv;
> +       const struct element *wpsie;
> +
> +       /* Look for a WPS IE and add it to the probe request */
> +       wpsie = cfg80211_find_vendor_elem(WLAN_OUI_MICROSOFT,
> +                                        
> WLAN_OUI_TYPE_MICROSOFT_WPS,
> +                                         ie, ie_len);
> +       if (!wpsie)
> +               return 0;
> +
> +       /* Convert the WPS IE to a TLV. The IE looks like this:
> +        *   u8      type (WLAN_EID_VENDOR_SPECIFIC)
> +        *   u8      len
> +        *   u8[]    data
> +        * but the TLV will look like this instead:
> +        *   __le16  type (TLV_TYPE_WPS_ENROLLEE)
> +        *   __le16  len
> +        *   u8[]    data
> +        */
> +       wpstlv->header.type = cpu_to_le16(TLV_TYPE_WPS_ENROLLEE);
> +       wpstlv->header.len = cpu_to_le16(wpsie->datalen);
> +       memcpy(wpstlv->data, wpsie->data, wpsie->datalen);
> +
> +       /* Return the total number of bytes added to the TLV buffer
> */
> +       return sizeof(struct mrvl_ie_header) + wpsie->datalen;
> +}
> +
>  /*
>   * Set Channel
>   */
> @@ -672,14 +707,15 @@ static int lbs_ret_scan(struct lbs_private
> *priv, unsigned long dummy,
>  
>  
>  /*
> - * Our scan command contains a TLV, consting of a SSID TLV, a
> channel list
> - * TLV and a rates TLV. Determine the maximum size of them:
> + * Our scan command contains a TLV, consisting of a SSID TLV, a
> channel list
> + * TLV, a rates TLV, and an optional WPS IE. Determine the maximum
> size of them:
>   */
>  #define LBS_SCAN_MAX_CMD_SIZE                  \
>         (sizeof(struct cmd_ds_802_11_scan)      \
>          + LBS_MAX_SSID_TLV_SIZE                \
>          + LBS_MAX_CHANNEL_LIST_TLV_SIZE        \
> -        + LBS_MAX_RATES_TLV_SIZE)
> +        + LBS_MAX_RATES_TLV_SIZE               \
> +        + LBS_MAX_WPS_ENROLLEE_TLV_SIZE)
>  
>  /*
>   * Assumes priv->scan_req is initialized and valid
> @@ -728,6 +764,11 @@ static void lbs_scan_worker(struct work_struct
> *work)
>         /* add rates TLV */
>         tlv += lbs_add_supported_rates_tlv(tlv);
>  
> +       /* add optional WPS enrollee TLV */
> +       if (priv->scan_req->ie && priv->scan_req->ie_len)
> +               tlv += lbs_add_wps_enrollee_tlv(tlv, priv->scan_req-
> >ie,
> +                                               priv->scan_req-
> >ie_len);
> +
>         if (priv->scan_channel < priv->scan_req->n_channels) {
>                 cancel_delayed_work(&priv->scan_work);
>                 if (netif_running(priv->dev))
> @@ -2114,6 +2155,7 @@ int lbs_cfg_register(struct lbs_private *priv)
>         int ret;
>  
>         wdev->wiphy->max_scan_ssids = 1;
> +       wdev->wiphy->max_scan_ie_len = 256;
>         wdev->wiphy->signal_type = CFG80211_SIGNAL_TYPE_MBM;
>  
>         wdev->wiphy->interface_modes =

Re: [PATCH v3 2/4] wifi: libertas: only add RSN/WPA IE in lbs_add_wpa_tlv

[Doug Brown]

On 1/17/2023 12:59 AM, Simon Horman wrote:
> On Mon, Jan 16, 2023 at 12:21:24PM -0800, Doug Brown wrote:
>> [You don't often get email from doug@schmorgal.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>
>> 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..3f35dc7a1d7d 100644
>> --- a/drivers/net/wireless/marvell/libertas/cfg.c
>> +++ b/drivers/net/wireless/marvell/libertas/cfg.c
> 
> ...
> 
>> @@ -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 = cpu_to_le16(wpaie->id);
>> +       wpatlv->header.len = cpu_to_le16(wpaie->datalen);
>> +       memcpy(wpatlv->data, wpaie->data, wpaie->datalen);
> 
> Hi Doug,
> 
> Thanks for fixing the endiness issues with cpu_to_le16()
> This part looks good to me now. Likewise for patch 4/4.
> 
> One suggestion I have, which is probably taking things to far,
> is a helper for what seems to be repeated code-pattern.
> But I don't feel strongly about that.

Thanks Simon. Is this basically what you're suggesting for a helper?

static int lbs_add_ie_tlv(u8 *tlvbuf, const struct element *ie, u16 tlvtype)
{
	struct mrvl_ie_data *tlv = (struct mrvl_ie_data *)tlvbuf;
	tlv->header.type = cpu_to_le16(tlvtype);
	tlv->header.len = cpu_to_le16(ie->datalen);
	memcpy(tlv->data, ie->data, ie->datalen);
	return sizeof(struct mrvl_ie_header) + ie->datalen;
}

And then in the two functions where I'm doing that, at the bottom:

return lbs_add_ie_tlv(tlv, wpaie, wpaie->id);
return lbs_add_ie_tlv(tlv, wpsie, TLV_TYPE_WPS_ENROLLEE);

I could definitely do that to avoid repeating the chunk of code that
fills out the struct in the two functions. A lot of the other
lbs_add_*_tlv functions follow a similar pattern of setting up a struct
pointer and filling out the header, so I don't think it's too crazy to
just repeat the code twice. On the other hand, the example above does
look pretty darn clean. I don't feel strongly either way myself.

> 
>> +
>> +       /* Return the total number of bytes added to the TLV buffer */
>> +       return sizeof(struct mrvl_ie_header) + wpaie->datalen;
>>   }
>>
>>   /*
>> --
>> 2.34.1
>>

Re: [PATCH v3 2/4] wifi: libertas: only add RSN/WPA IE in lbs_add_wpa_tlv

[Simon Horman]

On Tue, Jan 17, 2023 at 10:35:56PM -0800, Doug Brown wrote:
> On 1/17/2023 12:59 AM, Simon Horman wrote:
> > On Mon, Jan 16, 2023 at 12:21:24PM -0800, Doug Brown wrote:
> > > [You don't often get email from doug@schmorgal.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> > > 
> > > 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..3f35dc7a1d7d 100644
> > > --- a/drivers/net/wireless/marvell/libertas/cfg.c
> > > +++ b/drivers/net/wireless/marvell/libertas/cfg.c
> > 
> > ...
> > 
> > > @@ -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 = cpu_to_le16(wpaie->id);
> > > +       wpatlv->header.len = cpu_to_le16(wpaie->datalen);
> > > +       memcpy(wpatlv->data, wpaie->data, wpaie->datalen);
> > 
> > Hi Doug,
> > 
> > Thanks for fixing the endiness issues with cpu_to_le16()
> > This part looks good to me now. Likewise for patch 4/4.
> > 
> > One suggestion I have, which is probably taking things to far,
> > is a helper for what seems to be repeated code-pattern.
> > But I don't feel strongly about that.
> 
> Thanks Simon. Is this basically what you're suggesting for a helper?
> 
> static int lbs_add_ie_tlv(u8 *tlvbuf, const struct element *ie, u16 tlvtype)
> {
> 	struct mrvl_ie_data *tlv = (struct mrvl_ie_data *)tlvbuf;
> 	tlv->header.type = cpu_to_le16(tlvtype);
> 	tlv->header.len = cpu_to_le16(ie->datalen);
> 	memcpy(tlv->data, ie->data, ie->datalen);
> 	return sizeof(struct mrvl_ie_header) + ie->datalen;
> }
> 
> And then in the two functions where I'm doing that, at the bottom:
> 
> return lbs_add_ie_tlv(tlv, wpaie, wpaie->id);
> return lbs_add_ie_tlv(tlv, wpsie, TLV_TYPE_WPS_ENROLLEE);
> 
> I could definitely do that to avoid repeating the chunk of code that
> fills out the struct in the two functions. A lot of the other
> lbs_add_*_tlv functions follow a similar pattern of setting up a struct
> pointer and filling out the header, so I don't think it's too crazy to
> just repeat the code twice. On the other hand, the example above does
> look pretty darn clean. I don't feel strongly either way myself.

Hi Doug,

yes, I was thinking about something like that.
And wondering if it might be reused elsewhere (in the same file).

But again, I don't feel strongly about this.
So perhaps it's something to consider in future.