Re: [PATCH v3 1/3] wifi: wcn36xx: fix heap overflow from oversized firmware HAL response

[Jeff Johnson <jeff.johnson@oss.qualcomm.com>]

On 4/21/2026 6:50 AM, Tristan Madani wrote:
> From: Tristan Madani <tristan@talencesecurity.com>
> 
> The firmware response dispatcher copies all synchronous HAL responses
> into the 4096-byte hal_buf without validating the response length. A
> response exceeding WCN36XX_HAL_BUF_SIZE causes a heap buffer overflow
> with firmware-controlled content.
> 
> Add a bounds check on the response length.
> 
> Fixes: 8e84c2582169 ("wcn36xx: mac80211 driver for Qualcomm WCN3660/WCN3680 hardware")
> Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
> ---
> Changes in v3:
>   - Regenerated from wireless-next with proper git format-patch to
>     produce valid index hashes (v2 had post-processed index lines).
> 
> Changes in v2:
>   - No code changes from v1.
> 
>  drivers/net/wireless/ath/wcn36xx/smd.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> index 813553edcb789..f65328329f4f0 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -3293,6 +3293,10 @@ int wcn36xx_smd_rsp_process(struct rpmsg_device *rpdev,
>  	case WCN36XX_HAL_EXIT_IMPS_RSP:
>  	case WCN36XX_HAL_UPDATE_CHANNEL_LIST_RSP:
>  	case WCN36XX_HAL_ADD_BCN_FILTER_RSP:
> +		if (len > WCN36XX_HAL_BUF_SIZE) {
> +			wcn36xx_warn("HAL response too large: %d\n", len);
> +			break;
> +		}
>  		memcpy(wcn->hal_buf, buf, len);
>  		wcn->hal_rsp_len = len;
>  		complete(&wcn->hal_rsp_compl);

AI review points out that this logic will bypass the complete() meaning
callers waiting for completion will be stuck (either forever or until the
specified timeout expires). It proposes setting len = 0 instead of break and
having each waiter deal with the issue that wcn->hal_rsp_len is 0.

Further probing gave the observation that there is only one waiter
wcn36xx_smd_send_and_wait() so there isn't a "wait forever" scenario.

It also confirmed that setting wcn->hal_rsp_len = 0 would subsequently be
processed by wcn36xx_smd_rsp_status_check() which would return -EIO due to len
< sizeof(header) + sizeof(status_rsp).

So setting len = 0 and still calling complete() would avoid the timeout and
would cause -EIO vs -ETIME to be propagated.

I can go either way with this since it is not expected to occur.

/jeff