diff mbox series

[wireless-next] wifi: mac80211: correctly parse S1G beacon optional elements

Message ID 20250602053521.410650-1-lachlan.hodges@morsemicro.com
State Superseded
Headers show
Series [wireless-next] wifi: mac80211: correctly parse S1G beacon optional elements | expand

Commit Message

Lachlan Hodges June 2, 2025, 5:35 a.m. UTC 
S1G beacons are not traditional beacons but a type of extension frame.
Extension frames contain the frame control and duration fields, followed
by zero or more optional fields before the frame body. These optional
fields are distinct from information elements (IEs).

The presence of optional fields is indicated in the frame control field.
To correctly locate the IE offset, the frame control must be parsed to
identify which optional fields are present. Currently, mac80211 parses
S1G beacons based on fixed assumptions about the frame layout, without
inspecting the frame control field. This can result in incorrect offsets
to the "variable" portion of the frame.

This patch adds proper support for parsing S1G beacon frames by using
the field lengths defined in IEEE 802.11-2024, section 9.3.4.3, ensuring
that the IE offset is calculated accurately.

Fixes: 9eaffe5078ca ("cfg80211: convert S1G beacon to scan results")
Signed-off-by: Lachlan Hodges <lachlan.hodges@morsemicro.com>
---
 include/linux/ieee80211.h | 79 ++++++++++++++++++++++++++++++++++-----
 net/mac80211/mlme.c       |  7 +---
 net/mac80211/scan.c       | 10 ++---
 net/wireless/scan.c       | 18 +++------
 4 files changed, 81 insertions(+), 33 deletions(-)

Comments

Johannes Berg June 2, 2025, 8:27 a.m. UTC | #1 
> 
> Fixes: 9eaffe5078ca ("cfg80211: convert S1G beacon to scan results")

Should it really be wireless-next rather than wireless with this? We're
still even in the merge window, so seems entirely appropriate to put it
into wireless instead, even if the bug is old, rather than wait months
for it to release?

It also seems you should split this to cfg80211 and mac80211, if
possible, and have two different Fixes: lines? The cfg80211 part this
has might fix the above, but for the mac80211 part that doesn't seem
plausible?

> +/**
> + * ieee80211_s1g_has_next_tbtt - check if IEEE80211_S1G_BCN_NEXT_TBTT
> + * @fc: frame control bytes in little-endian byteorder
> + * Return: whether or not the frame contains the variable-length
> + *  next TBTT field

nit: the indentation should be a tab there (and in the later instances
in the other functions)

> +/**
> + * ieee80211_s1g_optional_len - determine length of optional S1G beacon fields
> + * @fc: frame control bytes in little-endian byteorder
> + * Return: total length in bytes of the optional fixed-length fields
> + *
> + * S1G beacons may contain up to three optional fixed-length fields that
> + * precede the variable-length information elements (IEs). Whether these

Also kind of nit, but the current spec versions (for a long time) have
stopped calling it "information elements" and say just "elements"
instead, I've been trying to at least do that in new code in Linux.

>  	if (ieee80211_is_s1g_beacon(mgmt->frame_control)) {
>  		struct ieee80211_ext *ext = (void *) mgmt;
> -
> -		if (ieee80211_is_s1g_short_beacon(ext->frame_control))
> -			variable = ext->u.s1g_short_beacon.variable;
> -		else
> -			variable = ext->u.s1g_beacon.variable;
> +		variable = ext->u.s1g_beacon.variable
> +					+ ieee80211_s1g_optional_len(ext->frame_control);

Please do this like in other code:

	variable = ... +
		   ...;


(also more instances below)

>  	if (ieee80211_is_s1g_beacon(mgmt->frame_control)) {
> -		ext = (void *) mgmt;
> -		if (ieee80211_is_s1g_short_beacon(mgmt->frame_control))
> -			min_hdr_len = offsetof(struct ieee80211_ext,
> -					       u.s1g_short_beacon.variable);
> -		else
> -			min_hdr_len = offsetof(struct ieee80211_ext,
> -					       u.s1g_beacon.variable);
> +		ext = (struct ieee80211_ext *)mgmt;

Keeping the (void *) cast seems fine? I guess you can remove the space
if you like :)

Overall though I like the fact that we don't try to distinguish the
whole short/normal beacon thing, that was always a bit of a mess, and if
it fixes more bugs while at it ... nice :)

johannes
Lachlan Hodges June 3, 2025, 5:26 a.m. UTC | #2 
On Mon, Jun 02, 2025 at 10:27:53AM +0200, Johannes Berg wrote:
> >
> > Fixes: 9eaffe5078ca ("cfg80211: convert S1G beacon to scan results")
>
> Should it really be wireless-next rather than wireless with this? We're
> still even in the merge window, so seems entirely appropriate to put it
> into wireless instead, even if the bug is old, rather than wait months
> for it to release?
>
> It also seems you should split this to cfg80211 and mac80211, if
> possible, and have two different Fixes: lines? The cfg80211 part this
> has might fix the above, but for the mac80211 part that doesn't seem
> plausible?
>

Thanks for the quick reply! We don't think it's possible to split the
patches without breaking the build between the two so have opted to
keep it as a single patch. In addition I've included the two Fixes:
lines that reference the original changes.

lachlan
diff mbox series

Patch

diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index 420c7f9aa6ee..6c518989250d 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
@@ -111,6 +111,8 @@ 
 
 /* bits unique to S1G beacon */
 #define IEEE80211_S1G_BCN_NEXT_TBTT	0x100
+#define IEEE80211_S1G_BCN_CSSID     0x200
+#define IEEE80211_S1G_BCN_ANO       0x400
 
 /* see 802.11ah-2016 9.9 NDP CMAC frames */
 #define IEEE80211_S1G_1MHZ_NDP_BITS	25
@@ -153,9 +155,6 @@ 
 
 #define IEEE80211_ANO_NETTYPE_WILD              15
 
-/* bits unique to S1G beacon */
-#define IEEE80211_S1G_BCN_NEXT_TBTT    0x100
-
 /* control extension - for IEEE80211_FTYPE_CTL | IEEE80211_STYPE_CTL_EXT */
 #define IEEE80211_CTL_EXT_POLL		0x2000
 #define IEEE80211_CTL_EXT_SPR		0x3000
@@ -627,6 +626,42 @@  static inline bool ieee80211_is_s1g_beacon(__le16 fc)
 	       cpu_to_le16(IEEE80211_FTYPE_EXT | IEEE80211_STYPE_S1G_BEACON);
 }
 
+/**
+ * ieee80211_s1g_has_next_tbtt - check if IEEE80211_S1G_BCN_NEXT_TBTT
+ * @fc: frame control bytes in little-endian byteorder
+ * Return: whether or not the frame contains the variable-length
+ *  next TBTT field
+ */
+static inline bool ieee80211_s1g_has_next_tbtt(__le16 fc)
+{
+	return ieee80211_is_s1g_beacon(fc) &&
+		(fc & cpu_to_le16(IEEE80211_S1G_BCN_NEXT_TBTT));
+}
+
+/**
+ * ieee80211_s1g_has_ano - check if IEEE80211_S1G_BCN_ANO
+ * @fc: frame control bytes in little-endian byteorder
+ * Return: whether or not the frame contains the variable-length
+ *  ANO field
+ */
+static inline bool ieee80211_s1g_has_ano(__le16 fc)
+{
+	return ieee80211_is_s1g_beacon(fc) &&
+		(fc & cpu_to_le16(IEEE80211_S1G_BCN_ANO));
+}
+
+/**
+ * ieee80211_s1g_has_cssid - check if IEEE80211_S1G_BCN_CSSID
+ * @fc: frame control bytes in little-endian byteorder
+ * Return: whether or not the frame contains the variable-length
+ *  compressed SSID field
+ */
+static inline bool ieee80211_s1g_has_cssid(__le16 fc)
+{
+	return ieee80211_is_s1g_beacon(fc) &&
+		(fc & cpu_to_le16(IEEE80211_S1G_BCN_CSSID));
+}
+
 /**
  * ieee80211_is_s1g_short_beacon - check if frame is an S1G short beacon
  * @fc: frame control bytes in little-endian byteorder
@@ -1245,16 +1280,40 @@  struct ieee80211_ext {
 			u8 change_seq;
 			u8 variable[0];
 		} __packed s1g_beacon;
-		struct {
-			u8 sa[ETH_ALEN];
-			__le32 timestamp;
-			u8 change_seq;
-			u8 next_tbtt[3];
-			u8 variable[0];
-		} __packed s1g_short_beacon;
 	} u;
 } __packed __aligned(2);
 
+/**
+ * ieee80211_s1g_optional_len - determine length of optional S1G beacon fields
+ * @fc: frame control bytes in little-endian byteorder
+ * Return: total length in bytes of the optional fixed-length fields
+ *
+ * S1G beacons may contain up to three optional fixed-length fields that
+ * precede the variable-length information elements (IEs). Whether these
+ * fields are present is indicated by flags in the frame control field.
+ *
+ * From IEEE 802.11-2024 section 9.3.4.3:
+ *  - Next TBTT field may be 0 or 3 bytes
+ *  - Short SSID field may be 0 or 4 bytes
+ *  - Access Network Options (ANO) field may be 0 or 1 byte
+ */
+static inline size_t
+ieee80211_s1g_optional_len(__le16 fc)
+{
+	size_t len = 0;
+
+	if (ieee80211_s1g_has_next_tbtt(fc))
+		len += 3;
+
+	if (ieee80211_s1g_has_cssid(fc))
+		len += 4;
+
+	if (ieee80211_s1g_has_ano(fc))
+		len += 1;
+
+	return len;
+}
+
 #define IEEE80211_TWT_CONTROL_NDP			BIT(0)
 #define IEEE80211_TWT_CONTROL_RESP_MODE			BIT(1)
 #define IEEE80211_TWT_CONTROL_NEG_TYPE_BROADCAST	BIT(3)
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index b84150dbfe8c..7ce85266bcbb 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -7220,11 +7220,8 @@  static void ieee80211_rx_mgmt_beacon(struct ieee80211_link_data *link,
 	bssid = ieee80211_get_bssid(hdr, len, sdata->vif.type);
 	if (ieee80211_is_s1g_beacon(mgmt->frame_control)) {
 		struct ieee80211_ext *ext = (void *) mgmt;
-
-		if (ieee80211_is_s1g_short_beacon(ext->frame_control))
-			variable = ext->u.s1g_short_beacon.variable;
-		else
-			variable = ext->u.s1g_beacon.variable;
+		variable = ext->u.s1g_beacon.variable
+					+ ieee80211_s1g_optional_len(ext->frame_control);
 	}
 
 	baselen = (u8 *) variable - (u8 *) mgmt;
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 7b8da40a912d..6234f50a8a43 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -276,6 +276,7 @@  void ieee80211_scan_rx(struct ieee80211_local *local, struct sk_buff *skb)
 	struct ieee80211_mgmt *mgmt = (void *)skb->data;
 	struct ieee80211_bss *bss;
 	struct ieee80211_channel *channel;
+	struct ieee80211_ext *ext;
 	size_t min_hdr_len = offsetof(struct ieee80211_mgmt,
 				      u.probe_resp.variable);
 
@@ -285,12 +286,9 @@  void ieee80211_scan_rx(struct ieee80211_local *local, struct sk_buff *skb)
 		return;
 
 	if (ieee80211_is_s1g_beacon(mgmt->frame_control)) {
-		if (ieee80211_is_s1g_short_beacon(mgmt->frame_control))
-			min_hdr_len = offsetof(struct ieee80211_ext,
-					       u.s1g_short_beacon.variable);
-		else
-			min_hdr_len = offsetof(struct ieee80211_ext,
-					       u.s1g_beacon);
+		ext = (struct ieee80211_ext *)mgmt;
+		min_hdr_len = offsetof(struct ieee80211_ext, u.s1g_beacon.variable)
+				+ ieee80211_s1g_optional_len(ext->frame_control);
 	}
 
 	if (skb->len < min_hdr_len)
diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index ddd3a97f6609..6e1964f61246 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -3250,6 +3250,7 @@  cfg80211_inform_bss_frame_data(struct wiphy *wiphy,
 	const u8 *ie;
 	size_t ielen;
 	u64 tsf;
+	size_t s1g_optional_len;
 
 	if (WARN_ON(!mgmt))
 		return NULL;
@@ -3263,13 +3264,10 @@  cfg80211_inform_bss_frame_data(struct wiphy *wiphy,
 	trace_cfg80211_inform_bss_frame(wiphy, data, mgmt, len);
 
 	if (ieee80211_is_s1g_beacon(mgmt->frame_control)) {
-		ext = (void *) mgmt;
-		if (ieee80211_is_s1g_short_beacon(mgmt->frame_control))
-			min_hdr_len = offsetof(struct ieee80211_ext,
-					       u.s1g_short_beacon.variable);
-		else
-			min_hdr_len = offsetof(struct ieee80211_ext,
-					       u.s1g_beacon.variable);
+		ext = (struct ieee80211_ext *)mgmt;
+		s1g_optional_len = ieee80211_s1g_optional_len(ext->frame_control);
+		min_hdr_len = offsetof(struct ieee80211_ext, u.s1g_beacon.variable)
+					+ s1g_optional_len;
 	} else {
 		/* same for beacons */
 		min_hdr_len = offsetof(struct ieee80211_mgmt,
@@ -3285,11 +3283,7 @@  cfg80211_inform_bss_frame_data(struct wiphy *wiphy,
 		const struct ieee80211_s1g_bcn_compat_ie *compat;
 		const struct element *elem;
 
-		if (ieee80211_is_s1g_short_beacon(mgmt->frame_control))
-			ie = ext->u.s1g_short_beacon.variable;
-		else
-			ie = ext->u.s1g_beacon.variable;
-
+		ie = ext->u.s1g_beacon.variable + s1g_optional_len;
 		elem = cfg80211_find_elem(WLAN_EID_S1G_BCN_COMPAT, ie, ielen);
 		if (!elem)
 			return NULL;