[PATCH v3 0/6] wifi: mwifiex: firmware trust boundary hardening

[PATCH v3 0/6] wifi: mwifiex: firmware trust boundary hardening

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

This series adds missing bounds checks for firmware-controlled fields
in the Marvell mwifiex driver.

Patches cover: WMM queue_index, ADDBA TID, station list count, scan
response TLV lengths, multichannel intf_num, and IBSS peer TLV length.

Changes in v3:
  - Regenerated from wireless-next with proper git format-patch.

Changes in v2:
  - No code changes from v1.

Tristan Madani (6):
  wifi: mwifiex: fix OOB write from firmware queue_index in WMM status
    response
  wifi: mwifiex: fix OOB write from firmware TID in ADDBA response
    handler
  wifi: mwifiex: fix OOB read from firmware sta_count in station list
    response
  wifi: mwifiex: fix OOB read in scan response from mismatched TLV data
    sizes
  wifi: mwifiex: fix OOB read from firmware intf_num in multichannel
    event
  wifi: mwifiex: fix OOB read from inflated TLV length in IBSS peer
    event

 drivers/net/wireless/marvell/mwifiex/11n.c         |  5 +++++
 drivers/net/wireless/marvell/mwifiex/scan.c        |  9 ++++++---
 drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 10 +++++++++-
 drivers/net/wireless/marvell/mwifiex/sta_event.c   | 12 ++++++++++++
 drivers/net/wireless/marvell/mwifiex/wmm.c         |  5 +++++
 5 files changed, 37 insertions(+), 4 deletions(-)

-- 
2.47.3

[PATCH v3 1/6] wifi: mwifiex: fix OOB write from firmware queue_index in WMM status response

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

The firmware-controlled queue_index (u8) from the WMM queue status TLV
is used to index the 4-entry ac_status[] array without validation. An
out-of-range value causes out-of-bounds writes of three firmware-
controlled bytes into adjacent struct fields.

Add a bounds check before using queue_index as an array index.

Fixes: 5e6e3a92b9a4 ("wireless: mwifiex: initial commit for Marvell mwifiex driver")
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/marvell/mwifiex/wmm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/wmm.c b/drivers/net/wireless/marvell/mwifiex/wmm.c
index 841505e83c7fd..27e6dedcca2e8 100644
--- a/drivers/net/wireless/marvell/mwifiex/wmm.c
+++ b/drivers/net/wireless/marvell/mwifiex/wmm.c
@@ -943,6 +943,11 @@ int mwifiex_ret_wmm_get_status(struct mwifiex_private *priv,
 				    tlv_wmm_qstatus->flow_required,
 				    tlv_wmm_qstatus->disabled);
 
+
+			if (tlv_wmm_qstatus->queue_index >=
+			    IEEE80211_NUM_ACS) {
+				break;
+			}
 			ac_status = &priv->wmm.ac_status[tlv_wmm_qstatus->
 							 queue_index];
 			ac_status->disabled = tlv_wmm_qstatus->disabled;
-- 
2.47.3

[PATCH v3 2/6] wifi: mwifiex: fix OOB write from firmware TID in ADDBA response handler

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

The TID value extracted from the Block Ack parameter set is a 4-bit
field (0-15), but aggr_prio_tbl[] has only 8 entries. A TID >= 8 causes
an out-of-bounds write to adjacent struct mwifiex_private fields.

Add a bounds check after extracting the TID.

