[PATCH 1/2] [v2] wifi: mwifiex: avoid strlcpy() and use strscpy() where appropriate

[PATCH 1/2] [v2] wifi: mwifiex: avoid strlcpy() and use strscpy() where appropriate

[Dmitry Antipov]

Prefer 'strscpy()' over unsafe 'strlcpy()' and 'strcpy()' in
'mwifiex_init_hw_fw()' and 'mwifiex_register_dev()', respectively.
All other calls to 'strcpy(adapter->name, ...)' should be safe
because the firmware name is a compile-time constant of known
length and so guaranteed to fit into a destination buffer.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 drivers/net/wireless/marvell/mwifiex/main.c | 11 +++--------
 drivers/net/wireless/marvell/mwifiex/sdio.c |  4 +++-
 2 files changed, 6 insertions(+), 9 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,
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index a24bd40dd41a..a5d3128d7922 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -2483,7 +2483,9 @@ static int mwifiex_register_dev(struct mwifiex_adapter *adapter)
 		if ((val & card->reg->host_strap_mask) == card->reg->host_strap_value)
 			firmware = card->firmware_sdiouart;
 	}
-	strcpy(adapter->fw_name, firmware);
+	ret = strscpy(adapter->fw_name, firmware, sizeof(adapter->fw_name));
+	if (ret < 0)
+		return ret;
 
 	if (card->fw_dump_enh) {
 		adapter->mem_type_mapping_tbl = generic_mem_type_map;
-- 
2.41.0

[PATCH 2/2] [v2] 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.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 drivers/net/wireless/marvell/mwifiex/tdls.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/tdls.c b/drivers/net/wireless/marvell/mwifiex/tdls.c
index 97bb87c3676b..5a2941965757 100644
--- a/drivers/net/wireless/marvell/mwifiex/tdls.c
+++ b/drivers/net/wireless/marvell/mwifiex/tdls.c
@@ -762,7 +762,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,
+		memmove(pos + ETH_ALEN, &mgmt->u.action,
 			sizeof(mgmt->u.action.u.tdls_discover_resp));
 		/* init address 4 */
 		eth_broadcast_addr(pos);
-- 
2.41.0

Re: [PATCH 1/2] [v2] wifi: mwifiex: avoid strlcpy() and use strscpy() where appropriate

[Brian Norris]

On Tue, Jun 20, 2023 at 01:07:36PM +0300, Dmitry Antipov wrote:
> Prefer 'strscpy()' over unsafe 'strlcpy()' and 'strcpy()' in
> 'mwifiex_init_hw_fw()' and 'mwifiex_register_dev()', respectively.
> All other calls to 'strcpy(adapter->name, ...)' should be safe
> because the firmware name is a compile-time constant of known
> length and so guaranteed to fit into a destination buffer.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
>  drivers/net/wireless/marvell/mwifiex/main.c | 11 +++--------
>  drivers/net/wireless/marvell/mwifiex/sdio.c |  4 +++-
>  2 files changed, 6 insertions(+), 9 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));

I'm not sure how a compile-time constant makes this "unsafe" at all, but
if you feel the need to change this, then sure, this works too.

