| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20231016-strncpy-drivers-net-wireless-broadcom-brcm80211-brcmfmac-cfg80211-c-v2-1-6c7567e1d3b8@google.com>
Date: Mon, 16 Oct 2023 22:14:30 +0000
From: Justin Stitt <justinstitt@...gle.com>
To: Arend van Spriel <aspriel@...il.com>,
Franky Lin <franky.lin@...adcom.com>,
Hante Meuleman <hante.meuleman@...adcom.com>,
Kalle Valo <kvalo@...nel.org>
Cc: 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,
Justin Stitt <justinstitt@...gle.com>
Subject: [PATCH v2] brcmfmac: replace deprecated strncpy
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@...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>
Powered by blists - more mailing lists