mwifiex: replace deprecated strcpy() with strscpy()

[PATCH] mwifiex: replace deprecated strcpy() with strscpy()

Posted by Miguel García 3 months ago

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
strings copied here are bounded literals or validated inputs, so no
return-value handling is required.

Signed-off-by: Miguel García <miguelgarciaroman8@gmail.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.
  */
 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)
 
 	switch (card->dev->device) {
 	case PCIE_DEVICE_ID_MARVELL_88W8766P:
-		strcpy(adapter->fw_name, PCIE8766_DEFAULT_FW_NAME);
+		strscpy(adapter->fw_name,
+			PCIE8766_DEFAULT_FW_NAME,
+			sizeof(adapter->fw_name));
 		break;
+
 	case PCIE_DEVICE_ID_MARVELL_88W8897:
 		mwifiex_write_reg(adapter, 0x0c58, 0x80c00000);
 		mwifiex_read_reg(adapter, 0x0c58, &revision_id);
 		revision_id &= 0xff00;
+
 		switch (revision_id) {
 		case PCIE8897_A0:
-			strcpy(adapter->fw_name, PCIE8897_A0_FW_NAME);
+			strscpy(adapter->fw_name,
+				PCIE8897_A0_FW_NAME,
+				sizeof(adapter->fw_name));
 			break;
 		case PCIE8897_B0:
-			strcpy(adapter->fw_name, PCIE8897_B0_FW_NAME);
+			strscpy(adapter->fw_name,
+				PCIE8897_B0_FW_NAME,
+				sizeof(adapter->fw_name));
 			break;
 		default:
-			strcpy(adapter->fw_name, PCIE8897_DEFAULT_FW_NAME);
-
+			strscpy(adapter->fw_name,
+				PCIE8897_DEFAULT_FW_NAME,
+				sizeof(adapter->fw_name));
 			break;
 		}
 		break;
+
 	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;
+
 		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

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

Posted by Brian Norris 3 months ago

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@gmail.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
>

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

Posted by Johannes Berg 3 months ago

Please use the correct "wifi: " prefix for all your patches.

And I'm not sure how you arrived at the CC list, but it seems excessive,
for a trivial cleanup in particular.

johannes

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

Posted by jeff.chen_1@nxp.com 3 months ago

On Sat, Jul 05, 2025 at 03:36:00 PM +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
> strings copied here are bounded literals or validated inputs, so no
> return-value handling is required.
> 
> Signed-off-by: Miguel García <miguelgarciaroman8@gmail.com>

Reviewed-by: Jeff Chen <jeff.chen_1@nxp.con>