wifi: fix PMSR lifetime and frame validation issues

Hi,

This series fixes a set of PMSR lifetime and management-frame validation issues found while reviewing the wireless stack.

It is based on wireless/main commit cb8afea4655f ("wifi: cfg80211: bound element ID read when checking non-inheritance").

The patches cover:

- PMSR lifetime handling in mac80211_hwsim and iwlwifi MLD: hwsim drops its saved cfg80211 request pointers once an abort succeeds, hwsim reports are accepted only from the registered wmediumd sender in the matching netgroup, and iwlwifi MLD gains an abort hook for active FTM PMSR requests.
- cfg80211 parsing of the optional FTM NOMINAL_TIME attribute.
- fixed-field validation for mac80211 probe-response countdown offsets, S1G individual TWT setup frames before driver callbacks, and mwifiex action frame category/action-code fields.
- an nl80211 color-change error path that leaked the beacon data parsed for the request; it is now freed as in the other beacon setters.

A larger local draft covered more issues, but the AP-template double-free, MLE and assoc/MLME length validation, S1G beacon TSF, and RNR cleanup fixes have since landed in wireless/main and are dropped here.

Checks run:

- git am of patches 1-8 on wireless/main cb8afea4655f: clean
- scripts/checkpatch.pl --strict --no-signoff on this draft: clean
- VPS x86_64 KASAN build: bzImage plus iwlwifi MLD and mwifiex modules build clean
- VPS QEMU boot of the patched KASAN+hwsim kernel: a PMSR-capable hwsim radio completed PMSR_START; the stale REPORT_PMSR attempt was rejected with -EINVAL; no KASAN, use-after-free, out-of-bounds, general protection, or "unable to handle" splat was observed

Zhao Li (8):
wifi: mac80211_hwsim: clear PMSR request state on abort
wifi: mac80211_hwsim: authenticate PMSR report senders
wifi: iwlwifi: mld: abort active PMSR requests
wifi: cfg80211: guard optional PMSR nominal time
wifi: mac80211: validate probe response countdown offsets
wifi: mac80211: validate S1G TWT params before driver setup
wifi: mwifiex: validate action frame fixed fields
wifi: nl80211: clean up color-change beacon data on errors

 .../intel/iwlwifi/mld/ftm-initiator.c         | 22 +++++++++++++++++++
 .../intel/iwlwifi/mld/ftm-initiator.h         |  2 ++
 .../net/wireless/intel/iwlwifi/mld/mac80211.c | 10 +++++++++
 drivers/net/wireless/marvell/mwifiex/util.c   |  9 ++++++++
 .../wireless/virtual/mac80211_hwsim_main.c    | 19 +++++++++++++---
 net/mac80211/s1g.c                            |  4 ++++
 net/mac80211/tx.c                             |  4 ++++
 net/wireless/nl80211.c                        | 10 +++++----
 net/wireless/pmsr.c                           |  5 +++--
 9 files changed, 76 insertions(+), 9 deletions(-)

2.50.1 (Apple Git-155)

mac80211_hwsim saves the in-flight cfg80211 PMSR request and its wdev in data->pmsr_request / data->pmsr_request_wdev when a measurement starts, and clears them only when it reports completion.

mac80211_hwsim_abort_pmsr() never cleared that saved state. cfg80211 owns the request and frees it once the abort callback returns (cfg80211_pmsr_process_abort() calls rdev_abort_pmsr() then kfree(req)), so after an abort data->pmsr_request dangles. A later hwsim PMSR report then dereferences the freed request in hwsim_pmsr_report_nl() and completes it; a use-after-free.

Clear data->pmsr_request and data->pmsr_request_wdev once the abort matches the active request. Move the wmediumd/virtio notification check below the clear so the saved state is dropped even when no notification is sent.

---
 drivers/net/wireless/virtual/mac80211_hwsim_main.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

