On Tue, Oct 17, 2023 at 9:49 AM Franky Lin <franky.lin@xxxxxxxxxxxx> wrote:
>
> Hi Justin,
>
> On Mon, Oct 16, 2023 at 3:14 PM Justin Stitt <justinstitt@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> > Signed-off-by: Justin Stitt <justinstitt@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx
> > ---
> > 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@xxxxxxxxxx>
> >
Thanks
Justin
