[PATCH] wifi: mwifiex: serialize synchronous command buffer access

[PATCH] wifi: mwifiex: serialize synchronous command buffer access

[Jaeyeong Lee]

Commit ef06882c7d8a ("wifi: mwifiex: Detach sync cmd buffer on
interrupted wait") clears a synchronous command data_buf after an
interrupted or timed-out wait. The response side does not hold
mwifiex_cmd_lock while using that pointer, however. It can cache
data_buf before the clear, then write to the caller stack after the
waiter returns. If the clear wins, three response handlers instead
dereference NULL.

The normal cfg80211 path is affected. An unprivileged
NL80211_CMD_GET_INTERFACE request on a connected HT interface reaches
get_channel, which passes a stack channel_band to a synchronous
STA_CONFIGURE command. Its response writes through data_buf. Raw host
commands have the same check/use window.

Move the detach into mwifiex_cancel_all_pending_cmd() while
mwifiex_cmd_lock is held and before completing the command. Dispatch
only the short, non-sleeping response handlers that copy to data_buf
under the same lock through their final write. Protect the VERSION_EXT
copy separately because its other branch queues a command. Serialize
the raw host-command copy as well, and let the three unconditional
handlers discard a detached buffer.

If cancellation wins the lock, a later response observes NULL. If the
response wins it, cancellation cannot wake the buffer owner until
copyout has completed.

Compile-tested with LLVM 22.1.8 and W=1. Runtime testing was not possible
because no supported mwifiex hardware was available.

Fixes: 3d026d09b28d ("mwifiex: cancel pending commands for signal")
Cc: stable@vger.kernel.org
Signed-off-by: Jaeyeong Lee <lee@jaeyeong.cc>
---
 drivers/net/wireless/marvell/mwifiex/cmdevt.c |   8 ++
 .../wireless/marvell/mwifiex/sta_cmdresp.c    | 119 ++++++++++++------
 .../net/wireless/marvell/mwifiex/sta_ioctl.c  |  12 --
 3 files changed, 89 insertions(+), 50 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
index 6e04d7da6217..bf462dd17c52 100644
--- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
+++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
@@ -838,11 +838,14 @@ int mwifiex_process_cmdresp(struct mwifiex_adapter *adapter)
 		mwifiex_dbg(adapter, INFO,
 			    "info: host cmd resp size = %d\n", size);
 		size = min_t(u16, size, MWIFIEX_SIZE_OF_CMD_BUFFER);
+		/* Serialize against cancellation of the caller-owned buffer. */
+		spin_lock_bh(&adapter->mwifiex_cmd_lock);
 		if (adapter->curr_cmd->data_buf) {
 			hostcmd = adapter->curr_cmd->data_buf;
 			hostcmd->len = size;
 			memcpy(hostcmd->cmd, resp, size);
 		}
+		spin_unlock_bh(&adapter->mwifiex_cmd_lock);
 	}
 
 	/* Get BSS number and corresponding priv */