index 956ff9b94526d..bc0818b525224 100644
@@ -3841,9 +3841,6 @@ static void mac80211_hwsim_abort_pmsr(struct ieee80211_hw *hw,
 	int err = 0;
 
 	data = hw->priv;
-	_portid = READ_ONCE(data->wmediumd);
-	if (!_portid && !hwsim_virtio_enabled)
-		return;
 
 	mutex_lock(&data->mutex);
 

@@ -3852,6 +3849,13 @@ static void mac80211_hwsim_abort_pmsr(struct ieee80211_hw *hw,
 		goto out;
 	}
 
+	data->pmsr_request = NULL;
+	data->pmsr_request_wdev = NULL;
+
+	_portid = READ_ONCE(data->wmediumd);
+	if (!_portid && !hwsim_virtio_enabled)
+		goto out;
+
 	skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!skb) {
 		err = -ENOMEM;

2.50.1 (Apple Git-155)

hwsim_pmsr_report_nl() looks up the radio by HWSIM_ATTR_ADDR_TRANSMITTER and, when data->pmsr_request is set, parses the reported peer results, hands them to cfg80211_pmsr_report(), then unconditionally clears data->pmsr_request and calls cfg80211_pmsr_complete() to end the measurement.

Unlike the sibling wmediumd data-path handlers hwsim_tx_info_frame_received_nl() and hwsim_cloned_frame_received_nl(), which check the sending socket's netgroup against data->netgroup and its portid against data->wmediumd, this handler did not check the sender at all, and its genl op carries no GENL_UNS_ADMIN_PERM flag. In non-virtio (wmediumd) mode any process in the netns that can reach the hwsim generic netlink family could therefore send a report. The transmitter address is not secret, so such a process could inject spoofed ranging results for another radio's in-flight request and, because the handler always completes the measurement, terminate a ranging operation owned by the real wmediumd session.

Reject reports whose sender does not match the registered wmediumd instance, mirroring the sibling handlers: in non-virtio mode require the sending socket's netgroup to equal data->netgroup and info->snd_portid to equal data->wmediumd before touching the request state.

---
 drivers/net/wireless/virtual/mac80211_hwsim_main.c | 9 +++++++++
 1 file changed, 9 insertions(+)

index bc0818b525224..6bf9736098b67 100644
@@ -4211,6 +4211,15 @@ static int hwsim_pmsr_report_nl(struct sk_buff *msg, struct genl_info *info)
 	if (!data)
 		return -EINVAL;
 
+	if (!hwsim_virtio_enabled) {
+		if (hwsim_net_get_netgroup(genl_info_net(info)) !=
+		    data->netgroup)
+			return -EINVAL;
+
+		if (info->snd_portid != data->wmediumd)
+			return -EPERM;
+	}
+
 	mutex_lock(&data->mutex);
 	if (!data->pmsr_request) {
 		err = -EINVAL;

2.50.1 (Apple Git-155)

The iwlwifi MLD driver registers .start_pmsr (iwl_mld_start_pmsr -> iwl_mld_ftm_start) but not .abort_pmsr. iwl_mld_ftm_start() stores the cfg80211 peer-measurement request in mld->ftm_initiator.req and the owning wdev in mld->ftm_initiator.req_wdev, and keeps both until a TOF range response completes the measurement.

When the user space that started the FTM session closes its netlink socket (or the wdev goes down) while the measurement is still in flight, cfg80211_release_pmsr()/cfg80211_pmsr_wdev_down() schedule the abort work and cfg80211_pmsr_process_abort() runs:

	rdev_abort_pmsr(rdev, wdev, req);
	kfree(req);

rdev_abort_pmsr() only calls the driver op when ops->abort_pmsr is set, so for MLD it is a no-op. The request is freed while mld->ftm_initiator.req still points at it; nothing clears the pointer.

A subsequent TOF range response from the firmware reaches iwl_mld_handle_ftm_resp_notif(). The !mld->ftm_initiator.req guard does not catch this because the pointer is dangling, not NULL, so the handler dereferences the freed request (req->cookie, req->n_peers, req->peers[]) and passes it with req_wdev to cfg80211_pmsr_report()/cfg80211_pmsr_complete(); a use-after-free of the already freed request.

Implement .abort_pmsr. iwl_mld_ftm_abort() checks that the aborted request is the active one, cancels the pending FTM notifications, resets the ftm_initiator state (clearing req and req_wdev), and sends TOF_RANGE_ABORT_CMD to the firmware so no further range responses are generated for the aborted request.

---
 .../intel/iwlwifi/mld/ftm-initiator.c         | 22 +++++++++++++++++++
 .../intel/iwlwifi/mld/ftm-initiator.h         |  2 ++
 .../net/wireless/intel/iwlwifi/mld/mac80211.c | 10 +++++++++
 3 files changed, 34 insertions(+)

index 81df3fdfcbf56..b18e4fa9dedf1 100644
@@ -13,6 +13,7 @@
 #include "constants.h"
 #include "fw/api/location.h"
 #include "ftm-initiator.h"
+#include "hcmd.h"
 
 static void iwl_mld_ftm_cmd_common(struct iwl_mld *mld,
 				   struct ieee80211_vif *vif,

@@ -282,6 +283,27 @@ static void iwl_mld_ftm_reset(struct iwl_mld *mld)
 	       sizeof(mld->ftm_initiator.responses));
 }
 
+void iwl_mld_ftm_abort(struct iwl_mld *mld,
+		       struct cfg80211_pmsr_request *req)
+{
+	struct iwl_tof_range_abort_cmd cmd = {
+		.request_id = req->cookie,
+	};
+
+	lockdep_assert_wiphy(mld->wiphy);
+
+	if (req != mld->ftm_initiator.req)
+		return;
+
+	iwl_mld_cancel_notifications_of_object(mld, IWL_MLD_OBJECT_TYPE_FTM_REQ,
+					       (u8)req->cookie);
+	iwl_mld_ftm_reset(mld);
+
+	if (iwl_mld_send_cmd_pdu(mld, WIDE_ID(LOCATION_GROUP, TOF_RANGE_ABORT_CMD),
+				 &cmd))
+		IWL_ERR(mld, "failed to abort FTM process\n");
+}
+
 static int iwl_mld_ftm_range_resp_valid(struct iwl_mld *mld, u8 request_id,
 					u8 num_of_aps)
 {

index 3fab25a52508a..7b807605af503 100644
@@ -21,6 +21,8 @@ struct ftm_initiator_data {
 
 int iwl_mld_ftm_start(struct iwl_mld *mld, struct ieee80211_vif *vif,
 		      struct cfg80211_pmsr_request *req);
+void iwl_mld_ftm_abort(struct iwl_mld *mld,
+		       struct cfg80211_pmsr_request *req);
 
 void iwl_mld_handle_ftm_resp_notif(struct iwl_mld *mld,
 				   struct iwl_rx_packet *pkt);

index 17286b3341c02..614c55967fcca 100644
@@ -2853,6 +2853,15 @@ static int iwl_mld_start_pmsr(struct ieee80211_hw *hw,
 	return iwl_mld_ftm_start(mld, vif, request);
 }
 
+static void iwl_mld_abort_pmsr(struct ieee80211_hw *hw,
+			       struct ieee80211_vif *vif,
+			       struct cfg80211_pmsr_request *request)
+{
+	struct iwl_mld *mld = IWL_MAC80211_GET_MLD(hw);
+
+	iwl_mld_ftm_abort(mld, request);
+}
+
 static enum ieee80211_neg_ttlm_res
 iwl_mld_can_neg_ttlm(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 		     struct ieee80211_neg_ttlm *neg_ttlm)

@@ -2974,6 +2983,7 @@ const struct ieee80211_ops iwl_mld_hw_ops = {
 	.prep_add_interface = iwl_mld_prep_add_interface,
 	.set_hw_timestamp = iwl_mld_set_hw_timestamp,
 	.start_pmsr = iwl_mld_start_pmsr,
+	.abort_pmsr = iwl_mld_abort_pmsr,
 	.can_neg_ttlm = iwl_mld_can_neg_ttlm,
 	.start_nan = iwl_mld_start_nan,
 	.stop_nan = iwl_mld_stop_nan,

2.50.1 (Apple Git-155)

pmsr_parse_ftm() rejects a request that omits NOMINAL_TIME only for non-trigger-based PD ranging. It then reads the attribute unconditionally for every non-trigger-based request:

	out->ftm.nominal_time =
		nla_get_u32(tb[NL80211_PMSR_FTM_REQ_ATTR_NOMINAL_TIME]);

For the other non-trigger-based request types NOMINAL_TIME is optional, so tb[...] can be NULL and nla_get_u32() dereferences a NULL pointer.

Keep the requirement for PD ranging and read the nominal-time value only when the attribute is present.

---
 net/wireless/pmsr.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 34c3625f7fd5e..d1e2fae5bc0e5 100644
@@ -263,8 +263,9 @@ static int pmsr_parse_ftm(struct cfg80211_registered_device *rdev,
 				       "FTM: nominal time is required for PD NTB ranging");
 			return -EINVAL;
 		}
-		out->ftm.nominal_time =
-			nla_get_u32(tb[NL80211_PMSR_FTM_REQ_ATTR_NOMINAL_TIME]);
+		if (tb[NL80211_PMSR_FTM_REQ_ATTR_NOMINAL_TIME])
+			out->ftm.nominal_time =
+				nla_get_u32(tb[NL80211_PMSR_FTM_REQ_ATTR_NOMINAL_TIME]);
 
 		if (tb[NL80211_PMSR_FTM_REQ_ATTR_MIN_TIME_BETWEEN_MEASUREMENTS])
 			out->ftm.min_time_between_measurements =

2.50.1 (Apple Git-155)

ieee80211_set_beacon_cntdwn() writes the current countdown value into each configured counter offset. For the beacon it skips offset 0 as an unused entry and bounds each non-zero offset against the template length before writing. The AP probe-response branch reuses the same counter-offset array but indexes resp->data[] with neither the zero-sentinel skip nor the length check.

A probe-response template whose countdown offset is 0 or points past resp->len therefore takes a stray or out-of-bounds write. Apply the same zero-sentinel and length checks the beacon path uses before writing the probe-response countdown byte.

---
 net/mac80211/tx.c | 4 ++++
 1 file changed, 4 insertions(+)

index 91b14112e24f0..84b6eda46a8f0 100644
@@ -5249,6 +5249,10 @@ static void ieee80211_set_beacon_cntdwn(struct ieee80211_sub_if_data *sdata,
 		if (sdata->vif.type == NL80211_IFTYPE_AP && resp) {
 			u16 *resp_offsets = resp->cntdwn_counter_offsets;
 
+			if (!resp_offsets[i])
+				continue;
+			if (WARN_ON_ONCE(resp_offsets[i] >= resp->len))
+				return;
 			resp->data[resp_offsets[i]] = count;
 		}
 	}

2.50.1 (Apple Git-155)

A received S1G TWT setup frame is length-checked in ieee80211_process_rx_twt_action() before it is queued: it requires

skb->len >= IEEE80211_MIN_ACTION_SIZE + sizeof(twt_setup) + 2

and then skb->len >= IEEE80211_MIN_ACTION_SIZE + 3 + twt->length, where twt->length is attacker-controlled. twt->length can be as small as 3 (the control byte plus the 2-byte req_type) and the frame still passes, so only those bytes are guaranteed present.

For an individual (non-broadcast) agreement, ieee80211_s1g_rx_twt_setup() calls drv_add_twt_setup(), and both trace_drv_add_twt_setup() and the driver ->add_twt_setup() callback read the full struct ieee80211_twt_params via twt->params (req_type, twt, min_twt_dur, mantissa, channel). That needs twt->length >= sizeof(twt->control) + sizeof(struct ieee80211_twt_params) = 15, so with the minimal 3-byte element they read up to 12 bytes past the end of the frame.

The broadcast path only rejects the agreement and touches req_type, which is always present, so it is unaffected. For the individual path, require twt->length to cover the control byte plus a full ieee80211_twt_params block before calling drv_add_twt_setup(), and drop the frame otherwise.

---
 net/mac80211/s1g.c | 4 ++++
 1 file changed, 4 insertions(+)

index 5af4a0c6c6424..abc338e22e59c 100644
@@ -101,6 +101,10 @@ ieee80211_s1g_rx_twt_setup(struct ieee80211_sub_if_data *sdata,
 	struct ieee80211_twt_setup *twt = (void *)mgmt->u.action.s1g.variable;
 	struct ieee80211_twt_params *twt_agrt = (void *)twt->params;
 
+	if (!(twt->control & IEEE80211_TWT_CONTROL_NEG_TYPE_BROADCAST) &&
+	    twt->length < sizeof(twt->control) + sizeof(*twt_agrt))
+		return;
+
 	twt_agrt->req_type &= cpu_to_le16(~IEEE80211_TWT_REQTYPE_REQUEST);
 
 	/* broadcast TWT not supported yet */

2.50.1 (Apple Git-155)

mwifiex_process_mgmt_packet() accepts an rx_pkt_length as small as a 4-address struct ieee80211_hdr plus the 2-byte firmware length prefix. After the prefix is stripped, mwifiex_parse_mgmt_packet() can be called with len equal to sizeof(struct ieee80211_hdr).

For an action frame the parser then reads the category byte just past that header, and for a public action frame the action code at the next byte, without checking that len covers them. A minimal-length action frame therefore causes a one- or two-byte read past the end of the RX buffer.

Reject frames shorter than the header, and require the category and (for public action frames) action-code bytes to be present before reading them.

---
 drivers/net/wireless/marvell/mwifiex/util.c | 9 +++++++++
 1 file changed, 9 insertions(+)

index 7d3631d212236..e2d76c2d9b05b 100644
@@ -313,13 +313,22 @@ mwifiex_parse_mgmt_packet(struct mwifiex_private *priv, u8 *payload, u16 len,
 	u8 category, action_code, *addr2;
 	struct ieee80211_hdr *ieee_hdr = (void *)payload;
 
+	if (len < sizeof(*ieee_hdr))
+		return -1;
+
 	stype = (le16_to_cpu(ieee_hdr->frame_control) & IEEE80211_FCTL_STYPE);
 
 	switch (stype) {
 	case IEEE80211_STYPE_ACTION:
+		if (len < sizeof(*ieee_hdr) + 1)
+			return -1;
+
 		category = *(payload + sizeof(struct ieee80211_hdr));
 		switch (category) {
 		case WLAN_CATEGORY_PUBLIC:
+			if (len < sizeof(*ieee_hdr) + 2)
+				return -1;
+
 			action_code = *(payload + sizeof(struct ieee80211_hdr)
 					+ 1);
 			if (action_code == WLAN_PUB_ACTION_TDLS_DISCOVER_RES) {

2.50.1 (Apple Git-155)

nl80211_color_change() calls nl80211_parse_beacon() for the beacon_next template, which can allocate params.beacon_next.mbssid_ies and .rnr_ies. The out: label frees those, but two error paths after that parse returned directly instead of jumping to it: nl80211_parse_beacon() itself failing, and the kzalloc_objs() of the attribute table failing. Both leaked whatever mbssid_ies/rnr_ies had been allocated in beacon_next.

Initialize tb to NULL and route both error paths through the out: label so the parsed beacon data is freed.

---
 net/wireless/nl80211.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

index 5adcb6bd0fc56..7e796d7c1a661 100644
@@ -18724,7 +18724,7 @@ static int nl80211_color_change(struct sk_buff *skb, struct genl_info *info)
 	struct cfg80211_color_change_settings params = {};
 	struct net_device *dev = info->user_ptr[1];
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
-	struct nlattr **tb;
+	struct nlattr **tb = NULL;
 	u16 offset;
 	int err;
 

@@ -18754,11 +18754,13 @@ static int nl80211_color_change(struct sk_buff *skb, struct genl_info *info)
 				   wdev->links[params.link_id].ap.chandef.chan,
 				   info->extack);
 	if (err)
-		return err;
+		goto out;
 
 	tb = kzalloc_objs(*tb, NL80211_ATTR_MAX + 1);
-	if (!tb)
-		return -ENOMEM;
+	if (!tb) {
+		err = -ENOMEM;
+		goto out;
+	}
 
 	err = nla_parse_nested(tb, NL80211_ATTR_MAX,
 			       info->attrs[NL80211_ATTR_COLOR_CHANGE_ELEMS],

2.50.1 (Apple Git-155)

nl80211_color_change() calls nl80211_parse_beacon() for the beacon_next template, which can allocate params.beacon_next.mbssid_ies and .rnr_ies. A parsing failure returned directly instead of using the out: cleanup, leaking any allocations completed before the error.

Allocate the nested attribute table before parsing beacon_next. Its allocation failure can then return before beacon data exists, while a later parsing failure uses out: to release the parsed data.

---
Changes in v2:
- Allocate the attribute table before parsing beacon_next, as suggested by Johannes.

 net/wireless/nl80211.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index 5adcb6bd0fc5..755f8fe711fb 100644
@@ -18750,15 +18750,15 @@ static int nl80211_color_change(struct sk_buff *skb, struct genl_info *info)
 	if (!wdev->links[params.link_id].ap.beacon_interval)
 		return -EINVAL;
 
+	tb = kzalloc_objs(*tb, NL80211_ATTR_MAX + 1);
+	if (!tb)
+		return -ENOMEM;
+
 	err = nl80211_parse_beacon(rdev, info->attrs, &params.beacon_next,
 				   wdev->links[params.link_id].ap.chandef.chan,
 				   info->extack);
 	if (err)
-		return err;
-
-	tb = kzalloc_objs(*tb, NL80211_ATTR_MAX + 1);
-	if (!tb)
-		return -ENOMEM;
+		goto out;
 
 	err = nla_parse_nested(tb, NL80211_ATTR_MAX,
 			       info->attrs[NL80211_ATTR_COLOR_CHANGE_ELEMS],

base-commit: 57aa1718d5953dd532137d43b696c68545c2e0b3

2.50.1 (Apple Git-155)

This is honestly a bit messy as a series, covering all kinds of drivers and CC'ing everyone. It'd be a lot simpler for everyone if you just sent them all individually, or at least per subsystem, putting only the ones that depend on each other together.

I'm going to look at these only for wireless-next later, I don't think hwsim is all that relevant in deployed systems.

I don't know about this, Miri should look (I think she said it was handled already or so in patchwork).

That one vaguely seems like I should apply it now.

I think this one is falling into a typical LLM pattern patching trap, it describes [0] as being out-of-bounds for some allocated data, and neglects that the offsets have already been checked by cfg80211. Please describe more why it's actually an issue beyond "there's a similar pattern elsewhere".

I don't know about this, you should probably resend it. Anyway I'm not loosing sleep over it since there's no driver with the callback, tracing is mostly not enabled, and while a potential 12 bytes OOB read at the end of the SKB is a bug, it's not a very important one since there should be stuff there anyway.

Francesco should look at that - again, sending it all together makes this really hard - but it seems to me it should use MIN_ACTION_SIZE()?

In this one it seems it'd be much simpler to allocate the memory first, i.e. change the order of operations rather than complicate the cleanup?

johannes

Also in addition - please resend these, the s1g should go also to Lachlan I think who wrote it, and I don't think anyone is paying attention to this patchset :)

johannes

The twt stuff has been in for a while and was added by Lorenzo [1]. I'm not sure why it sits in the s1g.c file, I assume because it was first added to the standard for S1G then expanded to other standards? Might be worth moving all these to a separate twt.c file (I can if you think it's worth it).

[1] https://lore.kernel.org/all/257512f2e22ba42b9f2624942a128dd8f141de4b.1629741512.git.lorenzo@kernel.org/

lachlan

Oops, my bad, sorry!

Right, it's the frame format there and all. I guess if I can remember in the future that not all of the s1g* code is from you, it'll be OK, but if you don't want to take the risk we can move it ;-)

johannes

Hi Johannes,

Thanks for the reminder. They should never be a patchset. Patch 3/8 is with the Intel maintainers, and 4/8 is applied. I'll send the remaining patches individually after rebasing each one.

- 5/8 (probe-response countdown): cfg80211 already bounds a nonzero offset so original out-of-bounds explanation was totally wrong. But zero-initialized probe-response offsets are treated as valid, which corrupts resp->data[0]. The v2 skips zero offsets instead.

- 6/8 (S1G TWT): added a guard before the driver callback, with Lachlan and Lorenzo copied, sent to wireless-next.

- 7/8 (mwifiex action frame): will use IEEE80211_MIN_ACTION_SIZE() and go to the mwifiex maintainers.

Regards,
Zhao