>  
>  	if (req_fw_nowait) {
>  		ret = request_firmware_nowait(THIS_MODULE, 1, adapter->fw_name,
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index a24bd40dd41a..a5d3128d7922 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -2483,7 +2483,9 @@ static int mwifiex_register_dev(struct mwifiex_adapter *adapter)
>  		if ((val & card->reg->host_strap_mask) == card->reg->host_strap_value)
>  			firmware = card->firmware_sdiouart;
>  	}
> -	strcpy(adapter->fw_name, firmware);
> +	ret = strscpy(adapter->fw_name, firmware, sizeof(adapter->fw_name));

FWIW, this 'firmware' pointer is all derived from compile-time constants
too. So the commit messages seems misleading ("all other calls [...]
should be safe" --> well, *all* calls are safe). But the changes are all
fine, so:

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

> +	if (ret < 0)
> +		return ret;
>  
>  	if (card->fw_dump_enh) {
>  		adapter->mem_type_mapping_tbl = generic_mem_type_map;
> -- 
> 2.41.0
> 

Re: [PATCH 2/2] [v2] wifi: mwifiex: fix fortify warningg

[Brian Norris]

On Tue, Jun 20, 2023 at 01:07:37PM +0300, Dmitry Antipov wrote:
> 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.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
>  drivers/net/wireless/marvell/mwifiex/tdls.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/tdls.c b/drivers/net/wireless/marvell/mwifiex/tdls.c
> index 97bb87c3676b..5a2941965757 100644
> --- a/drivers/net/wireless/marvell/mwifiex/tdls.c
> +++ b/drivers/net/wireless/marvell/mwifiex/tdls.c
> @@ -762,7 +762,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,
> +		memmove(pos + ETH_ALEN, &mgmt->u.action,
>  			sizeof(mgmt->u.action.u.tdls_discover_resp));

This invocation seems a bit suspect, as it uses a 'sizeof' of a field
that doesn't match the actual pointer (it's off by 1 byte), but that's
not your fault. I suppose it's no wonder we had so many problems with
TDLS support on mwifiex...

Anyway, the refactor looks fine:

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

>  		/* init address 4 */
>  		eth_broadcast_addr(pos);
> -- 
> 2.41.0
> 

Re: [PATCH 1/2] [v2] wifi: mwifiex: avoid strlcpy() and use strscpy() where appropriate

[Dmitry Antipov]

On 6/20/23 19:08, Brian Norris wrote:

> I'm not sure how a compile-time constant makes this "unsafe" at all, but
> if you feel the need to change this, then sure, this works too.

The only reason is to avoid strlcpy() which is now considered deprecated.

> FWIW, this 'firmware' pointer is all derived from compile-time constants
> too. So the commit messages seems misleading ("all other calls [...]
> should be safe" --> well, *all* calls are safe).

Indeed. So I think we can stay with strcpy() everywhere except strlcpy() to strscpy() replacement
(just to follow https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy rather than
to fix something).

Dmitry

Re: [PATCH 2/2] [v2] wifi: mwifiex: fix fortify warningg

[Dmitry Antipov]

On 6/20/23 19:26, Brian Norris wrote:

> This invocation seems a bit suspect, as it uses a 'sizeof' of a field
> that doesn't match the actual pointer (it's off by 1 byte), but that's
> not your fault. I suppose it's no wonder we had so many problems with
> TDLS support on mwifiex...

Hm, ieee80211_prep_tdls_direct() seems takes this byte into account. But
do you know why 'u.action.u.tdls_discover_resp' is ended with a flexible
array, e.g.:

struct {
	u8 action_code;
	u8 dialog_token;
	__le16 capability;
	u8 variable[0];
} __packed tdls_discover_resp;

Dmitry

Re: [PATCH 1/2] [v2] wifi: mwifiex: avoid strlcpy() and use strscpy() where appropriate

[Brian Norris]

On Wed, Jun 21, 2023 at 11:08:46AM +0300, Dmitry Antipov wrote:
> On 6/20/23 19:08, Brian Norris wrote:
> 
> > I'm not sure how a compile-time constant makes this "unsafe" at all, but
> > if you feel the need to change this, then sure, this works too.
> 
> The only reason is to avoid strlcpy() which is now considered deprecated.

Sure, OK.

> > FWIW, this 'firmware' pointer is all derived from compile-time constants
> > too. So the commit messages seems misleading ("all other calls [...]
> > should be safe" --> well, *all* calls are safe).
> 
> Indeed. So I think we can stay with strcpy() everywhere except strlcpy() to strscpy() replacement
> (just to follow https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy rather than
> to fix something).

That works too. It's cool to drop stcrpy() anyway though, since it's
really just a feature of a poor language (C) that we have to reason
about whether any given string operation is "safe" or not. I was just
noting that your commit message reasoning was slightly off.

Brian

Re: [PATCH 2/2] [v2] wifi: mwifiex: fix fortify warningg

[Brian Norris]

On Wed, Jun 21, 2023 at 11:32:25AM +0300, Dmitry Antipov wrote:
> On 6/20/23 19:26, Brian Norris wrote:
> 
> > This invocation seems a bit suspect, as it uses a 'sizeof' of a field
> > that doesn't match the actual pointer (it's off by 1 byte), but that's
> > not your fault. I suppose it's no wonder we had so many problems with
> > TDLS support on mwifiex...
> 
> Hm, ieee80211_prep_tdls_direct() seems takes this byte into account.

Presumably it's part of the standard packet format. (I haven't
checked.) But in this case, we're talking about the firmware format that
Marvell firmware expects -- which isn't documented at all. Usually it's
at least related to the IEEE spec, but it isn't guaranteed to be laid
out exactly the same.

BTW, mwifiex doesn't actually use those ieee8021_*() functions for the
most part, because it's not a mac80211 driver.

> But
> do you know why 'u.action.u.tdls_discover_resp' is ended with a flexible
> array, e.g.:
> 
> struct {
> 	u8 action_code;
> 	u8 dialog_token;
> 	__le16 capability;
> 	u8 variable[0];
> } __packed tdls_discover_resp;

Not without more guess-based investigation. My poking around this driver
is more often based on code reading and problem investigation, not based
on any private knowledge of the mwifiex firmware or hardware.

But my guess is that it's supposed to reflect the dynamic amount of
additional IEs appended to this frame, before the body -- such as what
is copied in mwifiex_tdls_append_rates_ie() (or
ieee80211_tdls_add_ies() if we're talking about a mac80211 driver?). The
field itself doesn't actually matter, because it isn't used in the
driver AFAICT.

Brian