[PATCH] wifi: cfg80211: publish PMSR request before starting the driver

[PATCH] wifi: cfg80211: publish PMSR request before starting the driver

[Zhao Li]

nl80211_pmsr_start() assigns the request cookie, calls the driver's
->start_pmsr() callback, and only then adds the request to
wdev->pmsr_list, without holding pmsr_lock for the addition.

mac80211_hwsim saves the request in its start callback and returns. Since
nl80211 uses parallel_ops, an immediate REPORT_PMSR can then run before
nl80211_pmsr_start() reaches its post-start list_add_tail(). hwsim also
dispatches reports from its virtio receive workqueue. Completion removes
the request from wdev->pmsr_list under pmsr_lock and frees it.

Thus completion can precede publication, race the unlocked list mutation,
or free the request before nl80211_pmsr_start() reads req->cookie for the
netlink reply.

Add the request to wdev->pmsr_list under pmsr_lock before calling the
driver, and use a cookie value saved before the call so the request is not
dereferenced after a successful start. On an error return the driver has
not retained or completed the request, so remove it from the list under the
lock and free it.

This ordering also permits a successful driver callback to complete the
request synchronously. Document the resulting start_pmsr lifetime contract.

Fixes: 9bb7e0f24e7e ("cfg80211: add peer measurement with FTM initiator API")
Assisted-by: Codex:gpt-5
Assisted-by: Claude:opus-4.8
Signed-off-by: Zhao Li <enderaoelyther@gmail.com>
---
 include/net/cfg80211.h |  6 +++++-
 include/net/mac80211.h |  6 +++++-
 net/wireless/pmsr.c    | 21 +++++++++++++++++----
 3 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index f5abf1db7558..a8ba484ecad6 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -5202,7 +5202,11 @@ struct mgmt_frame_regs {
  *
  * @get_ftm_responder_stats: Retrieve FTM responder statistics, if available.
  *	Statistics should be cumulative, currently no way to reset is provided.
- * @start_pmsr: start peer measurement (e.g. FTM)
+ * @start_pmsr: start peer measurement (e.g. FTM). The callback may
+ *	complete the request before returning success. After completing it,
+ *	the driver must not access the request. If the callback returns an
+ *	error, the driver must not retain the request or later report results
+ *	or completion for it.
  * @abort_pmsr: abort peer measurement
  *
  * @update_owe_info: Provide updated OWE info to driver. Driver implementing SME
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 4f95da023746..64600e7bd251 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -4661,7 +4661,11 @@ struct ieee80211_prep_tx_info {
  * @get_ftm_responder_stats: Retrieve FTM responder statistics, if available.
  *	Statistics should be cumulative, currently no way to reset is provided.
  *
- * @start_pmsr: start peer measurement (e.g. FTM) (this call can sleep)
+ * @start_pmsr: start peer measurement (e.g. FTM) (this call can sleep).
+ *	The callback may complete the request before returning success.
+ *	After completing it, the driver must not access the request. If the
+ *	callback returns an error, the driver must not retain the request or
+ *	later report results or completion for it.
  * @abort_pmsr: abort peer measurement (this call can sleep)
  * @set_tid_config: Apply TID specific configurations. This callback may sleep.
  * @reset_tid_config: Reset TID specific configuration for the peer.
diff --git a/net/wireless/pmsr.c b/net/wireless/pmsr.c
index d1e2fae5bc0e..3484956ebb50 100644
--- a/net/wireless/pmsr.c
+++ b/net/wireless/pmsr.c
@@ -420,6 +420,7 @@ int nl80211_pmsr_start(struct sk_buff *skb, struct genl_info *info)
 	const struct cfg80211_pmsr_capabilities *capa;
 	struct cfg80211_pmsr_request *req;
 	struct nlattr *peers, *peer;
+	u64 cookie;
 
 	capa = rdev->wiphy.pmsr_capa;
 
@@ -521,14 +522,26 @@ int nl80211_pmsr_start(struct sk_buff *skb, struct genl_info *info)
 	}
 	req->cookie = cfg80211_assign_cookie(rdev);
 	req->nl_portid = info->snd_portid;
+	cookie = req->cookie;
+
+	/*
+	 * Publish before the driver can complete the request. Completion may free
+	 * it before rdev_start_pmsr() returns, so use the cookie snapshot below.
+	 */
+	spin_lock_bh(&wdev->pmsr_lock);
+	list_add_tail(&req->list, &wdev->pmsr_list);
+	spin_unlock_bh(&wdev->pmsr_lock);
 
 	err = rdev_start_pmsr(rdev, wdev, req);
-	if (err)
+	if (err) {
+		/* An error return leaves the request owned by this path. */
+		spin_lock_bh(&wdev->pmsr_lock);
+		list_del(&req->list);
+		spin_unlock_bh(&wdev->pmsr_lock);
 		goto out_err;
+	}
 
-	list_add_tail(&req->list, &wdev->pmsr_list);
-
-	nl_set_extack_cookie_u64(info->extack, req->cookie);
+	nl_set_extack_cookie_u64(info->extack, cookie);
 	return 0;
 out_err:
 	kfree(req);
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] wifi: cfg80211: publish PMSR request before starting the driver

[Johannes Berg]

On Thu, 2026-07-23 at 09:09 +0800, Zhao Li wrote:
> 
> This ordering also permits a successful driver callback to complete the
> request synchronously. Document the resulting start_pmsr lifetime contract.

