| Message ID | 20231016-strncpy-drivers-net-wireless-broadcom-brcm80211-brcmfmac-cfg80211-c-v2-1-6c7567e1d3b8@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] brcmfmac: replace deprecated strncpy | expand |
Hi Justin, On Mon, Oct 16, 2023 at 3:14 PM Justin Stitt <justinstitt@google.com> wrote: > > strncpy() is deprecated for use on NUL-terminated destination strings > [1] and as such we should prefer more robust and less ambiguous string > interfaces. > > This patch replaces multiple strncpy uses. For easier reading, I'll list > each destination buffer and mention whether it requires either > NUL-termination or NUL-padding. Kudos to the detailed analysis of each instance. One thing I can think of to make this better is to split it into smaller patches so if any regression is observed, only the specific code causing it needs to be reverted. Maybe instance 2, 3, 4 can be handled in one patch since they are touching the country code in one file. The other instances each can be an individual patch. The "brcmfmac" in the title is not accurate. The change touches both brcmfmac and brcmsmac. So "brcm80211" would be more appropriate. Thanks, - Franky > > 1) 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)); > > In this context, a suitable replacement is `strscpy` [2] due to the fact > that it guarantees NUL-termination on the destination buffer without > unnecessarily NUL-padding. > > 2) 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. > > 3) wlc->country_default, 4) wlc->autocountry_default > > FWICT, these two aren't used anywhere. At any rate, let's apply the same > reasoning as above and just use memcpy(). > > 5) 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); > > 6) 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); > > 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 > Cc: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt <justinstitt@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 > --- > Note: build-tested only. > > I've grouped these all into a single patch instead of a series as many > of the replacements are related to others and rely on context from one > another to justify changes. > --- > 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(-) > > 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/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); > > 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; > > --- > base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2 > change-id: 20231016-strncpy-drivers-net-wireless-broadcom-brcm80211-brcmfmac-cfg80211-c-a20108421685 > > Best regards, > -- > Justin Stitt <justinstitt@google.com> >
Franky Lin <franky.lin@broadcom.com> writes: > Hi Justin, > > On Mon, Oct 16, 2023 at 3:14 PM Justin Stitt <justinstitt@google.com> wrote: >> >> strncpy() is deprecated for use on NUL-terminated destination strings >> [1] and as such we should prefer more robust and less ambiguous string >> interfaces. >> >> This patch replaces multiple strncpy uses. For easier reading, I'll list >> each destination buffer and mention whether it requires either >> NUL-termination or NUL-padding. > > Kudos to the detailed analysis of each instance. One thing I can think > of to make this better is to split it into smaller patches so if any > regression is observed, only the specific code causing it needs to be > reverted. Maybe instance 2, 3, 4 can be handled in one patch since > they are touching the country code in one file. The other instances > each can be an individual patch. > > The "brcmfmac" in the title is not accurate. The change touches both > brcmfmac and brcmsmac. So "brcm80211" would be more appropriate. Please also add "wifi:" to the title, see the wiki link below for more info.
On Tue, Oct 17, 2023 at 9:49 AM Franky Lin <franky.lin@broadcom.com> wrote: > > Hi Justin, > > On Mon, Oct 16, 2023 at 3:14 PM Justin Stitt <justinstitt@google.com> wrote: > > > > strncpy() is deprecated for use on NUL-terminated destination strings > > [1] and as such we should prefer more robust and less ambiguous string > > interfaces. > > > > This patch replaces multiple strncpy uses. For easier reading, I'll list > > each destination buffer and mention whether it requires either > > NUL-termination or NUL-padding. > > Kudos to the detailed analysis of each instance. One thing I can think > of to make this better is to split it into smaller patches so if any > regression is observed, only the specific code causing it needs to be > reverted. Maybe instance 2, 3, 4 can be handled in one patch since > they are touching the country code in one file. The other instances > each can be an individual patch. > > The "brcmfmac" in the title is not accurate. The change touches both > brcmfmac and brcmsmac. So "brcm80211" would be more appropriate. Got it, will send new revision with a new subject + split up patches. > > Thanks, > - Franky > > > > > 1) 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)); > > > > In this context, a suitable replacement is `strscpy` [2] due to the fact > > that it guarantees NUL-termination on the destination buffer without > > unnecessarily NUL-padding. > > > > 2) 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. > > > > 3) wlc->country_default, 4) wlc->autocountry_default > > > > FWICT, these two aren't used anywhere. At any rate, let's apply the same > > reasoning as above and just use memcpy(). > > > > 5) 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); > > > > 6) 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); > > > > 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 > > Cc: linux-hardening@vger.kernel.org > > Signed-off-by: Justin Stitt <justinstitt@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 > > --- > > Note: build-tested only. > > > > I've grouped these all into a single patch instead of a series as many > > of the replacements are related to others and rely on context from one > > another to justify changes. > > --- > > 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(-) > > > > 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/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); > > > > 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; > > > > --- > > base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2 > > change-id: 20231016-strncpy-drivers-net-wireless-broadcom-brcm80211-brcmfmac-cfg80211-c-a20108421685 > > > > Best regards, > > -- > > Justin Stitt <justinstitt@google.com> > > Thanks Justin
On Tue, Oct 17, 2023 at 9:54 AM Kalle Valo <kvalo@kernel.org> wrote: > > Franky Lin <franky.lin@broadcom.com> writes: > > > Hi Justin, > > > > On Mon, Oct 16, 2023 at 3:14 PM Justin Stitt <justinstitt@google.com> wrote: > >> > >> strncpy() is deprecated for use on NUL-terminated destination strings > >> [1] and as such we should prefer more robust and less ambiguous string > >> interfaces. > >> > >> This patch replaces multiple strncpy uses. For easier reading, I'll list > >> each destination buffer and mention whether it requires either > >> NUL-termination or NUL-padding. > > > > Kudos to the detailed analysis of each instance. One thing I can think > > of to make this better is to split it into smaller patches so if any > > regression is observed, only the specific code causing it needs to be > > reverted. Maybe instance 2, 3, 4 can be handled in one patch since > > they are touching the country code in one file. The other instances > > each can be an individual patch. > > > > The "brcmfmac" in the title is not accurate. The change touches both > > brcmfmac and brcmsmac. So "brcm80211" would be more appropriate. > > Please also add "wifi:" to the title, see the wiki link below for more > info. Sure thing :) > > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches Thanks Justin
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/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); 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;
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. This patch replaces multiple strncpy uses. For easier reading, I'll list each destination buffer and mention whether it requires either NUL-termination or NUL-padding. 1) 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)); In this context, a suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer without unnecessarily NUL-padding. 2) 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. 3) wlc->country_default, 4) wlc->autocountry_default FWICT, these two aren't used anywhere. At any rate, let's apply the same reasoning as above and just use memcpy(). 5) 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); 6) 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); 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 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@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 --- Note: build-tested only. I've grouped these all into a single patch instead of a series as many of the replacements are related to others and rely on context from one another to justify changes. --- 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>