linux-kernel - Re: [PATCH] mwifiex: replace deprecated strcpy() with strscpy()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aG1lcWYjk9GARp1P@google.com>
Date: Tue, 8 Jul 2025 11:37:37 -0700
From: Brian Norris <briannorris@...omium.org>
To: Miguel García <miguelgarciaroman8@...il.com>
Cc: linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
	francesco@...cini.it, thomas.weissschuh@...utronix.de,
	tglx@...utronix.de, johannes.berg@...el.com, mingo@...nel.org,
	christophe.jaillet@...adoo.fr, skhan@...uxfoundation.org
Subject: Re: [PATCH] mwifiex: replace deprecated strcpy() with strscpy()

Hi Miguel,

On Sat, Jul 05, 2025 at 03:36:00PM +0200, Miguel García wrote:
> strcpy() is deprecated for NUL-terminated strings because it may overflow
> the destination buffer and does not guarantee termination.  strscpy()
> avoids these issues.
> 
> adapter->fw_name is a fixed-size char array (64 bytes).  All source

It's actually 32 bytes. Not sure where 64 came from.

> strings copied here are bounded literals or validated inputs, so no
> return-value handling is required.
> 
> Signed-off-by: Miguel García <miguelgarciaroman8@...il.com>
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 40 ++++++++++++++-------
>  1 file changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index a760de191fce..2aad9ab210e0 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -3098,9 +3098,8 @@ static int mwifiex_pcie_request_irq(struct mwifiex_adapter *adapter)
>  }
>  
>  /*
> - * This function gets the firmware name for downloading by revision id
> - *
> - * Read revision id register to get revision id
> + * Get firmware name for download by revision id
> + * Uses strscpy() to ensure NUL-termination and avoid overflow.

The original comments are strange here (as are many of the comments in
this driver), so you probably have a good idea to tweak them. But IMO,
their main problem is that they repeat themselves, and don't really add
much value over simply having well-named functions. And particularly, we
don't need to write out full sentences to describe every step that we
do.

So, please drop the "Use strscpy() [...]" sentence. It doesn't need to
be here. If it's not obvious what str*() APIs are doing, then we have
bigger problems.

This seems fine:

/*
 * Get firmware name for download by revision ID
 */

>   */
>  static void mwifiex_pcie_get_fw_name(struct mwifiex_adapter *adapter)
>  {
> @@ -3110,39 +3109,56 @@ static void mwifiex_pcie_get_fw_name(struct mwifiex_adapter *adapter)
...
>  	case PCIE_DEVICE_ID_MARVELL_88W8997:
>  		mwifiex_read_reg(adapter, 0x8, &revision_id);
>  		mwifiex_read_reg(adapter, 0x0cd0, &version);
>  		mwifiex_read_reg(adapter, 0x0cd4, &magic);
> +
>  		revision_id &= 0xff;
> -		version &= 0x7;
> -		magic &= 0xff;
> +		version     &= 0x7;
> +		magic       &= 0xff;

Don't make arbitrary whitespace changes. The whitespace was fine as-is.

Thanks,
Brian

> +
>  		if (revision_id == PCIE8997_A1 &&
>  		    magic == CHIP_MAGIC_VALUE &&
>  		    version == CHIP_VER_PCIEUART)
> -			strcpy(adapter->fw_name, PCIEUART8997_FW_NAME_V4);
> +			strscpy(adapter->fw_name,
> +				PCIEUART8997_FW_NAME_V4,
> +				sizeof(adapter->fw_name));
>  		else
> -			strcpy(adapter->fw_name, PCIEUSB8997_FW_NAME_V4);
> +			strscpy(adapter->fw_name,
> +				PCIEUSB8997_FW_NAME_V4,
> +				sizeof(adapter->fw_name));
>  		break;
> +
>  	default:
>  		break;
>  	}
> -- 
> 2.34.1
>