[PATCH 1/3] [v4] wifi: mwifiex: prefer strscpy() over strlcpy()

[PATCH 1/3] [v4] wifi: mwifiex: prefer strscpy() over strlcpy()

[Dmitry Antipov]

Prefer 'strscpy()' over 'strlcpy()' in 'mwifiex_init_hw_fw()'.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v4: simplify to drop strlcpy() only (Brian Norris)
---
 drivers/net/wireless/marvell/mwifiex/main.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index ea22a08e6c08..64512b00e8b5 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -724,14 +724,9 @@ static int mwifiex_init_hw_fw(struct mwifiex_adapter *adapter,
 	/* Override default firmware with manufacturing one if
 	 * manufacturing mode is enabled
 	 */
-	if (mfg_mode) {
-		if (strlcpy(adapter->fw_name, MFG_FIRMWARE,
-			    sizeof(adapter->fw_name)) >=
-			    sizeof(adapter->fw_name)) {
-			pr_err("%s: fw_name too long!\n", __func__);
-			return -1;
-		}
-	}
+	if (mfg_mode)
+		strscpy(adapter->fw_name, MFG_FIRMWARE,
+			sizeof(adapter->fw_name));
 
 	if (req_fw_nowait) {
 		ret = request_firmware_nowait(THIS_MODULE, 1, adapter->fw_name,
-- 
2.41.0

[PATCH 2/3] [v4] wifi: mwifiex: fix fortify warning

[Dmitry Antipov]

When compiling with gcc 13.1 and CONFIG_FORTIFY_SOURCE=y,
I've noticed the following:

In function ‘fortify_memcpy_chk’,
    inlined from ‘mwifiex_construct_tdls_action_frame’ at drivers/net/wireless/marvell/mwifiex/tdls.c:765:3,
    inlined from ‘mwifiex_send_tdls_action_frame’ at drivers/net/wireless/marvell/mwifiex/tdls.c:856:6:
./include/linux/fortify-string.h:529:25: warning: call to ‘__read_overflow2_field’
declared with attribute warning: detected read beyond size of field (2nd parameter);
maybe use struct_group()? [-Wattribute-warning]
  529 |                         __read_overflow2_field(q_size_field, size);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The compiler actually complains on:

memmove(pos + ETH_ALEN, &mgmt->u.action.category,
	sizeof(mgmt->u.action.u.tdls_discover_resp));

and it happens because the fortification logic interprets this
as an attempt to overread 1-byte 'u.action.category' member of
'struct ieee80211_mgmt'. To silence this warning, it's enough
to pass an address of 'u.action' itself instead of an address
of its first member.

This also fixes an improper usage of 'sizeof()'. Since 'skb' is
extended with 'sizeof(mgmt->u.action.u.tdls_discover_resp) + 1'
bytes (where 1 is actually 'sizeof(mgmt->u.action.category)'),
I assume that the same number of bytes should be copied.

Suggested-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v4: fix memmove() size calculation (Brian Norris)
---
 drivers/net/wireless/marvell/mwifiex/tdls.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/tdls.c b/drivers/net/wireless/marvell/mwifiex/tdls.c
index 97bb87c3676b..6c60621b6ccc 100644
--- a/drivers/net/wireless/marvell/mwifiex/tdls.c
+++ b/drivers/net/wireless/marvell/mwifiex/tdls.c
@@ -735,6 +735,7 @@ mwifiex_construct_tdls_action_frame(struct mwifiex_private *priv,
 	int ret;
 	u16 capab;
 	struct ieee80211_ht_cap *ht_cap;
+	unsigned int extra;
 	u8 radio, *pos;
 
 	capab = priv->curr_bss_params.bss_descriptor.cap_info_bitmap;
@@ -753,7 +754,10 @@ mwifiex_construct_tdls_action_frame(struct mwifiex_private *priv,
 
 	switch (action_code) {
 	case WLAN_PUB_ACTION_TDLS_DISCOVER_RES:
-		skb_put(skb, sizeof(mgmt->u.action.u.tdls_discover_resp) + 1);
+		/* See the layout of 'struct ieee80211_mgmt'. */
+		extra = sizeof(mgmt->u.action.u.tdls_discover_resp) +
+			sizeof(mgmt->u.action.category);
+		skb_put(skb, extra);
 		mgmt->u.action.category = WLAN_CATEGORY_PUBLIC;
 		mgmt->u.action.u.tdls_discover_resp.action_code =
 					      WLAN_PUB_ACTION_TDLS_DISCOVER_RES;
@@ -762,8 +766,7 @@ mwifiex_construct_tdls_action_frame(struct mwifiex_private *priv,
 		mgmt->u.action.u.tdls_discover_resp.capability =
 							     cpu_to_le16(capab);
 		/* move back for addr4 */
-		memmove(pos + ETH_ALEN, &mgmt->u.action.category,
-			sizeof(mgmt->u.action.u.tdls_discover_resp));
+		memmove(pos + ETH_ALEN, &mgmt->u.action, extra);
 		/* init address 4 */
 		eth_broadcast_addr(pos);
 
-- 
2.41.0

[PATCH 3/3] [v4] wifi: mwifiex: drop BUG_ON() from TX error handling

[Dmitry Antipov]

Remove 'BUG_ON()' from 'mwifiex_process_sta_txpd()' and
'mwifiex_process_uap_txpd()'. In case of insufficient
headrom, issue warning and return NULL, which should be
gracefully handled in 'mwifiex_process_tx()'. Also mark
error handling branches with 'unlikely()' and adjust
format specifiers to match actual 'unsigned int' type.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v4: initial version to match series
---
 drivers/net/wireless/marvell/mwifiex/sta_tx.c   | 13 +++++++++----
 drivers/net/wireless/marvell/mwifiex/uap_txrx.c | 13 +++++++++----
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sta_tx.c b/drivers/net/wireless/marvell/mwifiex/sta_tx.c
index 13c0e67ededf..d43f6ec1ad37 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_tx.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_tx.c
@@ -39,14 +39,19 @@ void *mwifiex_process_sta_txpd(struct mwifiex_private *priv,
 	u16 pkt_type, pkt_offset;
 	int hroom = adapter->intf_hdr_len;
 
-	if (!skb->len) {
+	if (unlikely(!skb->len)) {
 		mwifiex_dbg(adapter, ERROR,
-			    "Tx: bad packet length: %d\n", skb->len);
+			    "Tx: bad packet length: %u\n", skb->len);
 		tx_info->status_code = -1;
 		return skb->data;
 	}
-
-	BUG_ON(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN);
+	if (unlikely(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN)) {
+		mwifiex_dbg(adapter, ERROR,
+			    "Tx: insufficient skb headroom: %u\n",
+			    skb_headroom(skb));
+		tx_info->status_code = -1;
+		return NULL;
+	}
 
 	pkt_type = mwifiex_is_skb_mgmt_frame(skb) ? PKT_TYPE_MGMT : 0;
 
diff --git a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
index e495f7eaea03..b27266742795 100644
--- a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
+++ b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
@@ -452,14 +452,19 @@ void *mwifiex_process_uap_txpd(struct mwifiex_private *priv,
 	u16 pkt_type, pkt_offset;
 	int hroom = adapter->intf_hdr_len;
 
-	if (!skb->len) {
+	if (unlikely(!skb->len)) {
 		mwifiex_dbg(adapter, ERROR,
-			    "Tx: bad packet length: %d\n", skb->len);
+			    "Tx: bad packet length: %u\n", skb->len);
 		tx_info->status_code = -1;
 		return skb->data;
 	}
-
-	BUG_ON(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN);
+	if (unlikely(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN)) {
+		mwifiex_dbg(adapter, ERROR,
+			    "Tx: insufficient skb headroom: %u\n",
+			    skb_headroom(skb));
+		tx_info->status_code = -1;
+		return NULL;
+	}
 
 	pkt_type = mwifiex_is_skb_mgmt_frame(skb) ? PKT_TYPE_MGMT : 0;
 
-- 
2.41.0

Re: [PATCH 3/3] [v4] wifi: mwifiex: drop BUG_ON() from TX error handling

[Simon Horman]

On Thu, Jun 29, 2023 at 11:51:02AM +0300, Dmitry Antipov wrote:
> Remove 'BUG_ON()' from 'mwifiex_process_sta_txpd()' and
> 'mwifiex_process_uap_txpd()'. In case of insufficient
> headrom, issue warning and return NULL, which should be

Hi Dimitry,

a minor nit courtesy of checkpatch.pl --codespell: headrom -> headroom

...

Re: [PATCH 1/3] [v4] wifi: mwifiex: prefer strscpy() over strlcpy()

[Brian Norris]

On Thu, Jun 29, 2023 at 11:51:00AM +0300, Dmitry Antipov wrote:
> Prefer 'strscpy()' over 'strlcpy()' in 'mwifiex_init_hw_fw()'.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> v4: simplify to drop strlcpy() only (Brian Norris)

Reviewed-by: Brian Norris <briannorris@chromium.org>

Re: [PATCH 2/3] [v4] wifi: mwifiex: fix fortify warning

[Brian Norris]

On Thu, Jun 29, 2023 at 11:51:01AM +0300, Dmitry Antipov wrote:
[...]
> This also fixes an improper usage of 'sizeof()'. Since 'skb' is
> extended with 'sizeof(mgmt->u.action.u.tdls_discover_resp) + 1'
> bytes (where 1 is actually 'sizeof(mgmt->u.action.category)'),
> I assume that the same number of bytes should be copied.
> 
> Suggested-by: Brian Norris <briannorris@chromium.org>

I don't believe I actually *suggested* the change; I just highlighted
that the size looked sketchy in the original code. :)

But your change does look correct, and I don't see how we could possibly
*want* to be off by 1 here, so:

Reviewed-by: Brian Norris <briannorris@chromium.org>

> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> v4: fix memmove() size calculation (Brian Norris)
> ---
>  drivers/net/wireless/marvell/mwifiex/tdls.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Re: [PATCH 3/3] [v4] wifi: mwifiex: drop BUG_ON() from TX error handling

[Brian Norris]

On Thu, Jun 29, 2023 at 11:51:02AM +0300, Dmitry Antipov wrote:
> Remove 'BUG_ON()' from 'mwifiex_process_sta_txpd()' and
> 'mwifiex_process_uap_txpd()'. In case of insufficient
> headrom, issue warning and return NULL, which should be
> gracefully handled in 'mwifiex_process_tx()'. Also mark
> error handling branches with 'unlikely()' and adjust
> format specifiers to match actual 'unsigned int' type.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> v4: initial version to match series
> ---
>  drivers/net/wireless/marvell/mwifiex/sta_tx.c   | 13 +++++++++----
>  drivers/net/wireless/marvell/mwifiex/uap_txrx.c | 13 +++++++++----
>  2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_tx.c b/drivers/net/wireless/marvell/mwifiex/sta_tx.c
> index 13c0e67ededf..d43f6ec1ad37 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_tx.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_tx.c
> @@ -39,14 +39,19 @@ void *mwifiex_process_sta_txpd(struct mwifiex_private *priv,
>  	u16 pkt_type, pkt_offset;
>  	int hroom = adapter->intf_hdr_len;
>  
> -	if (!skb->len) {
> +	if (unlikely(!skb->len)) {
>  		mwifiex_dbg(adapter, ERROR,
> -			    "Tx: bad packet length: %d\n", skb->len);
> +			    "Tx: bad packet length: %u\n", skb->len);
>  		tx_info->status_code = -1;
>  		return skb->data;
>  	}
> -
> -	BUG_ON(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN);
> +	if (unlikely(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN)) {
> +		mwifiex_dbg(adapter, ERROR,
> +			    "Tx: insufficient skb headroom: %u\n",
> +			    skb_headroom(skb));
> +		tx_info->status_code = -1;
> +		return NULL;

I'm not sure why this return (NULL) should be different than the one for
skb->len==0 (skb->data). mwifiex_process_tx() has...weird handling for
both.

For NULL, we fall into a default (ret==-1) case where the error message
is wrong ("mwifiex_write_data_async failed: ...").

For non-NULL skb->data, we still try to queue or transmit the
skb...which seems wrong.

I think they should both be returning NULL, and mwifiex_process_tx()
should improve its error handling to more explicitly handle that case,
instead of printing the wrong error message.

(Now, I expect neither failure cases are actually exercised in practice,
which makes most of this moot...)

I'm also not sure why this is part of the same series as the others.

Brian

> +	}
>  
>  	pkt_type = mwifiex_is_skb_mgmt_frame(skb) ? PKT_TYPE_MGMT : 0;
>  
> diff --git a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
> index e495f7eaea03..b27266742795 100644
> --- a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
> +++ b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
> @@ -452,14 +452,19 @@ void *mwifiex_process_uap_txpd(struct mwifiex_private *priv,
>  	u16 pkt_type, pkt_offset;
>  	int hroom = adapter->intf_hdr_len;
>  
> -	if (!skb->len) {
> +	if (unlikely(!skb->len)) {
>  		mwifiex_dbg(adapter, ERROR,
> -			    "Tx: bad packet length: %d\n", skb->len);
> +			    "Tx: bad packet length: %u\n", skb->len);
>  		tx_info->status_code = -1;
>  		return skb->data;
>  	}
> -
> -	BUG_ON(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN);
> +	if (unlikely(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN)) {
> +		mwifiex_dbg(adapter, ERROR,
> +			    "Tx: insufficient skb headroom: %u\n",
> +			    skb_headroom(skb));
> +		tx_info->status_code = -1;
> +		return NULL;
> +	}
>  
>  	pkt_type = mwifiex_is_skb_mgmt_frame(skb) ? PKT_TYPE_MGMT : 0;
>  
> -- 
> 2.41.0
> 

Re: [PATCH 1/3] [v4] wifi: mwifiex: prefer strscpy() over strlcpy()

[Kalle Valo]

Dmitry Antipov <dmantipov@yandex.ru> wrote:

> Prefer 'strscpy()' over 'strlcpy()' in 'mwifiex_init_hw_fw()'.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> Reviewed-by: Brian Norris <briannorris@chromium.org>

2 patches applied to wireless-next.git, thanks.

caf9ead2c7d0 wifi: mwifiex: prefer strscpy() over strlcpy()
dcce94b80a95 wifi: mwifiex: fix fortify warning

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20230629085115.180499-1-dmantipov@yandex.ru/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches