Hi Justin,
On Mon, Oct 16, 2023 at 3:14 PM Justin Stitt <[email protected]> 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: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> 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 <[email protected]>
>
Franky Lin <[email protected]> writes:
> Hi Justin,
>
> On Mon, Oct 16, 2023 at 3:14 PM Justin Stitt <[email protected]> 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.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Tue, Oct 17, 2023 at 9:54 AM Kalle Valo <[email protected]> wrote:
>
> Franky Lin <[email protected]> writes:
>
> > Hi Justin,
> >
> > On Mon, Oct 16, 2023 at 3:14 PM Justin Stitt <[email protected]> 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