| Message ID | 20230620100803.519926-1-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [1/2,v2] wifi: mwifiex: avoid strlcpy() and use strscpy() where appropriate | expand |
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 >
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
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
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;
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(-)