diff mbox series

[v2] wifi: mac80211: validate link status before deciding on mgmt tx

Message ID 20230119221306.24526-1-quic_srirrama@quicinc.com (mailing list archive)
State Changes Requested
Delegated to: Johannes Berg
Headers show
Series [v2] wifi: mac80211: validate link status before deciding on mgmt tx | expand

Commit Message

Sriram R Jan. 19, 2023, 10:13 p.m. UTC
Currently we check the status of bss active flag to see if the
AP is active. But in case of a MLD AP, when some of the links
are getting teardown and some are active, mgmt Tx(like deauth)
can be sent on some links before they are brought down as well.

In such cases, the bss active flag might not provide the exact
status of the MLD links, which becomes false on first link deleted.
Hence check if any of the links can be used for mgmt tx
before returning error status.

Also, use the link id passed from userspace when the link bss
address matches the mgmt SA and the chan params match the request.
This will avoid scenario where the link id from userspace
gets reset.

Signed-off-by: Sriram R <quic_srirrama@quicinc.com>
---
v2: added wifi prefix in commit title
 net/mac80211/offchannel.c | 50 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

Comments

Johannes Berg Feb. 14, 2023, 12:45 p.m. UTC | #1
On Fri, 2023-01-20 at 03:43 +0530, Sriram R wrote:
> Currently we check the status of bss active flag to see if the
> AP is active.
> 

Following so far :)

> But in case of a MLD AP, when some of the links
> are getting teardown 

"getting torn down"?

> and some are active, mgmt Tx(like deauth)
> can be sent on some links before they are brought down as well.

Makes sense.

> In such cases, the bss active flag might not provide the exact
> status of the MLD links, which becomes false on first link deleted.

Wait, isn't that already a bug?

> Hence check if any of the links can be used for mgmt tx
> before returning error status.
> 
> Also, use the link id passed from userspace when the link bss
> address matches the mgmt SA and the chan params match the request.
> This will avoid scenario where the link id from userspace
> gets reset.

"gets reset"??

>  
> +static bool ieee80211_is_link_bss_active(struct ieee80211_sub_if_data *sdata,
> +					 int link_id)
> +{
[...]
> +	sdata_lock(sdata);
> +	link = sdata_dereference(sdata->link[link_id], sdata);
> +	if (!link) {
> +		sdata_unlock(sdata);
> +		return false;
> +	}
> +
> +	if (sdata_dereference(link->u.ap.beacon, sdata)) {
> +		sdata_unlock(sdata);
> +		return true;
> +	}
> +
> +	sdata_unlock(sdata);

The locking here is ... decidedly odd. It feels like with all the
wdev_lock()ing going on in cfg80211_mlme_mgmt_tx() we should probably
just lock around the *entire* thing in cfg80211, including the
driver/mac80211 call?

> @@ -883,8 +920,17 @@ int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
>  				break;
>  			}
>  
> -			if (ether_addr_equal(conf->addr, mgmt->sa))
> +			if (ether_addr_equal(conf->addr, mgmt->sa)) {
> +				/* If userspace requested Tx on a specific link
> +				 * use the same link id if the link bss is matching
> +				 * the requested chan.
> +				 */
> +				if (sdata->vif.valid_links &&
> +				    params->link_id >= 0 && params->link_id == i &&
> +				    params->chan == chanctx_conf->def.chan)
> +					link_id = i;
>  				break;
> +			}


Not sure I get it, if it's bad (link ID doesn't match BSS) then
shouldn't we just reject it?

johannes
Sriram R Feb. 14, 2023, 4:24 p.m. UTC | #2
>-----Original Message-----
>From: Johannes Berg <johannes@sipsolutions.net>
>Sent: Tuesday, February 14, 2023 6:16 PM
>To: Sriram R (QUIC) <quic_srirrama@quicinc.com>
>Cc: linux-wireless@vger.kernel.org
>Subject: Re: [PATCH v2] wifi: mac80211: validate link status before deciding on
>mgmt tx
>
>On Fri, 2023-01-20 at 03:43 +0530, Sriram R wrote:
>> Currently we check the status of bss active flag to see if the AP is
>> active.
>>
>
>Following so far :)
>
>> But in case of a MLD AP, when some of the links are getting teardown
>
>"getting torn down"?
Right, I'll update in the next revision.
>
>> and some are active, mgmt Tx(like deauth) can be sent on some links
>> before they are brought down as well.
>
>Makes sense.
>
>> In such cases, the bss active flag might not provide the exact status
>> of the MLD links, which becomes false on first link deleted.
>
>Wait, isn't that already a bug?
This commit ("commit bfd8403adddd09f32033a14bf25be398291e7881") introduced this flag
for whole AP(MLD) and replaced  beacon since it is link specific. Hence I thought It was
brought in to represent a MLD whereas the beacon ptr can be checked if a certain link is active.
Hence used both bss->active and beacon checks to see if atleast one link is active.
>
>> Hence check if any of the links can be used for mgmt tx before
>> returning error status.
>>
>> Also, use the link id passed from userspace when the link bss address
>> matches the mgmt SA and the chan params match the request.
>> This will avoid scenario where the link id from userspace gets reset.
>
>"gets reset"??
In ieee80211_mgmt_tx() the link id which was passed from userspace is
Ignored if its not an action frame. Hence the below change was done to check if one of the
Link bss  matches with the link id passed
Also, in __ieee80211_tx_skb_tid_band() the below elseif condition might not be
appropriate in case the MLD address and one of the link address is same.
        } else if (memcmp(sdata->vif.addr, hdr->addr2, ETH_ALEN) == 0) {
                /* address from the MLD */
                link = IEEE80211_LINK_UNSPECIFIED;
        } else {

Hence wanted to fix in the ieee80211_mgmt_tx() itself to use the proper link id
passed from userspace and avoid getting reset to -1.
>
>>
>> +static bool ieee80211_is_link_bss_active(struct ieee80211_sub_if_data
>*sdata,
>> +					 int link_id)
>> +{
>[...]
>> +	sdata_lock(sdata);
>> +	link = sdata_dereference(sdata->link[link_id], sdata);
>> +	if (!link) {
>> +		sdata_unlock(sdata);
>> +		return false;
>> +	}
>> +
>> +	if (sdata_dereference(link->u.ap.beacon, sdata)) {
>> +		sdata_unlock(sdata);
>> +		return true;
>> +	}
>> +
>> +	sdata_unlock(sdata);
>
>The locking here is ... decidedly odd. It feels like with all the wdev_lock()ing
>going on in cfg80211_mlme_mgmt_tx() we should probably just lock around
>the *entire* thing in cfg80211, including the
>driver/mac80211 call?
Sure, let me revisit this in the next version.
>
>> @@ -883,8 +920,17 @@ int ieee80211_mgmt_tx(struct wiphy *wiphy, struct
>wireless_dev *wdev,
>>  				break;
>>  			}
>>
>> -			if (ether_addr_equal(conf->addr, mgmt->sa))
>> +			if (ether_addr_equal(conf->addr, mgmt->sa)) {
>> +				/* If userspace requested Tx on a specific link
>> +				 * use the same link id if the link bss is
>matching
>> +				 * the requested chan.
>> +				 */
>> +				if (sdata->vif.valid_links &&
>> +				    params->link_id >= 0 && params->link_id ==
>i &&
>> +				    params->chan == chanctx_conf->def.chan)
>> +					link_id = i;
>>  				break;
>> +			}
>
>
>Not sure I get it, if it's bad (link ID doesn't match BSS) then shouldn't we just
>reject it?
As mentioned above the link id gets set to -1 since the switch case for NL80211_IFTYPE_AP
sets the link id from params->link_id only when it’s an action frame.
We might have to honor the link id passed in params->link_id for any management frame
, right?, so that the address translation is done properly in driver for all mgmt. frames
Please let me know if something is misunderstood here.

Thanks,
Sriram.R
diff mbox series

Patch

diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c
index d78c82d6b696..5e312860a976 100644
--- a/net/mac80211/offchannel.c
+++ b/net/mac80211/offchannel.c
@@ -763,6 +763,43 @@  int ieee80211_cancel_remain_on_channel(struct wiphy *wiphy,
 	return ieee80211_cancel_roc(local, cookie, false);
 }
 
+static bool ieee80211_is_link_bss_active(struct ieee80211_sub_if_data *sdata,
+					 int link_id)
+{
+	struct ieee80211_link_data *link;
+
+	if (!sdata->bss)
+		return false;
+
+	/* This is consolidated status of the MLD or non ML bss */
+	if (sdata->bss->active)
+		return true;
+
+	if (link_id < 0)
+		return false;
+
+	if (!sdata->vif.valid_links)
+		return false;
+
+	if (!(sdata->vif.valid_links & BIT(link_id)))
+		return false;
+
+	sdata_lock(sdata);
+	link = sdata_dereference(sdata->link[link_id], sdata);
+	if (!link) {
+		sdata_unlock(sdata);
+		return false;
+	}
+
+	if (sdata_dereference(link->u.ap.beacon, sdata)) {
+		sdata_unlock(sdata);
+		return true;
+	}
+
+	sdata_unlock(sdata);
+	return false;
+}
+
 int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
 		      struct cfg80211_mgmt_tx_params *params, u64 *cookie)
 {
@@ -804,7 +841,7 @@  int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
 	case NL80211_IFTYPE_P2P_GO:
 		if (sdata->vif.type != NL80211_IFTYPE_ADHOC &&
 		    !ieee80211_vif_is_mesh(&sdata->vif) &&
-		    !sdata->bss->active)
+		    !ieee80211_is_link_bss_active(sdata, params->link_id))
 			need_offchan = true;
 
 		rcu_read_lock();
@@ -883,8 +920,17 @@  int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
 				break;
 			}
 
-			if (ether_addr_equal(conf->addr, mgmt->sa))
+			if (ether_addr_equal(conf->addr, mgmt->sa)) {
+				/* If userspace requested Tx on a specific link
+				 * use the same link id if the link bss is matching
+				 * the requested chan.
+				 */
+				if (sdata->vif.valid_links &&
+				    params->link_id >= 0 && params->link_id == i &&
+				    params->chan == chanctx_conf->def.chan)
+					link_id = i;
 				break;
+			}
 
 			chanctx_conf = NULL;
 		}