Fixes: 5e6e3a92b9a4 ("wireless: mwifiex: initial commit for Marvell mwifiex driver")
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/marvell/mwifiex/11n.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/11n.c b/drivers/net/wireless/marvell/mwifiex/11n.c
index cef8a55427dd0..5d97bddc71381 100644
--- a/drivers/net/wireless/marvell/mwifiex/11n.c
+++ b/drivers/net/wireless/marvell/mwifiex/11n.c
@@ -154,6 +154,11 @@ int mwifiex_ret_11n_addba_req(struct mwifiex_private *priv,
 	tid = (block_ack_param_set & IEEE80211_ADDBA_PARAM_TID_MASK)
 	       >> BLOCKACKPARAM_TID_POS;
 
+	if (tid >= MAX_NUM_TID) {
+		mwifiex_dbg(priv->adapter, ERROR,
+			    "ADDBA RSP: invalid tid %d\n", tid);
+		return -EINVAL;
+	}
 	tid_down = mwifiex_wmm_downgrade_tid(priv, tid);
 	ra_list = mwifiex_wmm_get_ralist_node(priv, tid_down, add_ba_rsp->
 		peer_mac_addr);
-- 
2.47.3

[PATCH v3 3/6] wifi: mwifiex: fix OOB read from firmware sta_count in station list response

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

The firmware-controlled sta_count (u16) is used as an unbounded loop
counter for iterating station info entries. An inflated count drives
reads past the response buffer into kernel heap memory.

Add a check that sta_count fits within the response size.

Fixes: b21783e94e20 ("mwifiex: add sta_list firmware command")
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/marvell/mwifiex/sta_cmdresp.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
index 85512f526c5f2..4cf654046c6ae 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
@@ -976,8 +976,16 @@ static int mwifiex_ret_uap_sta_list(struct mwifiex_private *priv,
 	struct mwifiex_ie_types_sta_info *sta_info = (void *)&sta_list->tlv;
 	int i;
 	struct mwifiex_sta_node *sta_node;
+	u16 resp_size = le16_to_cpu(resp->size);
+	u16 count = le16_to_cpu(sta_list->sta_count);
+	u16 max_count;
 
-	for (i = 0; i < (le16_to_cpu(sta_list->sta_count)); i++) {
+	if (resp_size < sizeof(*resp) - sizeof(resp->params) + sizeof(*sta_list))
+		return -EINVAL;
+	max_count = (resp_size - sizeof(*resp) + sizeof(resp->params) -
+		     sizeof(*sta_list)) / sizeof(*sta_info);
+	count = min(count, max_count);
+	for (i = 0; i < count; i++) {
 		sta_node = mwifiex_get_sta_entry(priv, sta_info->mac);
 		if (unlikely(!sta_node))
 			continue;
-- 
2.47.3

[PATCH v3 4/6] wifi: mwifiex: fix OOB read in scan response from mismatched TLV data sizes

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

The TSF and ChanBand TLV arrays are indexed by the firmware-controlled
number_of_sets without cross-checking against the TLV header length
fields. When number_of_sets exceeds the TLV data, the loop reads past
the TLV data into adjacent command response memory.

Stop using the TLV data once the index exceeds its reported length.

Fixes: 5e6e3a92b9a4 ("wireless: mwifiex: initial commit for Marvell mwifiex driver")
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/marvell/mwifiex/scan.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c
index 97c0ec3b822e7..059215c86dffd 100644
--- a/drivers/net/wireless/marvell/mwifiex/scan.c
+++ b/drivers/net/wireless/marvell/mwifiex/scan.c
@@ -2187,10 +2187,13 @@ int mwifiex_ret_802_11_scan(struct mwifiex_private *priv,
 		 * received.
 		 */
 		if (tsf_tlv)
-			memcpy(&fw_tsf, &tsf_tlv->tsf_data[idx * TSF_DATA_SIZE],
-			       sizeof(fw_tsf));
+			if ((idx + 1) * TSF_DATA_SIZE <=
+			    le16_to_cpu(tsf_tlv->header.len))
+				memcpy(&fw_tsf, &tsf_tlv->tsf_data[idx * TSF_DATA_SIZE],
+				       sizeof(fw_tsf));
 
-		if (chan_band_tlv) {
+		if (chan_band_tlv && (idx + 1) * sizeof(*chan_band) <=
+		    le16_to_cpu(chan_band_tlv->header.len)) {
 			chan_band = &chan_band_tlv->chan_band_param[idx];
 			radio_type = &chan_band->radio_type;
 		} else {
-- 
2.47.3

[PATCH v3 5/6] wifi: mwifiex: fix OOB read from firmware intf_num in multichannel event

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

The firmware-controlled intf_num is used to iterate the flexible array
bss_type_numlist[] without checking it against the TLV data length. An
inflated value causes out-of-bounds reads past the TLV data.

Clamp intf_num to the available TLV data.

Fixes: 8d6b538a5eac ("mwifiex: handle multichannel event")
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/marvell/mwifiex/sta_event.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/sta_event.c b/drivers/net/wireless/marvell/mwifiex/sta_event.c
index fecd88967ceb8..6b7e5b6a66a9e 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_event.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_event.c
@@ -450,6 +450,14 @@ void mwifiex_process_multi_chan_event(struct mwifiex_private *priv,
 
 		grp_info = (struct mwifiex_ie_types_mc_group_info *)tlv;
 		intf_num = grp_info->intf_num;
+		{
+			u16 fixed_len = sizeof(*grp_info) -
+					sizeof(grp_info->header);
+			if (tlv_len < fixed_len ||
+			    intf_num > tlv_len - fixed_len)
+				intf_num = 0;
+		}
+
 		for (i = 0; i < intf_num; i++) {
 			bss_type = grp_info->bss_type_numlist[i] >> 4;
 			bss_num = grp_info->bss_type_numlist[i] & BSS_NUM_MASK;
-- 
2.47.3

[PATCH v3 6/6] wifi: mwifiex: fix OOB read from inflated TLV length in IBSS peer event

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

The IBSS connected handler replaces the buffer-bounded evt_len with
the firmware-controlled TLV header length. An inflated value drives the
IE parsing loop past the event buffer into adjacent kernel heap memory.

Cap the TLV-derived length at the remaining event data size.

Fixes: 432da7d243da ("mwifiex: add HT aggregation support for adhoc mode")
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/marvell/mwifiex/sta_event.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/sta_event.c b/drivers/net/wireless/marvell/mwifiex/sta_event.c
index 6b7e5b6a66a9e..62a879c09106e 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_event.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_event.c
@@ -45,6 +45,10 @@ static int mwifiex_check_ibss_peer_capabilities(struct mwifiex_private *priv,
 		 */
 		evt_len = le16_to_cpu(tlv_mgmt_frame->header.len);
 		curr += (sizeof(*tlv_mgmt_frame) + 12);
+		if (evt_len > event->len -
+		    (curr - event->data))
+			evt_len = event->len -
+				  (curr - event->data);
 	} else {
 		mwifiex_dbg(priv->adapter, MSG,
 			    "management frame tlv not found!\n");
-- 
2.47.3

Re: [PATCH v3 1/6] wifi: mwifiex: fix OOB write from firmware queue_index in WMM status response

[Brian Norris]

Hi Tristan,

I haven't gotten through all of these yet, but so far, they seem good
aside from some cosmetic things.

Comments inline.

On Tue, Apr 21, 2026 at 01:49:33PM +0000, Tristan Madani wrote:
> From: Tristan Madani <tristan@talencesecurity.com>
> 
> The firmware-controlled queue_index (u8) from the WMM queue status TLV
> is used to index the 4-entry ac_status[] array without validation. An
> out-of-range value causes out-of-bounds writes of three firmware-
> controlled bytes into adjacent struct fields.
> 
> Add a bounds check before using queue_index as an array index.
> 
> Fixes: 5e6e3a92b9a4 ("wireless: mwifiex: initial commit for Marvell mwifiex driver")
> 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/marvell/mwifiex/wmm.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/wmm.c b/drivers/net/wireless/marvell/mwifiex/wmm.c
> index 841505e83c7fd..27e6dedcca2e8 100644
> --- a/drivers/net/wireless/marvell/mwifiex/wmm.c
> +++ b/drivers/net/wireless/marvell/mwifiex/wmm.c
> @@ -943,6 +943,11 @@ int mwifiex_ret_wmm_get_status(struct mwifiex_private *priv,
>  				    tlv_wmm_qstatus->flow_required,
>  				    tlv_wmm_qstatus->disabled);
>  
> +

Checkpatch complains about these double blank lines. Remove one.

> +			if (tlv_wmm_qstatus->queue_index >=
> +			    IEEE80211_NUM_ACS) {

Unnecessary line break.

You could also replace IEEE80211_NUM_ACS with
ARRAY_SIZE(priv->wmm.ac_status) for clarity.

> +				break;
> +			}

Might as well drop the braces, for a simple 1-liner conditional.

Also, please add a blank line after.

Thanks,
Brian

>  			ac_status = &priv->wmm.ac_status[tlv_wmm_qstatus->
>  							 queue_index];
>  			ac_status->disabled = tlv_wmm_qstatus->disabled;
> -- 
> 2.47.3
> 

Re: [PATCH v3 5/6] wifi: mwifiex: fix OOB read from firmware intf_num in multichannel event

[Brian Norris]

On Tue, Apr 21, 2026 at 01:49:37PM +0000, Tristan Madani wrote:
> From: Tristan Madani <tristan@talencesecurity.com>
> 
> The firmware-controlled intf_num is used to iterate the flexible array
> bss_type_numlist[] without checking it against the TLV data length. An
> inflated value causes out-of-bounds reads past the TLV data.
> 
> Clamp intf_num to the available TLV data.
> 
> Fixes: 8d6b538a5eac ("mwifiex: handle multichannel event")
> 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/marvell/mwifiex/sta_event.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_event.c b/drivers/net/wireless/marvell/mwifiex/sta_event.c
> index fecd88967ceb8..6b7e5b6a66a9e 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_event.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_event.c
> @@ -450,6 +450,14 @@ void mwifiex_process_multi_chan_event(struct mwifiex_private *priv,
>  
>  		grp_info = (struct mwifiex_ie_types_mc_group_info *)tlv;
>  		intf_num = grp_info->intf_num;
> +		{

I don't think it's typical style to add arbitrary context blocks /
braces just to declare a new variable. It increases the indentation
unnecesarily, for one.

I'd suggest dropping these braces and moving the 'u16 fixed_len;'
declaration up to the top of this block.

> +			u16 fixed_len = sizeof(*grp_info) -
> +					sizeof(grp_info->header);
> +			if (tlv_len < fixed_len ||
> +			    intf_num > tlv_len - fixed_len)

...then there will be less indentation and these line breaks are less
necessary.

Brian

> +				intf_num = 0;
> +		}
> +
>  		for (i = 0; i < intf_num; i++) {
>  			bss_type = grp_info->bss_type_numlist[i] >> 4;
>  			bss_num = grp_info->bss_type_numlist[i] & BSS_NUM_MASK;
> -- 
> 2.47.3
> 

Re: [PATCH v3 6/6] wifi: mwifiex: fix OOB read from inflated TLV length in IBSS peer event

[Brian Norris]

Hi Tristan,

On Tue, Apr 21, 2026 at 01:49:38PM +0000, Tristan Madani wrote:
> From: Tristan Madani <tristan@talencesecurity.com>
> 
> The IBSS connected handler replaces the buffer-bounded evt_len with
> the firmware-controlled TLV header length. An inflated value drives the
> IE parsing loop past the event buffer into adjacent kernel heap memory.
> 
> Cap the TLV-derived length at the remaining event data size.
> 
> Fixes: 432da7d243da ("mwifiex: add HT aggregation support for adhoc mode")
> 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/marvell/mwifiex/sta_event.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_event.c b/drivers/net/wireless/marvell/mwifiex/sta_event.c
> index 6b7e5b6a66a9e..62a879c09106e 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_event.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_event.c
> @@ -45,6 +45,10 @@ static int mwifiex_check_ibss_peer_capabilities(struct mwifiex_private *priv,
>  		 */
>  		evt_len = le16_to_cpu(tlv_mgmt_frame->header.len);
>  		curr += (sizeof(*tlv_mgmt_frame) + 12);
> +		if (evt_len > event->len -
> +		    (curr - event->data))
> +			evt_len = event->len -
> +				  (curr - event->data);

The above 4 line have excessive line breaks. You're not even close to 80
characters (the old-school line limit), let alone the more modern 100
limit that checkpatch uses.

Just make this 2 unbroken lines:

		if (condition)
			evt_len = ...;

Brian

>  	} else {
>  		mwifiex_dbg(priv->adapter, MSG,
>  			    "management frame tlv not found!\n");
> -- 
> 2.47.3
> 

Re: [PATCH v3 2/6] wifi: mwifiex: fix OOB write from firmware TID in ADDBA response handler

[Brian Norris]

On Tue, Apr 21, 2026 at 01:49:34PM +0000, Tristan Madani wrote:
> From: Tristan Madani <tristan@talencesecurity.com>
> 
> The TID value extracted from the Block Ack parameter set is a 4-bit
> field (0-15), but aggr_prio_tbl[] has only 8 entries. A TID >= 8 causes
> an out-of-bounds write to adjacent struct mwifiex_private fields.
> 
> Add a bounds check after extracting the TID.
> 
> Fixes: 5e6e3a92b9a4 ("wireless: mwifiex: initial commit for Marvell mwifiex driver")
> 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/marvell/mwifiex/11n.c | 5 +++++
>  1 file changed, 5 insertions(+)

Acked-by: Brian Norris <briannorris@chromium.org>

Re: [PATCH v3 3/6] wifi: mwifiex: fix OOB read from firmware sta_count in station list response

[Brian Norris]

On Tue, Apr 21, 2026 at 01:49:35PM +0000, Tristan Madani wrote:
> From: Tristan Madani <tristan@talencesecurity.com>
> 
> The firmware-controlled sta_count (u16) is used as an unbounded loop
> counter for iterating station info entries. An inflated count drives
> reads past the response buffer into kernel heap memory.
> 
> Add a check that sta_count fits within the response size.
> 
> Fixes: b21783e94e20 ("mwifiex: add sta_list firmware command")
> 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/marvell/mwifiex/sta_cmdresp.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> index 85512f526c5f2..4cf654046c6ae 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> @@ -976,8 +976,16 @@ static int mwifiex_ret_uap_sta_list(struct mwifiex_private *priv,
>  	struct mwifiex_ie_types_sta_info *sta_info = (void *)&sta_list->tlv;
>  	int i;
>  	struct mwifiex_sta_node *sta_node;
> +	u16 resp_size = le16_to_cpu(resp->size);
> +	u16 count = le16_to_cpu(sta_list->sta_count);
> +	u16 max_count;
>  
> -	for (i = 0; i < (le16_to_cpu(sta_list->sta_count)); i++) {
> +	if (resp_size < sizeof(*resp) - sizeof(resp->params) + sizeof(*sta_list))
> +		return -EINVAL;
> +	max_count = (resp_size - sizeof(*resp) + sizeof(resp->params) -
> +		     sizeof(*sta_list)) / sizeof(*sta_info);

The repeated arithmetic is a bit weird, but I'm not sure if it'd
actually be better to stash it in its own variable. Seems good enough I
suppose.

Acked-by: Brian Norris <briannorris@chromium.org>

> +	count = min(count, max_count);
> +	for (i = 0; i < count; i++) {
>  		sta_node = mwifiex_get_sta_entry(priv, sta_info->mac);
>  		if (unlikely(!sta_node))
>  			continue;
> -- 
> 2.47.3
> 

Re: [PATCH v3 4/6] wifi: mwifiex: fix OOB read in scan response from mismatched TLV data sizes

[Brian Norris]

On Tue, Apr 21, 2026 at 01:49:36PM +0000, Tristan Madani wrote:
> From: Tristan Madani <tristan@talencesecurity.com>
> 
> The TSF and ChanBand TLV arrays are indexed by the firmware-controlled
> number_of_sets without cross-checking against the TLV header length
> fields. When number_of_sets exceeds the TLV data, the loop reads past
> the TLV data into adjacent command response memory.
> 
> Stop using the TLV data once the index exceeds its reported length.
> 
> Fixes: 5e6e3a92b9a4 ("wireless: mwifiex: initial commit for Marvell mwifiex driver")
> 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/marvell/mwifiex/scan.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

My brain cries a little every time I have to read and trust this sort of
arithmetic in C.

Acked-by: Brian Norris <briannorris@chromium.org>

Re: [PATCH v3 3/6] wifi: mwifiex: fix OOB read from firmware sta_count in station list response

[Johannes Berg]

On Tue, 2026-04-21 at 13:49 +0000, Tristan Madani wrote:
> From: Tristan Madani <tristan@talencesecurity.com>
> 
> The firmware-controlled sta_count (u16) is used as an unbounded loop
> counter for iterating station info entries. An inflated count drives
> reads past the response buffer into kernel heap memory.
> 
> Add a check that sta_count fits within the response size.
> 
> Fixes: b21783e94e20 ("mwifiex: add sta_list firmware command")
> 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).

For the record, that wasn't the problem. I _think_ the problem was that
your post-processing also skipped whitespace-only lines, which are very
relevant in patches.

johannes

Re: [PATCH v3 3/6] wifi: mwifiex: fix OOB read from firmware sta_count in station list response

[Johannes Berg]

On Wed, 2026-04-22 at 11:26 -0700, Brian Norris wrote:
> 
> > +	u16 resp_size = le16_to_cpu(resp->size);
> > +	u16 count = le16_to_cpu(sta_list->sta_count);
> > +	u16 max_count;
> >  
> > -	for (i = 0; i < (le16_to_cpu(sta_list->sta_count)); i++) {
> > +	if (resp_size < sizeof(*resp) - sizeof(resp->params) + sizeof(*sta_list))
> > +		return -EINVAL;
> > +	max_count = (resp_size - sizeof(*resp) + sizeof(resp->params) -
> > +		     sizeof(*sta_list)) / sizeof(*sta_info);
> 
> The repeated arithmetic is a bit weird, but I'm not sure if it'd
> actually be better to stash it in its own variable. Seems good enough I
> suppose.

I think it might be simpler if instead trying to limit:

> > +	count = min(count, max_count);

it'd just check the needed length based on the given count, and reject
anything above that?

Also, the whole sizeof(*resp) - sizeof(resp->params) etc. shouldn't be
there, that should just be

	offsetofend(resp, sta_list.tlv)

and then suddenly it becomes _way_ simpler:

	if (resp_size < offsetofend(resp, sta_list.tlv))
		return -EINVAL;
	if (resp_size < offsetofend(resp, sta_list.tlv) +
			sizeof(*sta_info) * le16_to_cpu(sta_.list->sta_count))
		return -EINVAL;

But regardless, I question the sanity of checking the size against the
size the firmware said the whole thing was going to be, rather than
checking against the actual buffer size ...

johannes

Re: [PATCH v3 3/6] wifi: mwifiex: fix OOB read from firmware sta_count in station list response

[Brian Norris]

On Wed, Apr 22, 2026 at 09:12:11PM +0200, Johannes Berg wrote:
> On Wed, 2026-04-22 at 11:26 -0700, Brian Norris wrote:
> > 
> > > +	u16 resp_size = le16_to_cpu(resp->size);
> > > +	u16 count = le16_to_cpu(sta_list->sta_count);
> > > +	u16 max_count;
> > >  
> > > -	for (i = 0; i < (le16_to_cpu(sta_list->sta_count)); i++) {
> > > +	if (resp_size < sizeof(*resp) - sizeof(resp->params) + sizeof(*sta_list))
> > > +		return -EINVAL;
> > > +	max_count = (resp_size - sizeof(*resp) + sizeof(resp->params) -
> > > +		     sizeof(*sta_list)) / sizeof(*sta_info);
> > 
> > The repeated arithmetic is a bit weird, but I'm not sure if it'd
> > actually be better to stash it in its own variable. Seems good enough I
> > suppose.
> 
> I think it might be simpler if instead trying to limit:
> 
> > > +	count = min(count, max_count);
> 
> it'd just check the needed length based on the given count, and reject
> anything above that?

That might be better.

> Also, the whole sizeof(*resp) - sizeof(resp->params) etc. shouldn't be
> there, that should just be
> 
> 	offsetofend(resp, sta_list.tlv)

TIL. I don't recall seeing that macro before. Or at least, I didn't know
it well enough to recommend it.

> and then suddenly it becomes _way_ simpler:
> 
> 	if (resp_size < offsetofend(resp, sta_list.tlv))
> 		return -EINVAL;
> 	if (resp_size < offsetofend(resp, sta_list.tlv) +
> 			sizeof(*sta_info) * le16_to_cpu(sta_.list->sta_count))
> 		return -EINVAL;

Looks good to me.

> But regardless, I question the sanity of checking the size against the
> size the firmware said the whole thing was going to be, rather than
> checking against the actual buffer size ...

Admittedly, I get lost in this driver sometimes...
...but I think you have a very good point. AFAICT, we never do anything
to check the size of adapter->curr_cmd->resp_skb. We generally assume
it's big enough to fit 'struct host_cmd_ds_command' (since we allocate
it ourselves). But we don't ever go back to check these
dynamically-sized fields don't overflow it.

Brian

Re: [PATCH v3 3/6] wifi: mwifiex: fix OOB read from firmware sta_count in station list response

[Johannes Berg]

On Wed, 2026-04-22 at 12:54 -0700, Brian Norris wrote:
> > But regardless, I question the sanity of checking the size against the
> > size the firmware said the whole thing was going to be, rather than
> > checking against the actual buffer size ...
> 
> Admittedly, I get lost in this driver sometimes...
> ...but I think you have a very good point. AFAICT, we never do anything
> to check the size of adapter->curr_cmd->resp_skb. We generally assume
> it's big enough to fit 'struct host_cmd_ds_command' (since we allocate
> it ourselves). But we don't ever go back to check these
> dynamically-sized fields don't overflow it.
> 

There are some (response) buffers where the size is checked before
copying, but I didn't trace this back further than the SKB coming from
pcie/sdio/usb, but I don't see any check of the firmware-advertised size
vs. the actual skb->len.

johannes

Re: [PATCH v3 3/6] wifi: mwifiex: fix OOB read from firmware sta_count in station list response

[Johannes Berg]

On Wed, 2026-04-22 at 21:57 +0200, Johannes Berg wrote:
> On Wed, 2026-04-22 at 12:54 -0700, Brian Norris wrote:
> > > But regardless, I question the sanity of checking the size against the
> > > size the firmware said the whole thing was going to be, rather than
> > > checking against the actual buffer size ...
> > 
> > Admittedly, I get lost in this driver sometimes...
> > ...but I think you have a very good point. AFAICT, we never do anything
> > to check the size of adapter->curr_cmd->resp_skb. We generally assume
> > it's big enough to fit 'struct host_cmd_ds_command' (since we allocate
> > it ourselves). But we don't ever go back to check these
> > dynamically-sized fields don't overflow it.
> > 
> 
> There are some (response) buffers where the size is checked before
> copying, but I didn't trace this back further than the SKB coming from
> pcie/sdio/usb, but I don't see any check of the firmware-advertised size
> vs. the actual skb->len.
> 

In PCIe for example it looks like there are multiple length fields, and
various mwifiex_map_pci_memory() calls with different sizes

 - MWIFIEX_UPLD_SIZE (2312)
 - MWIFIEX_RX_DATA_BUF_SIZE (4k)
 - MAX_EVENT_SIZE (2k)

If we assume strict iommu we'll get protection there (even if bounce-
buffered due to the weird sizes).

I don't see however any cross-check of the cmd_resp->size vs. the actual
size. If we had _that_ then we could rely on the cmd_resp->size later I
guess...

This all seems way too complicated anyway - should probably only ever
have whole pages allocated, for example.

johannes