From patchwork Mon Oct 16 22:14:30 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Justin Stitt X-Patchwork-Id: 735018 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9B1E8CDB482 for ; Mon, 16 Oct 2023 22:14:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233728AbjJPWOf (ORCPT ); Mon, 16 Oct 2023 18:14:35 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40030 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232985AbjJPWOf (ORCPT ); Mon, 16 Oct 2023 18:14:35 -0400 Received: from mail-oa1-x49.google.com (mail-oa1-x49.google.com [IPv6:2001:4860:4864:20::49]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4D8FDA2 for ; Mon, 16 Oct 2023 15:14:32 -0700 (PDT) Received: by mail-oa1-x49.google.com with SMTP id 586e51a60fabf-1bf00f8cf77so7057978fac.0 for ; Mon, 16 Oct 2023 15:14:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1697494471; x=1698099271; darn=vger.kernel.org; h=cc:to:from:subject:message-id:mime-version:date:from:to:cc:subject :date:message-id:reply-to; bh=ALxTMHKSX9vk9LInFGXIsQlmMV2+SYhgETcxWL9x/EE=; b=V5Apc0xzqaOQbvtUB53YlnDjhvDY+irp2i3O+b04eR8IZOS9xyE4uVUESYiDCcdewA jqTlMiWlAJ/P37vr3A5hAKFRZYE7Vvf58meZc/vwts5IIrlg42/gH2W2Am4uYwwzX+Jd WmEV2bIyj1pjylMeJjx9BhmnUbPv75HzfgNjrTBS9DUcYmdzIXjtlGvHtFy0xxTT5XNV aC0C+XH1xaguL+5kFUIeJen/2hIDAQfrSmWbnvIoeVIY0cJG+MvdYmVJAF4m4Fco/pEW XCdcKqJaW/mrPxq7EvCmXF6aDEEXaT7o/tt8untE+1amPtoMbUj2x2SejkKdobaOOiq1 RokQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697494471; x=1698099271; h=cc:to:from:subject:message-id:mime-version:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=ALxTMHKSX9vk9LInFGXIsQlmMV2+SYhgETcxWL9x/EE=; b=QZ8FdoVETVAF8wjktfxF/sptbEBPyrc9xVSSaGdoMUp7emZk/X/7RJNZDhDTjiCLVm eY4gv3sxXarWFshLS8ZHAjFA+irOlYcbk3MW0aHMe8H4cHm2pUhpFOxPdrPzHHUNGa4Y dud7znqA7RQzepuuzX0oBz+xJquNBp5IFUOeo68E1pJZie8TdYZ5L5ImsxKAq4QpsviV i9Y84x/642XYP3Rwp+/I70/NLWA2bVL/JAYfstvBIzakInVt23En8DL8r5YECVhIzFh0 2A9I+9tXUxSp25QkXN53njIPpbNXcNzJjizCadEf0VC5kTbBFD2l883FLJ8i5Kaw5ahD wo2A== X-Gm-Message-State: AOJu0YyHTG31IuGHnPimUFUeNnD4ixjMfBzklp2iJTdSTUw1AAABtY1T AbqfwwzHDbJs43RK3iTN5mFz8uYK45LbkUdqMA== X-Google-Smtp-Source: AGHT+IGTkjig8ugPvK85JlzEG2yFhqIXubRd7Jd2JB/wxjUE5zKQN2nA8oXT08qQRlvT5ondf1fmt/E24ve9DaydGQ== X-Received: from jstitt-linux1.c.googlers.com ([fda3:e722:ac3:cc00:2b:ff92:c0a8:23b5]) (user=justinstitt job=sendgmr) by 2002:a05:6870:f14d:b0:1e9:ee3f:4c7c with SMTP id l13-20020a056870f14d00b001e9ee3f4c7cmr191133oac.2.1697494471600; Mon, 16 Oct 2023 15:14:31 -0700 (PDT) Date: Mon, 16 Oct 2023 22:14:30 +0000 Mime-Version: 1.0 X-B4-Tracking: v=1; b=H4sIAMW1LWUC/62OQQ6CMBBFr2K6dkynIDSuvIdhUctQmgAlU1Ilh Ltb8Qru3vvJz/+biMSeoridNsGUfPRhyqLOJ2F7MzkC32YXSqoCJVYQF57svELLPhFHmGiBl2c aKEZ4cjCtDWMGO2qpEA/qRmPBdu6XWDBKotSlwkpfRV6amTr/Pl48muy9j0vg9TiV8Jv+dz8hI JSqKLWsrZE13V0IbqBLropm3/cP8FA8kxcBAAA= X-Developer-Key: i=justinstitt@google.com; a=ed25519; pk=tC3hNkJQTpNX/gLKxTNQKDmiQl6QjBNCGKJINqAdJsE= X-Developer-Signature: v=1; a=ed25519-sha256; t=1697494469; l=8326; i=justinstitt@google.com; s=20230717; h=from:subject:message-id; bh=x6UyWSnThuXH9c2MRuctGdlyru9O2YWpxiOjh3U5t6w=; b=4kTogY9/mozkZBn6aXlySV3d0PM4HHnjkJ/jTCsj9agkf5mWF14vf29nvu4sLRN4Xnke/uhp/ twkvb6GcJ6jBM/Mw5TVhgo5SUrlzyEKSXh8YBUccPuIb5WVew1aGpO4 X-Mailer: b4 0.12.3 Message-ID: <20231016-strncpy-drivers-net-wireless-broadcom-brcm80211-brcmfmac-cfg80211-c-v2-1-6c7567e1d3b8@google.com> Subject: [PATCH v2] brcmfmac: replace deprecated strncpy From: Justin Stitt To: Arend van Spriel , Franky Lin , Hante Meuleman , Kalle Valo Cc: linux-wireless@vger.kernel.org, brcm80211-dev-list.pdl@broadcom.com, SHA-cyfmac-dev-list@infineon.com, linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org, Justin Stitt Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org 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 ""; 71 | 72 | if (ifp->ndev) 73 | return ifp->ndev->name; 74 | 75 | return ""; 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 --- 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 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;