diff mbox series

[v3,2/2] wifi: brcmsmac: replace deprecated strncpy with memcpy

Message ID 20231017-strncpy-drivers-net-wireless-broadcom-brcm80211-brcmfmac-cfg80211-c-v3-2-af780d74ae38@google.com (mailing list archive)
State Accepted
Commit a614f95797055dd96802202af542b04fead7274f
Delegated to: Kalle Valo
Headers show
Series wifi: brcm80211: replace deprecated strncpy | expand

Commit Message

Justin Stitt Oct. 17, 2023, 8:11 p.m. UTC
Let's move away from using strncpy and instead use the more obvious
interface for this context.

For wlc->pub->srom_ccode, we're just copying two bytes from ccode into
wlc->pub->srom_ccode with no expectation that srom_ccode be
NUL-terminated:
wlc->pub->srom_ccode is only used in regulatory_hint():
1193 |       if (wl->pub->srom_ccode[0] &&
1194 |           regulatory_hint(wl->wiphy, wl->pub->srom_ccode))
1195 |               wiphy_err(wl->wiphy, "%s: regulatory hint failed\n", __func__);

We can see that only index 0 and index 1 are accessed.
3307 |       int regulatory_hint(struct wiphy *wiphy, const char *alpha2)
3308 |       {
...  |          ...
3322 |          request->alpha2[0] = alpha2[0];
3323 |          request->alpha2[1] = alpha2[1];
...  |          ...
3332 |       }

Since this is just a simple byte copy with correct lengths, let's use
memcpy(). There should be no functional change.

In a similar boat, both wlc->country_default and
wlc->autocountry_default are just simple byte copies so let's use
memcpy. However, FWICT they aren't used anywhere. (they should be
used or removed -- not in scope of my patch, though).

Signed-off-by: Justin Stitt <justinstitt@google.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Kees Cook Oct. 19, 2023, 12:03 a.m. UTC | #1
On Tue, Oct 17, 2023 at 08:11:29PM +0000, Justin Stitt wrote:
> Let's move away from using strncpy and instead use the more obvious
> interface for this context.
> 
> For wlc->pub->srom_ccode, we're just copying two bytes from ccode into
> wlc->pub->srom_ccode with no expectation that srom_ccode be
> NUL-terminated:
> wlc->pub->srom_ccode is only used in regulatory_hint():
> 1193 |       if (wl->pub->srom_ccode[0] &&
> 1194 |           regulatory_hint(wl->wiphy, wl->pub->srom_ccode))
> 1195 |               wiphy_err(wl->wiphy, "%s: regulatory hint failed\n", __func__);
> 
> We can see that only index 0 and index 1 are accessed.
> 3307 |       int regulatory_hint(struct wiphy *wiphy, const char *alpha2)
> 3308 |       {
> ...  |          ...
> 3322 |          request->alpha2[0] = alpha2[0];
> 3323 |          request->alpha2[1] = alpha2[1];
> ...  |          ...
> 3332 |       }
> 
> Since this is just a simple byte copy with correct lengths, let's use
> memcpy(). There should be no functional change.
> 
> In a similar boat, both wlc->country_default and
> wlc->autocountry_default are just simple byte copies so let's use
> memcpy. However, FWICT they aren't used anywhere. (they should be
> used or removed -- not in scope of my patch, though).
> 
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c
> index 5a6d9c86552a..f6962e558d7c 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c
> @@ -341,7 +341,7 @@ struct brcms_cm_info *brcms_c_channel_mgr_attach(struct brcms_c_info *wlc)
>  	/* store the country code for passing up as a regulatory hint */
>  	wlc_cm->world_regd = brcms_world_regd(ccode, ccode_len);
>  	if (brcms_c_country_valid(ccode))
> -		strncpy(wlc->pub->srom_ccode, ccode, ccode_len);
> +		memcpy(wlc->pub->srom_ccode, ccode, ccode_len);

        const char *ccode = sprom->alpha2;
        int ccode_len = sizeof(sprom->alpha2);

