[PATCH] wifi: mwifiex: validate scan response TLV lengths

[PATCH] wifi: mwifiex: validate scan response TLV lengths

[Aamir Ahmed]

The scan response handler mwifiex_ret_802_11_scan() has three issues
when processing firmware responses:

1) bss_descript_size is not validated against resp->size before being
   used to compute tlv_buf_size.  If the firmware sends a
   bss_descript_size larger than the response, the subtraction wraps
   to a huge u32 value, causing the TLV search to read past the
   response buffer.

2) The TSF timestamp TLV data is indexed by number_of_sets * TSF_DATA_SIZE
   without checking that the TLV header.len is large enough.  A firmware
   response with number_of_sets=64 but a TSF TLV with only a few bytes
   causes an out-of-bounds heap read.

3) The channel-band TLV is indexed by number_of_sets without checking
   that the TLV has enough chan_band_param entries.  Same OOB read as
   above.

Add validation for all three: reject responses where bss_descript_size
exceeds the response, and invalidate TSF/chan-band TLVs whose reported
lengths are too short for the number of BSS entries.

Fixes: 5e6e3a92b9a4 ("wireless: mwifiex: initial commit for Marvell mwifiex driver")
Cc: stable@vger.kernel.org
Signed-off-by: Aamir Ahmed <elb12345@hotmail.co.uk>
---
 drivers/net/wireless/marvell/mwifiex/scan.c | 27 ++++++++++++++++++---
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c
index 97c0ec3b822e..4d0f3c412a90 100644
--- a/drivers/net/wireless/marvell/mwifiex/scan.c
+++ b/drivers/net/wireless/marvell/mwifiex/scan.c
@@ -2137,11 +2137,18 @@ int mwifiex_ret_802_11_scan(struct mwifiex_private *priv,
 	bss_info = scan_rsp->bss_desc_and_tlv_buffer;
 
 	/*
-	 * The size of the TLV buffer is equal to the entire command response
-	 *   size (scan_resp_size) minus the fixed fields (sizeof()'s), the
-	 *   BSS Descriptions (bss_descript_size as bytesLef) and the command
-	 *   response header (S_DS_GEN)
+	 * Validate that bss_descript_size fits within the response to
+	 * prevent an underflow in the TLV buffer size computation below.
 	 */
+	if (bytes_left + sizeof(scan_rsp->bss_descript_size) +
+	    sizeof(scan_rsp->number_of_sets) + S_DS_GEN > scan_resp_size) {
+		mwifiex_dbg(adapter, ERROR,
+			    "SCAN_RESP: bss_descript_size %u exceeds resp size %u\n",
+			    bytes_left, scan_resp_size);
+		ret = -1;
+		goto check_next_scan;
+	}
+
 	tlv_buf_size = scan_resp_size - (bytes_left
 					 + sizeof(scan_rsp->bss_descript_size)
 					 + sizeof(scan_rsp->number_of_sets)
@@ -2158,6 +2165,12 @@ int mwifiex_ret_802_11_scan(struct mwifiex_private *priv,
 					     (struct mwifiex_ie_types_data **)
 					     &tsf_tlv);
 
+	/* Validate TSF TLV has enough data for all reported BSS entries */
+	if (tsf_tlv &&
+	    le16_to_cpu(tsf_tlv->header.len) <
+	    scan_rsp->number_of_sets * TSF_DATA_SIZE)
+		tsf_tlv = NULL;
+
 	/* Search the TLV buffer space in the scan response for any valid
 	   TLVs */
 	mwifiex_ret_802_11_scan_get_tlv_ptrs(adapter, tlv_data, tlv_buf_size,
@@ -2165,6 +2178,12 @@ int mwifiex_ret_802_11_scan(struct mwifiex_private *priv,
 					     (struct mwifiex_ie_types_data **)
 					     &chan_band_tlv);
 
+	/* Validate chan-band TLV has enough entries for all reported BSS */
+	if (chan_band_tlv &&
+	    le16_to_cpu(chan_band_tlv->header.len) <
+	    scan_rsp->number_of_sets * sizeof(struct chan_band_param_set))
+		chan_band_tlv = NULL;
+
 #ifdef CONFIG_PM
 	if (priv->wdev.wiphy->wowlan_config)
 		nd_config = priv->wdev.wiphy->wowlan_config->nd_config;
-- 
2.43.0

Re: [PATCH] wifi: mwifiex: validate scan response TLV lengths

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 2 · Medium: 1 · Low: 0

- [High] Incomplete fix: the new bound check in mwifiex_ret_802_11_scan()
  (drivers/net/wireless/marvell/mwifiex/scan.c:2143-2150) validates the…
- [High] The newly added `chan_band_tlv = NULL` invalidation
  (drivers/net/wireless/marvell/mwifiex/scan.c:2182-2185) creates a…
- [Medium] The new bss_descript_size validation
  (drivers/net/wireless/marvell/mwifiex/scan.c:2143-2150) and the…

--

wifi: mwifiex: validate scan response TLV lengths

This adds three checks to mwifiex_ret_802_11_scan(): a bound check on the
firmware-supplied bss_descript_size against the response size, and
invalidation of the TSF and channel-band TLVs when their reported lengths
are too short for number_of_sets entries.  A few questions on the new
checks below.

> diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c
> index 97c0ec3b822e7..4d0f3c412a900 100644
> --- a/drivers/net/wireless/marvell/mwifiex/scan.c
> +++ b/drivers/net/wireless/marvell/mwifiex/scan.c
> @@ -2137,11 +2137,18 @@ int mwifiex_ret_802_11_scan(struct mwifiex_private *priv,
>  	bss_info = scan_rsp->bss_desc_and_tlv_buffer;
>  
>  	/*
> -	 * The size of the TLV buffer is equal to the entire command response
> -	 *   size (scan_resp_size) minus the fixed fields (sizeof()'s), the
> -	 *   BSS Descriptions (bss_descript_size as bytesLef) and the command
> -	 *   response header (S_DS_GEN)
> +	 * Validate that bss_descript_size fits within the response to
> +	 * prevent an underflow in the TLV buffer size computation below.
>  	 */
> +	if (bytes_left + sizeof(scan_rsp->bss_descript_size) +
> +	    sizeof(scan_rsp->number_of_sets) + S_DS_GEN > scan_resp_size) {
> +		mwifiex_dbg(adapter, ERROR,
> +			    "SCAN_RESP: bss_descript_size %u exceeds resp size %u\n",
> +			    bytes_left, scan_resp_size);
> +		ret = -1;
> +		goto check_next_scan;
> +	}
> +

[Severity: High]
Does this check actually bound the parse to the received buffer?  The
reference used here is scan_resp_size, which comes from
le16_to_cpu(resp->size) a few lines above, and resp->size is itself a
firmware-supplied field.

The dispatcher never reconciles it with the length of the skb that was
actually received:

drivers/net/wireless/marvell/mwifiex/cmdevt.c:mwifiex_process_cmdresp() {
	...
	resp = (struct host_cmd_ds_command *)adapter->curr_cmd->resp_skb->data;
	...
}

A grep for resp_skb->len in drivers/net/wireless/marvell/mwifiex/ returns
no matches, and the only clamp against MWIFIEX_SIZE_OF_CMD_BUFFER is on the
CMD_F_HOSTCMD copy path, which does not apply to the scan handler.  On PCIe
the response buffer is allocated at MWIFIEX_UPLD_SIZE and then trimmed to
the transport length:

drivers/net/wireless/marvell/mwifiex/pcie.c:mwifiex_pcie_process_cmd_complete() {
	...
	rx_len = get_unaligned_le16(skb->data);
	skb_put(skb, MWIFIEX_UPLD_SIZE - skb->len);
	skb_trim(skb, rx_len);
	...
}

and on USB only an upper bound on skb->len is enforced in
mwifiex_usb_recv().

So can a response that carries only a couple of kilobytes but declares
resp->size = 0xFFFF (with a small bss_descript_size) pass this new check
and leave tlv_buf_size at roughly 64 KB?

	tlv_buf_size = scan_resp_size - (bytes_left
					 + sizeof(scan_rsp->bss_descript_size)
					 + sizeof(scan_rsp->number_of_sets)
					 + S_DS_GEN);

mwifiex_ret_802_11_scan_get_tlv_ptrs() then walks TLV headers with that
bound, and mwifiex_parse_single_response_buf() consumes bytes_left
descriptor bytes, both past the end of the real allocation.  If that is
correct, the "TLV search to read past the response buffer" case described
in the changelog is still reachable, just bounded at ~64 KB rather than
unbounded.  Would it be better to anchor the check on
adapter->curr_cmd->resp_skb->len (or validate/clamp resp->size against it
in mwifiex_process_cmdresp()) instead of on resp->size?

[Severity: Medium]
Is the fixed-header term correct for the background-scan variant?  Earlier
in mwifiex_ret_802_11_scan() the response can be the nested bgscan form:

	if (is_bgscan_resp)
		scan_rsp = &resp->params.bg_scan_query_resp.scan_resp;
	else
		scan_rsp = &resp->params.scan_resp;

and fw.h defines that variant with an extra 32-bit field ahead of the
nested structure:

struct host_cmd_ds_802_11_bg_scan_query_rsp {
	__le32 report_condition;
	struct host_cmd_ds_802_11_scan_rsp scan_resp;
} __packed;

Since resp->size counts from the start of struct host_cmd_ds_command, the
fixed part for a HostCmd_CMD_802_11_BG_SCAN_QUERY response is
S_DS_GEN + 4 + 3, not S_DS_GEN + 3.  Does that mean the new check accepts a
bss_descript_size that extends four bytes past the firmware-declared
response, and that tlv_buf_size is likewise four bytes too large, so
mwifiex_ret_802_11_scan_get_tlv_ptrs() can accept a TLV header that lies
partly outside the declared payload?  The resulting header.len is then
trusted as the upper bound by the two new TSF/chan-band checks.  Would
deriving the fixed size from the selected response variant be preferable to
carrying the existing S_DS_GEN-only formula into the new guard?

>  	tlv_buf_size = scan_resp_size - (bytes_left
>  					 + sizeof(scan_rsp->bss_descript_size)
>  					 + sizeof(scan_rsp->number_of_sets)

[ ... ]

> @@ -2165,6 +2178,12 @@ int mwifiex_ret_802_11_scan(struct mwifiex_private *priv,
>  					     (struct mwifiex_ie_types_data **)
>  					     &chan_band_tlv);
>  
> +	/* Validate chan-band TLV has enough entries for all reported BSS */
> +	if (chan_band_tlv &&
> +	    le16_to_cpu(chan_band_tlv->header.len) <
> +	    scan_rsp->number_of_sets * sizeof(struct chan_band_param_set))
> +		chan_band_tlv = NULL;
> +

[Severity: High]
Can clearing chan_band_tlv here leave adapter->nd_info in an inconsistent
state?  n_matches is published from number_of_sets regardless:

	if (nd_config) {
		adapter->nd_info =
			kzalloc_flex(*adapter->nd_info, matches,
				     scan_rsp->number_of_sets, GFP_ATOMIC);

		if (adapter->nd_info)
			adapter->nd_info->n_matches = scan_rsp->number_of_sets;
	}

but the per-entry allocation in the loop below is gated on chan_band_tlv:

		if (chan_band_tlv && adapter->nd_info) {
			adapter->nd_info->matches[idx] =
				kzalloc(sizeof(*pmatch) + sizeof(u32),
					GFP_ATOMIC);

So with a short chan-band TLV, n_matches is non-zero while every
matches[i] stays NULL.  mwifiex_ret_wakeup_reason() then hands that object
to cfg80211:

drivers/net/wireless/marvell/mwifiex/cfg80211.c:mwifiex_ret_wakeup_reason() {
	...
	if (wiphy->wowlan_config->nd_config)
		wakeup_report.net_detect = adapter->nd_info;
	...
}

and nl80211 dereferences the entries without a NULL check:

net/wireless/nl80211.c:cfg80211_net_detect_results() {
	...
	for (i = 0; i < nd->n_matches; i++) {
		struct cfg80211_wowlan_nd_match *match = nd->matches[i];
		...
		if (nla_put(msg, NL80211_ATTR_SSID, match->ssid.ssid_len,
			    match->ssid.ssid)) {
	...
}

Before this hunk a short chan-band TLV still produced a non-NULL
chan_band_tlv (with the out-of-bounds read of chan_band_param[idx] the
changelog describes), so does this trade that read for a NULL dereference
on the wowlan wakeup report path?

The same inconsistency looks reachable without this patch as well, when
firmware omits the chan-band TLV entirely, when the GFP_ATOMIC kzalloc of a
match fails, or when the loop exits early on bytes_left == 0 or the parse
error goto.  Would setting n_matches after the loop to the number of
entries actually populated, or dropping nd_info when the chan-band TLV is
unusable, cover all of those?

>  #ifdef CONFIG_PM
>  	if (priv->wdev.wiphy->wowlan_config)
>  		nd_config = priv->wdev.wiphy->wowlan_config->nd_config;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/AS8P251MB0001DD15CC44F76B4BBD2853C8B32%40AS8P251MB0001.EURP251.PROD.OUTLOOK.COM