[PATCH v3 0/2] wifi: brcm80211: replace deprecated strncpy

[PATCH v3 0/2] wifi: brcm80211: replace deprecated strncpy

[Justin Stitt]

Hi,

This series used to be just one patch in [v2] but I've split it into two
separate patches.

The motivation behind this series is that strncpy() is deprecated for
use on NUL-terminated destination strings [1] and as such we should
prefer more robust and less ambiguous string interfaces.

In cases where we expect the destination buffer to be NUL-terminated
let's opt for strscpy() as this guarantees NUL-termination. Other cases
are just simple byte copies with pre-determined bounds; for these let's
use plain-ol' memcpy().

Each change is detailed in its accompanying patch message.

Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Changes in v3:
- split up into two separate patches (thanks Franky)
- use better subject line (thanks Franky + Kalle)
- Link to v2: https://lore.kernel.org/r/20231016-strncpy-drivers-net-wireless-broadcom-brcm80211-brcmfmac-cfg80211-c-v2-1-6c7567e1d3b8@google.com

Changes in v2:
- add other strncpy replacements
- Link to v1: https://lore.kernel.org/r/20231016-strncpy-drivers-net-wireless-broadcom-brcm80211-brcmfmac-cfg80211-c-v1-1-4234807ca07e@google.com

---
Justin Stitt (2):
      wifi: brcm80211: replace deprecated strncpy with strscpy
      wifi: brcmsmac: replace deprecated strncpy with memcpy

 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 +-
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c      | 2 +-
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c  | 6 +++---
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c      | 3 +--
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c     | 4 ++--
 5 files changed, 8 insertions(+), 9 deletions(-)
---
base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2
change-id: 20231016-strncpy-drivers-net-wireless-broadcom-brcm80211-brcmfmac-cfg80211-c-a20108421685

Best regards,
--
Justin Stitt <justinstitt@google.com>

[PATCH v3 1/2] wifi: brcm80211: replace deprecated strncpy with strscpy

[Justin Stitt]

Let's move away from using strncpy and instead favor a less ambiguous
and more robust interface.