struct ssb_sprom {
	...
        char alpha2[2];         /* Country Code as two chars like EU or US */

This should be marked __nonstring, IMO.

struct brcms_pub {
	...
        char srom_ccode[BRCM_CNTRY_BUF_SZ];     /* Country Code in SROM */

#define BRCM_CNTRY_BUF_SZ        4       /* Country string is 3 bytes + NUL */

This, however, is shown as explicitly %NUL terminated.

The old strncpy wasn't %NUL terminating wlc->pub->srom_ccode, though, so
the memcpy is the same result, but is that actually _correct_ here?

>  
>  	/*
>  	 * If no custom world domain is found in the SROM, use the
> @@ -354,10 +354,10 @@ struct brcms_cm_info *brcms_c_channel_mgr_attach(struct brcms_c_info *wlc)
>  	}
>  
>  	/* save default country for exiting 11d regulatory mode */
> -	strncpy(wlc->country_default, ccode, ccode_len);
> +	memcpy(wlc->country_default, ccode, ccode_len);
>  
>  	/* initialize autocountry_default to driver default */
> -	strncpy(wlc->autocountry_default, ccode, ccode_len);
> +	memcpy(wlc->autocountry_default, ccode, ccode_len);

struct brcms_c_info {
	...
        char country_default[BRCM_CNTRY_BUF_SZ];
        char autocountry_default[BRCM_CNTRY_BUF_SZ];

These are similar...

So, this change results in the same behavior, but is it right?

-Kees
Justin Stitt Oct. 19, 2023, 5:37 p.m. UTC | #2
On Wed, Oct 18, 2023 at 5:03 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Oct 17, 2023 at 08:11:29PM +0000, Justin Stitt wrote:
> > Let's move away from using strncpy and instead use the more obvious
> > interface for this context.
> >
> > For wlc->pub->srom_ccode, we're just copying two bytes from ccode into
> > wlc->pub->srom_ccode with no expectation that srom_ccode be
> > NUL-terminated:
> > wlc->pub->srom_ccode is only used in regulatory_hint():
> > 1193 |       if (wl->pub->srom_ccode[0] &&
> > 1194 |           regulatory_hint(wl->wiphy, wl->pub->srom_ccode))
> > 1195 |               wiphy_err(wl->wiphy, "%s: regulatory hint failed\n", __func__);
> >
> > We can see that only index 0 and index 1 are accessed.
> > 3307 |       int regulatory_hint(struct wiphy *wiphy, const char *alpha2)
> > 3308 |       {
> > ...  |          ...
> > 3322 |          request->alpha2[0] = alpha2[0];
> > 3323 |          request->alpha2[1] = alpha2[1];
> > ...  |          ...
> > 3332 |       }
> >
> > Since this is just a simple byte copy with correct lengths, let's use
> > memcpy(). There should be no functional change.
> >
> > In a similar boat, both wlc->country_default and
> > wlc->autocountry_default are just simple byte copies so let's use
> > memcpy. However, FWICT they aren't used anywhere. (they should be
> > used or removed -- not in scope of my patch, though).
> >
> > Signed-off-by: Justin Stitt <justinstitt@google.com>
> > ---
> >  drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c
> > index 5a6d9c86552a..f6962e558d7c 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c
> > @@ -341,7 +341,7 @@ struct brcms_cm_info *brcms_c_channel_mgr_attach(struct brcms_c_info *wlc)
> >       /* store the country code for passing up as a regulatory hint */
> >       wlc_cm->world_regd = brcms_world_regd(ccode, ccode_len);
> >       if (brcms_c_country_valid(ccode))
> > -             strncpy(wlc->pub->srom_ccode, ccode, ccode_len);
> > +             memcpy(wlc->pub->srom_ccode, ccode, ccode_len);
>
>         const char *ccode = sprom->alpha2;
>         int ccode_len = sizeof(sprom->alpha2);
>
> struct ssb_sprom {
>         ...
>         char alpha2[2];         /* Country Code as two chars like EU or US */
>
> This should be marked __nonstring, IMO.
>
> struct brcms_pub {
>         ...
>         char srom_ccode[BRCM_CNTRY_BUF_SZ];     /* Country Code in SROM */
>
> #define BRCM_CNTRY_BUF_SZ        4       /* Country string is 3 bytes + NUL */
>
> This, however, is shown as explicitly %NUL terminated.
>
> The old strncpy wasn't %NUL terminating wlc->pub->srom_ccode, though, so
> the memcpy is the same result, but is that actually _correct_ here?

Judging from the usage, we can see that only bytes at offset 0 and 1 are
used. I think the comment "/* Country string is 3 bytes + NUL */" might
be misleading or perhaps there are other uses that I can't find (which
require NUL-termination)?

>
> >
> >       /*
> >        * If no custom world domain is found in the SROM, use the
> > @@ -354,10 +354,10 @@ struct brcms_cm_info *brcms_c_channel_mgr_attach(struct brcms_c_info *wlc)
> >       }
> >
> >       /* save default country for exiting 11d regulatory mode */
> > -     strncpy(wlc->country_default, ccode, ccode_len);
> > +     memcpy(wlc->country_default, ccode, ccode_len);
> >
> >       /* initialize autocountry_default to driver default */
> > -     strncpy(wlc->autocountry_default, ccode, ccode_len);
> > +     memcpy(wlc->autocountry_default, ccode, ccode_len);
>
> struct brcms_c_info {
>         ...
>         char country_default[BRCM_CNTRY_BUF_SZ];
>         char autocountry_default[BRCM_CNTRY_BUF_SZ];
>
> These are similar...

I can't find any uses for these either.

>
> So, this change results in the same behavior, but is it right?
>
> -Kees
>
> --
> Kees Cook

Thanks
Justin
Kees Cook Oct. 26, 2023, 5:21 p.m. UTC | #3
On Wed, Oct 18, 2023 at 05:03:02PM -0700, Kees Cook wrote:
> On Tue, Oct 17, 2023 at 08:11:29PM +0000, Justin Stitt wrote:
> > Let's move away from using strncpy and instead use the more obvious
> > interface for this context.
> [...]
> So, this change results in the same behavior ...

I should have included my r-b tag:

Reviewed-by: Kees Cook <keescook@chromium.org>
diff mbox series

Patch

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c
index 5a6d9c86552a..f6962e558d7c 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c
@@ -341,7 +341,7 @@  struct brcms_cm_info *brcms_c_channel_mgr_attach(struct brcms_c_info *wlc)
 	/* store the country code for passing up as a regulatory hint */
 	wlc_cm->world_regd = brcms_world_regd(ccode, ccode_len);
 	if (brcms_c_country_valid(ccode))
-		strncpy(wlc->pub->srom_ccode, ccode, ccode_len);
+		memcpy(wlc->pub->srom_ccode, ccode, ccode_len);
 
 	/*
 	 * If no custom world domain is found in the SROM, use the
@@ -354,10 +354,10 @@  struct brcms_cm_info *brcms_c_channel_mgr_attach(struct brcms_c_info *wlc)
 	}
 
 	/* save default country for exiting 11d regulatory mode */
-	strncpy(wlc->country_default, ccode, ccode_len);
+	memcpy(wlc->country_default, ccode, ccode_len);
 
 	/* initialize autocountry_default to driver default */
-	strncpy(wlc->autocountry_default, ccode, ccode_len);
+	memcpy(wlc->autocountry_default, ccode, ccode_len);
 
 	brcms_c_set_country(wlc_cm, wlc_cm->world_regd);