From a locking perspective perhaps yes, but cfg80211_pmsr_complete()
sends the results to userspace immediately, so userspace might see a
completion with a cookie before it even got the cookie as the request
response, which (semantically) makes no sense.

Given that measurement requests are always going to take some time, I
don't think this is an issue in practice that we really need to work
hard to prevent (we'd have to do something like attaching the result to
the request on the list, pivot to a wiphy locked worker, etc.)

However, I don't think it makes sense to actually *document* that it's
now possible - it's only possible from the kernel's locking POV, from
userspace's POV it's still highly confusing at best, and it makes no
sense semantically either.

At best the documentation would be something like "the kernel doesn't
crash if the request is completed before returning success" but that's
not useful either :)

> +	/*
> +	 * Publish before the driver can complete the request. Completion may free
> +	 * it before rdev_start_pmsr() returns, so use the cookie snapshot below.
> +	 */

That comment then should call out how it's really about preventing races
with drivers from doing UAF (we could technically be preempted here
too), rather than making that sound like a reasonable order - just
saying "Under races and/or broken drivers immediate completion might
free it ..." or something along those lines would be better I think.

johannes

Re: [PATCH] wifi: cfg80211: publish PMSR request before starting the driver

[Zhao Li]

On Thu, 23 Jul 2026 at 10:18:33 +0200, Johannes Berg wrote:
> However, I don't think it makes sense to actually *document* that it's
> now possible - it's only possible from the kernel's locking POV, from
> userspace's POV it's still highly confusing at best, and it makes no
> sense semantically either.

Agreed. I'll drop the kernel-doc changes. Documenting it implies it's a
supported semantic when it's really just a locking side effect.

> That comment then should call out how it's really about preventing races
> with drivers from doing UAF (we could technically be preempted here too),
> rather than making that sound like a reasonable order - just saying "Under
> races and/or broken drivers immediate completion might free it ..." or
> something along those lines would be better I think.

Exactly. I'll reword the comment to frame it as defensive against races
and broken drivers, not as an intentional ordering.

Will send a v2.

Thanks,
Zhao

[PATCH v2] wifi: cfg80211: publish PMSR request before starting the driver

[Zhao Li]

nl80211_pmsr_start() assigns the request cookie, calls the driver's
->start_pmsr() callback, and only then adds the request to
wdev->pmsr_list, without holding pmsr_lock for the addition.

mac80211_hwsim saves the request in its start callback and returns. Since
nl80211 uses parallel_ops, an immediate REPORT_PMSR can then run before
nl80211_pmsr_start() reaches its post-start list_add_tail(). hwsim also
dispatches reports from its virtio receive workqueue. Completion removes
the request from wdev->pmsr_list under pmsr_lock and frees it.

Thus completion can precede publication, race the unlocked list mutation,
or free the request before nl80211_pmsr_start() reads req->cookie for the
netlink reply.

Add the request to wdev->pmsr_list under pmsr_lock before calling the
driver, and use a cookie value saved before the call so the request is not
dereferenced after a successful start. On an error return the driver has
not retained or completed the request, so remove it from the list under the
lock and free it.

Fixes: 9bb7e0f24e7e ("cfg80211: add peer measurement with FTM initiator API")
Link: https://lore.kernel.org/all/20260723010916.76433-1-enderaoelyther@gmail.com/
Assisted-by: Codex:gpt-5
Assisted-by: Claude:opus-4.8
Signed-off-by: Zhao Li <enderaoelyther@gmail.com>
---
Changes in v2:
- Drop callback documentation that implied synchronous completion was
  supported.
- Reword the local comment as defensive handling for races or broken
  drivers that complete the request before ->start_pmsr() returns.
---
 net/wireless/pmsr.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/net/wireless/pmsr.c b/net/wireless/pmsr.c
index d1e2fae5bc0e..97449bcb9a22 100644
--- a/net/wireless/pmsr.c
+++ b/net/wireless/pmsr.c
@@ -420,6 +420,7 @@ int nl80211_pmsr_start(struct sk_buff *skb, struct genl_info *info)
 	const struct cfg80211_pmsr_capabilities *capa;
 	struct cfg80211_pmsr_request *req;
 	struct nlattr *peers, *peer;
+	u64 cookie;
 
 	capa = rdev->wiphy.pmsr_capa;
 
@@ -521,14 +522,27 @@ int nl80211_pmsr_start(struct sk_buff *skb, struct genl_info *info)
 	}
 	req->cookie = cfg80211_assign_cookie(rdev);
 	req->nl_portid = info->snd_portid;
+	cookie = req->cookie;
+
+	/*
+	 * Add to the list before the driver call; under races or broken
+	 * drivers, completion may free the request before rdev_start_pmsr()
+	 * returns. Use the saved cookie below.
+	 */
+	spin_lock_bh(&wdev->pmsr_lock);
+	list_add_tail(&req->list, &wdev->pmsr_list);
+	spin_unlock_bh(&wdev->pmsr_lock);
 
 	err = rdev_start_pmsr(rdev, wdev, req);
-	if (err)
+	if (err) {
+		/* An error return leaves the request owned by this path. */
+		spin_lock_bh(&wdev->pmsr_lock);
+		list_del(&req->list);
+		spin_unlock_bh(&wdev->pmsr_lock);
 		goto out_err;
+	}
 
-	list_add_tail(&req->list, &wdev->pmsr_list);
-
-	nl_set_extack_cookie_u64(info->extack, req->cookie);
+	nl_set_extack_cookie_u64(info->extack, cookie);
 	return 0;
 out_err:
 	kfree(req);
-- 
2.50.1 (Apple Git-155)