The function brcmf_usb_dl_writeimage() calls the function
brcmf_usb_dl_cmd() but dose not check its return value. The
'state.state' and the 'state.bytes' are uninitialized if the
function brcmf_usb_dl_cmd() fails. It is dangerous to use
uninitialized variables in the conditions.

Add error handling for brcmf_usb_dl_cmd() to jump to error
handling path if the brcmf_usb_dl_cmd() fails and the
'state.state' and the 'state.bytes' are uninitialized.

Improve the error message to report more detailed error
information.

Fixes: 71bb244ba2fd ("brcm80211: fmac: add USB support for bcm43235/6/8 chipsets")
Cc: stable@vger.kernel.org # v3.4+
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index 2821c27f317e..d06c724f63d9 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -896,14 +896,16 @@ brcmf_usb_dl_writeimage(struct brcmf_usbdev_info *devinfo, u8 *fw, int fwlen)
 	}
 
 	/* 1) Prepare USB boot loader for runtime image */
-	brcmf_usb_dl_cmd(devinfo, DL_START, &state, sizeof(state));
+	err = brcmf_usb_dl_cmd(devinfo, DL_START, &state, sizeof(state));
+	if (err)
+		goto fail;
 
 	rdlstate = le32_to_cpu(state.state);
 	rdlbytes = le32_to_cpu(state.bytes);
 
 	/* 2) Check we are in the Waiting state */
 	if (rdlstate != DL_WAITING) {
-		brcmf_err("Failed to DL_START\n");
+		brcmf_err("Invalid DL state: %u\n", rdlstate);
 		err = -EINVAL;
 		goto fail;
 	}
-- 
2.42.0.windows.2

…
> brcmf_usb_dl_cmd() but dose not check its return value. The
…

Please avoid typos in such a change description.


> Add error handling for brcmf_usb_dl_cmd() to jump to error
> handling path if the brcmf_usb_dl_cmd() fails and the
> 'state.state' and the 'state.bytes' are uninitialized.

This wording is improvable.


> Improve the error message to report more detailed error
> information.

Please offer such an adjustment by a separate update step.
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.15-rc3#n81

Regards,
Markus

On 4/22/2025 12:43 PM, Markus Elfring wrote:
> …
>> brcmf_usb_dl_cmd() but dose not check its return value. The
> …
> 
> Please avoid typos in such a change description.

Please avoid such a stupid comment and just indicate what you think is 
wrong and give a proper suggestion to what is should be.

> 
>> Add error handling for brcmf_usb_dl_cmd() to jump to error
>> handling path if the brcmf_usb_dl_cmd() fails and the
>> 'state.state' and the 'state.bytes' are uninitialized.
> 
> This wording is improvable.

That is quite a generic wording you use there. Speak your mind and come 
up with improved feedback.

>> Improve the error message to report more detailed error
>> information.
> 
> Please offer such an adjustment by a separate update step.
> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.15-rc3#n81

Nah. The changes are small and within context. The patch is small enough 
to assure the bar is low enough for swift review. Splitting it up only 
increases the burden.

Regards,
Arend

On April 22, 2025 6:22:48 AM Wentao Liang <vulab@iscas.ac.cn> wrote:

> The function brcmf_usb_dl_writeimage() calls the function
> brcmf_usb_dl_cmd() but dose not check its return value. The
> 'state.state' and the 'state.bytes' are uninitialized if the
> function brcmf_usb_dl_cmd() fails. It is dangerous to use
> uninitialized variables in the conditions.
>
> Add error handling for brcmf_usb_dl_cmd() to jump to error
> handling path if the brcmf_usb_dl_cmd() fails and the
> 'state.state' and the 'state.bytes' are uninitialized.
>
> Improve the error message to report more detailed error
> information.
>
> Fixes: 71bb244ba2fd ("brcm80211: fmac: add USB support for bcm43235/6/8 
> chipsets")
> Cc: stable@vger.kernel.org # v3.4+

Thanks for this patch.

Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> index 2821c27f317e..d06c724f63d9 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> @@ -896,14 +896,16 @@ brcmf_usb_dl_writeimage(struct brcmf_usbdev_info 
> *devinfo, u8 *fw, int fwlen)
>  }
>
>  /* 1) Prepare USB boot loader for runtime image */
> - brcmf_usb_dl_cmd(devinfo, DL_START, &state, sizeof(state));
> + err = brcmf_usb_dl_cmd(devinfo, DL_START, &state, sizeof(state));
> + if (err)
> + goto fail;
>
>  rdlstate = le32_to_cpu(state.state);
>  rdlbytes = le32_to_cpu(state.bytes);
>
>  /* 2) Check we are in the Waiting state */
>  if (rdlstate != DL_WAITING) {
> - brcmf_err("Failed to DL_START\n");
> + brcmf_err("Invalid DL state: %u\n", rdlstate);
>  err = -EINVAL;
>  goto fail;
>  }
> --
> 2.42.0.windows.2