For ifp->ndev->name, we expect ifp->ndev->name to be NUL-terminated based
on its use in format strings within core.c:
67 |       char *brcmf_ifname(struct brcmf_if *ifp)
68 |       {
69 |            if (!ifp)
70 |                    return "<if_null>";
71 |
72 |            if (ifp->ndev)
73 |                    return ifp->ndev->name;
74 |
75 |            return "<if_none>";
76 |       }
...
288 |       static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
289 |                                              struct net_device *ndev) {
...
330 |       brcmf_dbg(INFO, "%s: insufficient headroom (%d)\n",
331 |                 brcmf_ifname(ifp), head_delta);
...
336 |       bphy_err(drvr, "%s: failed to expand headroom\n",
337 |                brcmf_ifname(ifp));

For di->name, we expect di->name to be NUL-terminated based on its usage
with format strings:
|       brcms_dbg_dma(di->core,
|                     "%s: DMA64 tx doesn't have AE set\n",
|                     di->name);

Looking at its allocation we can see that it is already zero-allocated
which means NUL-padding is not required:
|       di = kzalloc(sizeof(struct dma_info), GFP_ATOMIC);

For wlc->modulecb[i].name, we expect each name in wlc->modulecb to be
NUL-terminated based on their usage with strcmp():
|       if (!strcmp(wlc->modulecb[i].name, name) &&

NUL-padding is not required as wlc is zero-allocated in:
brcms_c_attach_malloc() ->
|       wlc = kzalloc(sizeof(struct brcms_c_info), GFP_ATOMIC);

For all these cases, a suitable replacement is `strscpy` due to the fact
that it guarantees NUL-termination on the destination buffer without
unnecessarily NUL-padding.

Signed-off-by: Justin Stitt <justinstitt@google.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 +-
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c      | 2 +-
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c      | 3 +--
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c     | 4 ++--
 4 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 2a90bb24ba77..7daa418df877 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -866,7 +866,7 @@ struct wireless_dev *brcmf_apsta_add_vif(struct wiphy *wiphy, const char *name,
 		goto fail;
 	}
 
-	strncpy(ifp->ndev->name, name, sizeof(ifp->ndev->name) - 1);
+	strscpy(ifp->ndev->name, name, sizeof(ifp->ndev->name));
 	err = brcmf_net_attach(ifp, true);
 	if (err) {
 		bphy_err(drvr, "Registering netdevice failed\n");
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
index d4492d02e4ea..6e0c90f4718b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
@@ -2334,7 +2334,7 @@ struct wireless_dev *brcmf_p2p_add_vif(struct wiphy *wiphy, const char *name,
 		goto fail;
 	}
 
-	strncpy(ifp->ndev->name, name, sizeof(ifp->ndev->name) - 1);
+	strscpy(ifp->ndev->name, name, sizeof(ifp->ndev->name));
 	ifp->ndev->name_assign_type = name_assign_type;
 	err = brcmf_net_attach(ifp, true);
 	if (err) {
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c
index b7df576bb84d..3d5c1ef8f7f2 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c
@@ -584,8 +584,7 @@ struct dma_pub *dma_attach(char *name, struct brcms_c_info *wlc,
 		      rxextheadroom, nrxpost, rxoffset, txregbase, rxregbase);
 
 	/* make a private copy of our callers name */
-	strncpy(di->name, name, MAXNAMEL);
-	di->name[MAXNAMEL - 1] = '\0';
+	strscpy(di->name, name, sizeof(di->name));
 
 	di->dmadev = core->dma_dev;
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c
index b3663c5ef382..34460b5815d0 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c
@@ -5551,8 +5551,8 @@ int brcms_c_module_register(struct brcms_pub *pub,
 	/* find an empty entry and just add, no duplication check! */
 	for (i = 0; i < BRCMS_MAXMODULES; i++) {
 		if (wlc->modulecb[i].name[0] == '\0') {
-			strncpy(wlc->modulecb[i].name, name,
-				sizeof(wlc->modulecb[i].name) - 1);
+			strscpy(wlc->modulecb[i].name, name,
+				sizeof(wlc->modulecb[i].name));
 			wlc->modulecb[i].hdl = hdl;
 			wlc->modulecb[i].down_fn = d_fn;
 			return 0;

-- 
2.42.0.655.g421f12c284-goog

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

[Justin Stitt]

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);
 
 	/*
 	 * 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);
 

-- 
2.42.0.655.g421f12c284-goog

Re: [PATCH v3 0/2] wifi: brcm80211: replace deprecated strncpy

[Franky Lin]

[-- Attachment #1: Type: text/plain, Size: 2420 bytes --]

On Tue, Oct 17, 2023 at 1:11 PM 'Justin Stitt' via
BRCM80211-DEV-LIST,PDL <brcm80211-dev-list.pdl@broadcom.com> wrote:
>
> Hi,
>
> This series used to be just one patch in [v2] but I've split it into two
> separate patches.
>
> The motivation behind this series is that strncpy() is deprecated for
> use on NUL-terminated destination strings [1] and as such we should
> prefer more robust and less ambiguous string interfaces.
>
> In cases where we expect the destination buffer to be NUL-terminated
> let's opt for strscpy() as this guarantees NUL-termination. Other cases
> are just simple byte copies with pre-determined bounds; for these let's
> use plain-ol' memcpy().
>
> Each change is detailed in its accompanying patch message.
>
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
> Link: https://github.com/KSPP/linux/issues/90
> Signed-off-by: Justin Stitt <justinstitt@google.com>

Reviewed-by: Franky Lin <franky.lin@broadcom.com>

> ---
> Changes in v3:
> - split up into two separate patches (thanks Franky)
> - use better subject line (thanks Franky + Kalle)
> - Link to v2: https://lore.kernel.org/r/20231016-strncpy-drivers-net-wireless-broadcom-brcm80211-brcmfmac-cfg80211-c-v2-1-6c7567e1d3b8@google.com
>
> Changes in v2:
> - add other strncpy replacements
> - Link to v1: https://lore.kernel.org/r/20231016-strncpy-drivers-net-wireless-broadcom-brcm80211-brcmfmac-cfg80211-c-v1-1-4234807ca07e@google.com
>
> ---
> Justin Stitt (2):
>       wifi: brcm80211: replace deprecated strncpy with strscpy
>       wifi: brcmsmac: replace deprecated strncpy with memcpy
>
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 +-
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c      | 2 +-
>  drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c  | 6 +++---
>  drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c      | 3 +--
>  drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c     | 4 ++--
>  5 files changed, 8 insertions(+), 9 deletions(-)
> ---
> base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2
> change-id: 20231016-strncpy-drivers-net-wireless-broadcom-brcm80211-brcmfmac-cfg80211-c-a20108421685
>
> Best regards,
> --
> Justin Stitt <justinstitt@google.com>
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4203 bytes --]

Re: [PATCH v3 0/2] wifi: brcm80211: replace deprecated strncpy

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 2301 bytes --]

On 10/17/2023 11:27 PM, Franky Lin wrote:
> On Tue, Oct 17, 2023 at 1:11 PM 'Justin Stitt' via
> BRCM80211-DEV-LIST,PDL <brcm80211-dev-list.pdl@broadcom.com> wrote:
>>
>> Hi,
>>
>> This series used to be just one patch in [v2] but I've split it into two
>> separate patches.
>>
>> The motivation behind this series is that strncpy() is deprecated for
>> use on NUL-terminated destination strings [1] and as such we should
>> prefer more robust and less ambiguous string interfaces.
>>
>> In cases where we expect the destination buffer to be NUL-terminated
>> let's opt for strscpy() as this guarantees NUL-termination. Other cases
>> are just simple byte copies with pre-determined bounds; for these let's
>> use plain-ol' memcpy().
>>
>> Each change is detailed in its accompanying patch message.
>>
>> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
>> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
>> Link: https://github.com/KSPP/linux/issues/90
>> Signed-off-by: Justin Stitt <justinstitt@google.com>
> 
> Reviewed-by: Franky Lin <franky.lin@broadcom.com>

Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>

>> ---
>> Changes in v3:
>> - split up into two separate patches (thanks Franky)
>> - use better subject line (thanks Franky + Kalle)
>> - Link to v2: https://lore.kernel.org/r/20231016-strncpy-drivers-net-wireless-broadcom-brcm80211-brcmfmac-cfg80211-c-v2-1-6c7567e1d3b8@google.com
>>
>> Changes in v2:
>> - add other strncpy replacements
>> - Link to v1: https://lore.kernel.org/r/20231016-strncpy-drivers-net-wireless-broadcom-brcm80211-brcmfmac-cfg80211-c-v1-1-4234807ca07e@google.com
>>
>> ---
>> Justin Stitt (2):
>>        wifi: brcm80211: replace deprecated strncpy with strscpy
>>        wifi: brcmsmac: replace deprecated strncpy with memcpy
>>
>>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 +-
>>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c      | 2 +-
>>   drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c  | 6 +++---
>>   drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c      | 3 +--
>>   drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c     | 4 ++--
>>   5 files changed, 8 insertions(+), 9 deletions(-)
>> ---

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

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

[Kees Cook]

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

-- 
Kees Cook

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

[Justin Stitt]

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

Re: [PATCH v3 1/2] wifi: brcm80211: replace deprecated strncpy with strscpy

[Kees Cook]

On Tue, Oct 17, 2023 at 08:11:28PM +0000, Justin Stitt wrote:
> Let's move away from using strncpy and instead favor a less ambiguous
> and more robust interface.
> 
> For ifp->ndev->name, we expect ifp->ndev->name to be NUL-terminated based
> on its use in format strings within core.c:
> 67 |       char *brcmf_ifname(struct brcmf_if *ifp)
> 68 |       {
> 69 |            if (!ifp)
> 70 |                    return "<if_null>";
> 71 |
> 72 |            if (ifp->ndev)
> 73 |                    return ifp->ndev->name;
> 74 |
> 75 |            return "<if_none>";
> 76 |       }
> ...
> 288 |       static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
> 289 |                                              struct net_device *ndev) {
> ...
> 330 |       brcmf_dbg(INFO, "%s: insufficient headroom (%d)\n",
> 331 |                 brcmf_ifname(ifp), head_delta);
> ...
> 336 |       bphy_err(drvr, "%s: failed to expand headroom\n",
> 337 |                brcmf_ifname(ifp));
> 
> For di->name, we expect di->name to be NUL-terminated based on its usage
> with format strings:
> |       brcms_dbg_dma(di->core,
> |                     "%s: DMA64 tx doesn't have AE set\n",
> |                     di->name);
> 
> Looking at its allocation we can see that it is already zero-allocated
> which means NUL-padding is not required:
> |       di = kzalloc(sizeof(struct dma_info), GFP_ATOMIC);
> 
> For wlc->modulecb[i].name, we expect each name in wlc->modulecb to be
> NUL-terminated based on their usage with strcmp():
> |       if (!strcmp(wlc->modulecb[i].name, name) &&
> 
> NUL-padding is not required as wlc is zero-allocated in:
> brcms_c_attach_malloc() ->
> |       wlc = kzalloc(sizeof(struct brcms_c_info), GFP_ATOMIC);
> 
> For all these cases, a suitable replacement is `strscpy` due to the fact
> that it guarantees NUL-termination on the destination buffer without
> unnecessarily NUL-padding.
> 
> Signed-off-by: Justin Stitt <justinstitt@google.com>

Good; this looks like standard direct replacements.

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

-- 
Kees Cook

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

[Kees Cook]

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>

-- 
Kees Cook

Re: [PATCH v3 1/2] wifi: brcm80211: replace deprecated strncpy with strscpy

[Kalle Valo]

Justin Stitt <justinstitt@google.com> wrote:

> Let's move away from using strncpy and instead favor a less ambiguous
> and more robust interface.
> 
> For ifp->ndev->name, we expect ifp->ndev->name to be NUL-terminated based
> on its use in format strings within core.c:
> 67 |       char *brcmf_ifname(struct brcmf_if *ifp)
> 68 |       {
> 69 |            if (!ifp)
> 70 |                    return "<if_null>";
> 71 |
> 72 |            if (ifp->ndev)
> 73 |                    return ifp->ndev->name;
> 74 |
> 75 |            return "<if_none>";
> 76 |       }
> ...
> 288 |       static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
> 289 |                                              struct net_device *ndev) {
> ...
> 330 |       brcmf_dbg(INFO, "%s: insufficient headroom (%d)\n",
> 331 |                 brcmf_ifname(ifp), head_delta);
> ...
> 336 |       bphy_err(drvr, "%s: failed to expand headroom\n",
> 337 |                brcmf_ifname(ifp));
> 
> For di->name, we expect di->name to be NUL-terminated based on its usage
> with format strings:
> |       brcms_dbg_dma(di->core,
> |                     "%s: DMA64 tx doesn't have AE set\n",
> |                     di->name);
> 
> Looking at its allocation we can see that it is already zero-allocated
> which means NUL-padding is not required:
> |       di = kzalloc(sizeof(struct dma_info), GFP_ATOMIC);
> 
> For wlc->modulecb[i].name, we expect each name in wlc->modulecb to be
> NUL-terminated based on their usage with strcmp():
> |       if (!strcmp(wlc->modulecb[i].name, name) &&
> 
> NUL-padding is not required as wlc is zero-allocated in:
> brcms_c_attach_malloc() ->
> |       wlc = kzalloc(sizeof(struct brcms_c_info), GFP_ATOMIC);
> 
> For all these cases, a suitable replacement is `strscpy` due to the fact
> that it guarantees NUL-termination on the destination buffer without
> unnecessarily NUL-padding.
> 
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>

2 patches applied to wireless-next.git, thanks.

9d0d0a207040 wifi: brcm80211: replace deprecated strncpy with strscpy
a614f9579705 wifi: brcmsmac: replace deprecated strncpy with memcpy

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20231017-strncpy-drivers-net-wireless-broadcom-brcm80211-brcmfmac-cfg80211-c-v3-1-af780d74ae38@google.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches