| Message ID | 20211103171055.16911-2-verdre@v0yd.nl (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | mwifiex: Add quirk to disable deep sleep with certain hardware revision | expand |
On Wed, Nov 03, 2021 at 06:10:54PM +0100, Jonas Dreßler wrote: > Since the version string we get from the firmware is always 128 > characters long, use a define for this size instead of having the number > 128 copied all over the place. > > Signed-off-by: Jonas Dreßler <verdre@v0yd.nl> Thanks for this patch. For the series: Reviewed-by: Brian Norris <briannorris@chromium.org> > --- > drivers/net/wireless/marvell/mwifiex/fw.h | 4 +++- > drivers/net/wireless/marvell/mwifiex/main.h | 2 +- > drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 5 +++-- > 3 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h > index 2ff23ab259ab..63c25c69ed2b 100644 > --- a/drivers/net/wireless/marvell/mwifiex/fw.h > +++ b/drivers/net/wireless/marvell/mwifiex/fw.h > @@ -2071,9 +2071,11 @@ struct mwifiex_ie_types_robust_coex { > __le32 mode; > } __packed; > > +#define MWIFIEX_VERSION_STR_LENGTH 128 > + > struct host_cmd_ds_version_ext { > u8 version_str_sel; > - char version_str[128]; > + char version_str[MWIFIEX_VERSION_STR_LENGTH]; > } __packed; > > struct host_cmd_ds_mgmt_frame_reg { > diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h > index 90012cbcfd15..65609ea2327e 100644 > --- a/drivers/net/wireless/marvell/mwifiex/main.h > +++ b/drivers/net/wireless/marvell/mwifiex/main.h > @@ -646,7 +646,7 @@ struct mwifiex_private { > struct wireless_dev wdev; > struct mwifiex_chan_freq_power cfp; > u32 versionstrsel; > - char version_str[128]; > + char version_str[MWIFIEX_VERSION_STR_LENGTH]; > #ifdef CONFIG_DEBUG_FS > struct dentry *dfs_dev_dir; > #endif > diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c > index 6b5d35d9e69f..20b69a37f9e1 100644 > --- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c > +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c > @@ -711,8 +711,9 @@ static int mwifiex_ret_ver_ext(struct mwifiex_private *priv, > if (version_ext) { > version_ext->version_str_sel = ver_ext->version_str_sel; > memcpy(version_ext->version_str, ver_ext->version_str, > - sizeof(char) * 128); > - memcpy(priv->version_str, ver_ext->version_str, 128); > + MWIFIEX_VERSION_STR_LENGTH); > + memcpy(priv->version_str, ver_ext->version_str, > + MWIFIEX_VERSION_STR_LENGTH); Not related to your patch, but this highlights that nobody is ensuring this string is 0-terminated, and various other places (notably, *not* your patch 2!) assume that it is. We should either fix those to use an snprintf()/length-restricted variant, or else just force: priv->version_str[MWIFIEX_VERSION_STR_LENGTH - 1] = '\0'; here. But that's a separate issue/patch. I can cook one on top of your series when it gets merged if you don't want to. Brian > } > return 0; > } > -- > 2.33.1 >
On 11/3/21 18:28, Brian Norris wrote: > On Wed, Nov 03, 2021 at 06:10:54PM +0100, Jonas Dreßler wrote: >> Since the version string we get from the firmware is always 128 >> characters long, use a define for this size instead of having the number >> 128 copied all over the place. >> >> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl> > > Thanks for this patch. For the series: > > Reviewed-by: Brian Norris <briannorris@chromium.org> > >> --- >> drivers/net/wireless/marvell/mwifiex/fw.h | 4 +++- >> drivers/net/wireless/marvell/mwifiex/main.h | 2 +- >> drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 5 +++-- >> 3 files changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h >> index 2ff23ab259ab..63c25c69ed2b 100644 >> --- a/drivers/net/wireless/marvell/mwifiex/fw.h >> +++ b/drivers/net/wireless/marvell/mwifiex/fw.h >> @@ -2071,9 +2071,11 @@ struct mwifiex_ie_types_robust_coex { >> __le32 mode; >> } __packed; >> >> +#define MWIFIEX_VERSION_STR_LENGTH 128 >> + >> struct host_cmd_ds_version_ext { >> u8 version_str_sel; >> - char version_str[128]; >> + char version_str[MWIFIEX_VERSION_STR_LENGTH]; >> } __packed; >> >> struct host_cmd_ds_mgmt_frame_reg { >> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h >> index 90012cbcfd15..65609ea2327e 100644 >> --- a/drivers/net/wireless/marvell/mwifiex/main.h >> +++ b/drivers/net/wireless/marvell/mwifiex/main.h >> @@ -646,7 +646,7 @@ struct mwifiex_private { >> struct wireless_dev wdev; >> struct mwifiex_chan_freq_power cfp; >> u32 versionstrsel; >> - char version_str[128]; >> + char version_str[MWIFIEX_VERSION_STR_LENGTH]; >> #ifdef CONFIG_DEBUG_FS >> struct dentry *dfs_dev_dir; >> #endif >> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c >> index 6b5d35d9e69f..20b69a37f9e1 100644 >> --- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c >> +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c >> @@ -711,8 +711,9 @@ static int mwifiex_ret_ver_ext(struct mwifiex_private *priv, >> if (version_ext) { >> version_ext->version_str_sel = ver_ext->version_str_sel; >> memcpy(version_ext->version_str, ver_ext->version_str, >> - sizeof(char) * 128); >> - memcpy(priv->version_str, ver_ext->version_str, 128); >> + MWIFIEX_VERSION_STR_LENGTH); >> + memcpy(priv->version_str, ver_ext->version_str, >> + MWIFIEX_VERSION_STR_LENGTH); > > Not related to your patch, but this highlights that nobody is ensuring > this string is 0-terminated, and various other places (notably, *not* > your patch 2!) assume that it is. > > We should either fix those to use an snprintf()/length-restricted > variant, or else just force: > > priv->version_str[MWIFIEX_VERSION_STR_LENGTH - 1] = '\0'; > > here. Indeed, right now we just trust the firmware to make sure that's the case. Let me add another patch appending the '\0' to priv->version_str... > > But that's a separate issue/patch. I can cook one on top of your series > when it gets merged if you don't want to. > > Brian > >> } >> return 0; >> } >> -- >> 2.33.1 >>
diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h index 2ff23ab259ab..63c25c69ed2b 100644 --- a/drivers/net/wireless/marvell/mwifiex/fw.h +++ b/drivers/net/wireless/marvell/mwifiex/fw.h @@ -2071,9 +2071,11 @@ struct mwifiex_ie_types_robust_coex { __le32 mode; } __packed; +#define MWIFIEX_VERSION_STR_LENGTH 128 + struct host_cmd_ds_version_ext { u8 version_str_sel; - char version_str[128]; + char version_str[MWIFIEX_VERSION_STR_LENGTH]; } __packed; struct host_cmd_ds_mgmt_frame_reg { diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h index 90012cbcfd15..65609ea2327e 100644 --- a/drivers/net/wireless/marvell/mwifiex/main.h +++ b/drivers/net/wireless/marvell/mwifiex/main.h @@ -646,7 +646,7 @@ struct mwifiex_private { struct wireless_dev wdev; struct mwifiex_chan_freq_power cfp; u32 versionstrsel; - char version_str[128]; + char version_str[MWIFIEX_VERSION_STR_LENGTH]; #ifdef CONFIG_DEBUG_FS struct dentry *dfs_dev_dir; #endif diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c index 6b5d35d9e69f..20b69a37f9e1 100644 --- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c @@ -711,8 +711,9 @@ static int mwifiex_ret_ver_ext(struct mwifiex_private *priv, if (version_ext) { version_ext->version_str_sel = ver_ext->version_str_sel; memcpy(version_ext->version_str, ver_ext->version_str, - sizeof(char) * 128); - memcpy(priv->version_str, ver_ext->version_str, 128); + MWIFIEX_VERSION_STR_LENGTH); + memcpy(priv->version_str, ver_ext->version_str, + MWIFIEX_VERSION_STR_LENGTH); } return 0; }
Since the version string we get from the firmware is always 128 characters long, use a define for this size instead of having the number 128 copied all over the place. Signed-off-by: Jonas Dreßler <verdre@v0yd.nl> --- drivers/net/wireless/marvell/mwifiex/fw.h | 4 +++- drivers/net/wireless/marvell/mwifiex/main.h | 2 +- drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 5 +++-- 3 files changed, 7 insertions(+), 4 deletions(-)