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

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.

[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)

[ ... ]

[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