@@ -1050,6 +1053,8 @@ mwifiex_cancel_all_pending_cmd(struct mwifiex_adapter *adapter)
 	spin_lock_bh(&adapter->mwifiex_cmd_lock);
 	/* Cancel current cmd */
 	if ((adapter->curr_cmd) && (adapter->curr_cmd->wait_q_enabled)) {
+		/* Do not wake the owner while a response can still use its data. */
+		adapter->curr_cmd->data_buf = NULL;
 		adapter->cmd_wait_q.status = -1;
 		mwifiex_complete_cmd(adapter, adapter->curr_cmd);
 		adapter->curr_cmd->wait_q_enabled = false;
@@ -1702,6 +1707,9 @@ int mwifiex_ret_wakeup_reason(struct mwifiex_private *priv,
 			      struct host_cmd_ds_command *resp,
 			      struct host_cmd_ds_wakeup_reason *wakeup_reason)
 {
+	if (!wakeup_reason)
+		return 0;
+
 	wakeup_reason->wakeup_reason =
 		resp->params.hs_wakeup_reason.wakeup_reason;
 
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
index 85512f526c5f..e59f52e97d7c 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
@@ -691,12 +691,14 @@ static int mwifiex_ret_802_11d_domain_info(struct mwifiex_private *priv,
  * to application.
  */
 static int mwifiex_ret_ver_ext(struct mwifiex_private *priv,
-			       struct host_cmd_ds_command *resp,
-			       struct host_cmd_ds_version_ext *version_ext)
+			       struct host_cmd_ds_command *resp)
 {
+	struct mwifiex_adapter *adapter = priv->adapter;
 	struct host_cmd_ds_version_ext *ver_ext = &resp->params.verext;
+	struct host_cmd_ds_version_ext *version_ext = NULL;
 
-	if (test_and_clear_bit(MWIFIEX_IS_REQUESTING_FW_VEREXT, &priv->adapter->work_flags)) {
+	if (test_and_clear_bit(MWIFIEX_IS_REQUESTING_FW_VEREXT,
+			       &adapter->work_flags)) {
 		if (strncmp(ver_ext->version_str, "ChipRev:20, BB:9b(10.00), RF:40(21)",
 			    MWIFIEX_VERSION_STR_LENGTH) == 0) {
 			struct mwifiex_ds_auto_ds auto_ds = {
@@ -716,6 +718,9 @@ static int mwifiex_ret_ver_ext(struct mwifiex_private *priv,
 		return 0;
 	}
 
+	spin_lock_bh(&adapter->mwifiex_cmd_lock);
+	if (adapter->curr_cmd)
+		version_ext = adapter->curr_cmd->data_buf;
 	if (version_ext) {
 		version_ext->version_str_sel = ver_ext->version_str_sel;
 		memcpy(version_ext->version_str, ver_ext->version_str,
@@ -726,6 +731,8 @@ static int mwifiex_ret_ver_ext(struct mwifiex_private *priv,
 		/* Ensure the version string from the firmware is 0-terminated */
 		priv->version_str[MWIFIEX_VERSION_STR_LENGTH - 1] = '\0';
 	}
+	spin_unlock_bh(&adapter->mwifiex_cmd_lock);
+
 	return 0;
 }
 
@@ -1024,6 +1031,9 @@ static int mwifiex_ret_robust_coex(struct mwifiex_private *priv,
 	u16 action = le16_to_cpu(coex->action);
 	u32 mode;
 
+	if (!is_timeshare)
+		return 0;
+
 	coex_tlv = (struct mwifiex_ie_types_robust_coex
 		    *)((u8 *)coex + sizeof(struct host_cmd_ds_robust_coex));
 	if (action == HostCmd_ACT_GEN_GET) {
@@ -1194,6 +1204,9 @@ static int mwifiex_ret_get_chan_info(struct mwifiex_private *priv,
 	struct host_cmd_ds_sta_configure *sta_cfg_cmd = &resp->params.sta_cfg;
 	struct host_cmd_tlv_channel_band *tlv_band_channel;
 
+	if (!channel_band)
+		return 0;
+
 	tlv_band_channel =
 	(struct host_cmd_tlv_channel_band *)sta_cfg_cmd->tlv_buffer;
 	memcpy(&channel_band->band_config, &tlv_band_channel->band_config,
@@ -1203,6 +1216,54 @@ static int mwifiex_ret_get_chan_info(struct mwifiex_private *priv,
 	return 0;
 }
 
+static bool
+mwifiex_process_sta_cmdresp_data_buf(struct mwifiex_private *priv,
+				     u16 cmdresp_no,
+				     struct host_cmd_ds_command *resp,
+				     void *data_buf, int *ret)
+{
+	lockdep_assert_held(&priv->adapter->mwifiex_cmd_lock);
+
+	switch (cmdresp_no) {
+	case HostCmd_CMD_802_11_PS_MODE_ENH:
+		*ret = mwifiex_ret_enh_power_mode(priv, resp, data_buf);
+		break;
+	case HostCmd_CMD_802_11_GET_LOG:
+		*ret = mwifiex_ret_get_log(priv, resp, data_buf);
+		break;
+	case HostCmd_CMD_802_11_SNMP_MIB:
+		*ret = mwifiex_ret_802_11_snmp_mib(priv, resp, data_buf);
+		break;
+	case HostCmd_CMD_REMAIN_ON_CHAN:
+		*ret = mwifiex_ret_remain_on_chan(priv, resp, data_buf);
+		break;
+	case HostCmd_CMD_P2P_MODE_CFG:
+		*ret = mwifiex_ret_p2p_mode_cfg(priv, resp, data_buf);
+		break;
+	case HostCmd_CMD_MAC_REG_ACCESS:
+	case HostCmd_CMD_BBP_REG_ACCESS:
+	case HostCmd_CMD_RF_REG_ACCESS:
+	case HostCmd_CMD_PMIC_REG_ACCESS:
+	case HostCmd_CMD_CAU_REG_ACCESS:
+	case HostCmd_CMD_802_11_EEPROM_ACCESS:
+		*ret = mwifiex_ret_reg_access(cmdresp_no, resp, data_buf);
+		break;
+	case HostCmd_CMD_HS_WAKEUP_REASON:
+		*ret = mwifiex_ret_wakeup_reason(priv, resp, data_buf);
+		break;
+	case HostCmd_CMD_ROBUST_COEX:
+		*ret = mwifiex_ret_robust_coex(priv, resp, data_buf);
+		break;
+	case HostCmd_CMD_STA_CONFIGURE:
+		*ret = mwifiex_ret_get_chan_info(priv, resp, data_buf);
+		break;
+	default:
+		return false;
+	}
+
+	return true;
+}
+
 /*
  * This function handles the command responses.
  *
@@ -1214,13 +1275,27 @@ int mwifiex_process_sta_cmdresp(struct mwifiex_private *priv, u16 cmdresp_no,
 {
 	int ret = 0;
 	struct mwifiex_adapter *adapter = priv->adapter;
-	void *data_buf = adapter->curr_cmd->data_buf;
+	void *data_buf;
 
 	/* If the command is not successful, cleanup and return failure */
 	if (resp->result != HostCmd_RESULT_OK) {
 		mwifiex_process_cmdresp_error(priv, resp);
 		return -1;
 	}
+
+	/*
+	 * Cancellation clears data_buf and wakes its owner under this lock.
+	 * Keep it held through copyout so the owner cannot release the buffer.
+	 */
+	spin_lock_bh(&adapter->mwifiex_cmd_lock);
+	data_buf = adapter->curr_cmd ? adapter->curr_cmd->data_buf : NULL;
+	if (mwifiex_process_sta_cmdresp_data_buf(priv, cmdresp_no, resp,
+						 data_buf, &ret)) {
+		spin_unlock_bh(&adapter->mwifiex_cmd_lock);
+		return ret;
+	}
+	spin_unlock_bh(&adapter->mwifiex_cmd_lock);
+
 	/* Command successful, handle response */
 	switch (cmdresp_no) {
 	case HostCmd_CMD_GET_HW_SPEC:
@@ -1265,9 +1340,6 @@ int mwifiex_process_sta_cmdresp(struct mwifiex_private *priv, u16 cmdresp_no,
 	case HostCmd_CMD_RF_ANTENNA:
 		ret = mwifiex_ret_rf_antenna(priv, resp);
 		break;
-	case HostCmd_CMD_802_11_PS_MODE_ENH:
-		ret = mwifiex_ret_enh_power_mode(priv, resp, data_buf);
-		break;
 	case HostCmd_CMD_802_11_HS_CFG_ENH:
 		ret = mwifiex_ret_802_11_hs_cfg(priv, resp);
 		break;
@@ -1284,32 +1356,20 @@ int mwifiex_process_sta_cmdresp(struct mwifiex_private *priv, u16 cmdresp_no,
 	case HostCmd_CMD_802_11_AD_HOC_STOP:
 		ret = mwifiex_ret_802_11_ad_hoc_stop(priv, resp);
 		break;
-	case HostCmd_CMD_802_11_GET_LOG:
-		ret = mwifiex_ret_get_log(priv, resp, data_buf);
-		break;
 	case HostCmd_CMD_RSSI_INFO:
 		ret = mwifiex_ret_802_11_rssi_info(priv, resp);
 		break;
-	case HostCmd_CMD_802_11_SNMP_MIB:
-		ret = mwifiex_ret_802_11_snmp_mib(priv, resp, data_buf);
-		break;
 	case HostCmd_CMD_802_11_TX_RATE_QUERY:
 		ret = mwifiex_ret_802_11_tx_rate_query(priv, resp);
 		break;
 	case HostCmd_CMD_VERSION_EXT:
-		ret = mwifiex_ret_ver_ext(priv, resp, data_buf);
-		break;
-	case HostCmd_CMD_REMAIN_ON_CHAN:
-		ret = mwifiex_ret_remain_on_chan(priv, resp, data_buf);
+		ret = mwifiex_ret_ver_ext(priv, resp);
 		break;
 	case HostCmd_CMD_11AC_CFG:
 		break;
 	case HostCmd_CMD_PACKET_AGGR_CTRL:
 		ret = mwifiex_ret_pkt_aggr_ctrl(priv, resp);
 		break;
-	case HostCmd_CMD_P2P_MODE_CFG:
-		ret = mwifiex_ret_p2p_mode_cfg(priv, resp, data_buf);
-		break;
 	case HostCmd_CMD_MGMT_FRAME_REG:
 	case HostCmd_CMD_FUNC_INIT:
 	case HostCmd_CMD_FUNC_SHUTDOWN:
@@ -1363,15 +1423,7 @@ int mwifiex_process_sta_cmdresp(struct mwifiex_private *priv, u16 cmdresp_no,
 		ret = mwifiex_ret_ibss_coalescing_status(priv, resp);
 		break;
 	case HostCmd_CMD_MEM_ACCESS:
-		ret = mwifiex_ret_mem_access(priv, resp, data_buf);
-		break;
-	case HostCmd_CMD_MAC_REG_ACCESS:
-	case HostCmd_CMD_BBP_REG_ACCESS:
-	case HostCmd_CMD_RF_REG_ACCESS:
-	case HostCmd_CMD_PMIC_REG_ACCESS:
-	case HostCmd_CMD_CAU_REG_ACCESS:
-	case HostCmd_CMD_802_11_EEPROM_ACCESS:
-		ret = mwifiex_ret_reg_access(cmdresp_no, resp, data_buf);
+		ret = mwifiex_ret_mem_access(priv, resp, NULL);
 		break;
 	case HostCmd_CMD_SET_BSS_MODE:
 		break;
@@ -1416,22 +1468,13 @@ int mwifiex_process_sta_cmdresp(struct mwifiex_private *priv, u16 cmdresp_no,
 	case HostCmd_CMD_SDIO_SP_RX_AGGR_CFG:
 		ret = mwifiex_ret_sdio_rx_aggr_cfg(priv, resp);
 		break;
-	case HostCmd_CMD_HS_WAKEUP_REASON:
-		ret = mwifiex_ret_wakeup_reason(priv, resp, data_buf);
-		break;
 	case HostCmd_CMD_TDLS_CONFIG:
 		break;
-	case HostCmd_CMD_ROBUST_COEX:
-		ret = mwifiex_ret_robust_coex(priv, resp, data_buf);
-		break;
 	case HostCmd_CMD_GTK_REKEY_OFFLOAD_CFG:
 		break;
 	case HostCmd_CMD_CHAN_REGION_CFG:
 		ret = mwifiex_ret_chan_region_cfg(priv, resp);
 		break;
-	case HostCmd_CMD_STA_CONFIGURE:
-		ret = mwifiex_ret_get_chan_info(priv, resp, data_buf);
-		break;
 	default:
 		mwifiex_dbg(adapter, ERROR,
 			    "CMD_RESP: unknown cmd response %#x\n",
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c b/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c
index 1d3874734851..51cdaecca2a2 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c
@@ -57,18 +57,6 @@ int mwifiex_wait_queue_complete(struct mwifiex_adapter *adapter,
 		mwifiex_dbg(adapter, ERROR, "cmd_wait_q terminated: %d\n",
 			    status);
 		mwifiex_cancel_all_pending_cmd(adapter);
-
-		/* The command response path writes through cmd_node->data_buf.
-		 * On an interrupted wait, the caller can return and release a
-		 * stack-allocated data_buf before a late firmware response is
-		 * processed. Detach the caller-owned buffer from the current
-		 * command so a late response cannot corrupt freed stack memory.
-		 */
-		spin_lock_bh(&adapter->mwifiex_cmd_lock);
-		if (adapter->curr_cmd == cmd_queued)
-			adapter->curr_cmd->data_buf = NULL;
-		spin_unlock_bh(&adapter->mwifiex_cmd_lock);
-
 		return status;
 	}
 
-- 
2.43.0

Re: [PATCH] wifi: mwifiex: serialize synchronous command buffer access

[Francesco Dolcini]

On Tue, Oct 27, 2026 at 06:57:07AM +0000, Jaeyeong Lee wrote:

[...]

> Compile-tested with LLVM 22.1.8 and W=1. Runtime testing was not possible
> because no supported mwifiex hardware was available.

What's the point of such a patch you did not even test?
No, thanks?

Francesco