Re: [PATCH] wifi: brcmfmac: Replace strlcat() with snprintf() in brcmf_fw_alloc_request()

[Kees Cook <kees@kernel.org>]

On Tue, Oct 13, 2026 at 05:55:52PM -0700, Mariia Nikitash wrote:
> From: Bill Wendling <morbo@google.com>
> 
> brcmf_fw_alloc_request() constructs firmware file paths by chaining
> strscpy() and multiple strlcat() calls for the module parameter path,
> separator, base name, and extension.
> 
> In preparation for removing the deprecated strlcat() API[1], format the
> complete firmware path in a single snprintf() call.
> 
> Link: https://github.com/KSPP/linux/issues/370 [1]
> Cc: codemender-patching+linux@google.com
> Assisted-by: LLM
> Signed-off-by: Bill Wendling <morbo@google.com>
> Signed-off-by: Mariia Nikitash <nikitash.mariiaw@gmail.com>
> ---
> 
> This patch was originally part of Bill Wendling's strlcat() removal
> series [2], and I am shepherding it individually to the subsystem
> maintainers.
> 
> No code changes from the original posting [3].
> 
> [2] https://lore.kernel.org/all/20260915081910.4142719-1-morbo@google.com/
> [3] https://lore.kernel.org/all/20260915081910.4142719-12-morbo@google.com/
> 
>  .../broadcom/brcm80211/brcmfmac/firmware.c    | 21 +++++++------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> index 22ff326f1924..2f74a952599e 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> @@ -845,22 +845,17 @@ brcmf_fw_alloc_request(u32 chip, u32 chiprev,
>  	fwreq->n_items = n_fwnames;
>  
>  	for (j = 0; j < n_fwnames; j++) {
> -		fwreq->items[j].path = fwnames[j].path;

yes, this is redundant to the final assignment.

> -		fwnames[j].path[0] = '\0';

this pre-truncation isn't needed either.

(Maybe this code used to have a fast-exit path that left stuff
unwritten?)

>  		/* check if firmware path is provided by module parameter */
>  		if (brcmf_mp_global.firmware_path[0] != '\0') {

This check is confusing, I would expect mp_path to be tested instead.

> -			strscpy(fwnames[j].path, mp_path,
> -				BRCMF_FW_NAME_LEN);
> -
> -			if (end != '/') {
> -				strlcat(fwnames[j].path, "/",
> -					BRCMF_FW_NAME_LEN);
> -			}
> +			snprintf(fwnames[j].path, BRCMF_FW_NAME_LEN, "%s%s%s%s",
> +				 mp_path, (end == '/') ? "" : "/",
> +				 mapping_table[i].fw_base,
> +				 fwnames[j].extension);
> +		} else {
> +			snprintf(fwnames[j].path, BRCMF_FW_NAME_LEN, "%s%s",
> +				 mapping_table[i].fw_base,
> +				 fwnames[j].extension);
>  		}

I don't see any reason not to fully collapse this, as the test is just
examining mp_path and doing end checking:

		snprintf(fwnames[j].path, BRCMF_FW_NAME_LEN, "%s%s%s%s",
			 mp_path ?: "",
			 mp_path && end != '/' ? "/" : "",
			 mapping_table[i].fw_base,
			 fwnames[j].extension);


> -		strlcat(fwnames[j].path, mapping_table[i].fw_base,
> -			BRCMF_FW_NAME_LEN);
> -		strlcat(fwnames[j].path, fwnames[j].extension,
> -			BRCMF_FW_NAME_LEN);
>  		fwreq->items[j].path = fwnames[j].path;
>  	}
>  
> -- 
> 2.43.0
> 

-Kees

-- 
Kees Cook