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@xxxxxxxxx>
> ---
> 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
>
