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 <[email protected]>
---
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 <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]> 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 <[email protected]>
>
> Reviewed-by: Franky Lin <[email protected]>
Acked-by: Arend van Spriel <[email protected]>
>> ---
>> 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(-)
>> ---
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 <[email protected]>
Good; this looks like standard direct replacements.
Reviewed-by: Kees Cook <[email protected]>
--
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 <[email protected]>
--
Kees Cook