Message-ID: <CAFhGd8pxU=ZS3hodJxP5BPpf21rq66KKKNU9MHouimAjX+wHLw@mail.gmail.com>
Date: Tue, 17 Oct 2023 10:28:49 -0700
From: Justin Stitt <justinstitt@...gle.com>
To: Franky Lin <franky.lin@...adcom.com>
Cc: Arend van Spriel <aspriel@...il.com>, Hante Meuleman <hante.meuleman@...adcom.com>, 
	Kalle Valo <kvalo@...nel.org>, linux-wireless@...r.kernel.org, 
	brcm80211-dev-list.pdl@...adcom.com, SHA-cyfmac-dev-list@...ineon.com, 
	linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2] brcmfmac: replace deprecated strncpy

On Tue, Oct 17, 2023 at 9:49 AM Franky Lin <franky.lin@...adcom.com> wrote:
>
>  Hi Justin,
>
> On Mon, Oct 16, 2023 at 3:14 PM Justin Stitt <justinstitt@...gle.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@...r.kernel.org
> > Signed-off-by: Justin Stitt <justinstitt@...gle.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@...gle.com>
> >

Thanks
Justin

Powered by blists - more mailing lists