On Mon, 2025-07-14 at 15:13 +1000, Lachlan Hodges wrote:
>
> +/**
> + * enum nl80211_s1g_short_beacon_attrs - S1G short beacon data
> + *
> + * @__NL80211_S1G_SHORT_BEACON_ATTR_INVALID: Invalid
> + *
> + * @NL80211_S1G_SHORT_BEACON_HEAD: Short beacon head (binary).
> + * @NL80211_S1G_SHORT_BEACON_TAIL: Short beacon tail (binary).
> + * @NL80211_S1G_SHORT_BEACON_INTERVAL: Time in TUs between each short
> + * beacon transmission (u32).
> + * @NL80211_S1G_SHORT_BEACON_DTIM_PERIOD: DTIM period for a short
> + * beaconing BSS (u8).
> + */
> +enum nl80211_s1g_short_beacon_attrs {
> + __NL80211_S1G_SHORT_BEACON_ATTR_INVALID,
> +
> + NL80211_S1G_SHORT_BEACON_HEAD,
> + NL80211_S1G_SHORT_BEACON_TAIL,
> + NL80211_S1G_SHORT_BEACON_INTERVAL,
> + NL80211_S1G_SHORT_BEACON_DTIM_PERIOD,
nit: we usually have _ATTR_ in there after the qualification, so
something like NL80211_S1G_SHORT_BEACON_ATTR_HEAD.
Also, the bot complained about some missing kernel-doc.
> +/*
> + * Short beacons contain a limited set of allowed elements as per
> + * IEEE80211-2024 9.3.4.3 Table 9-76. The TIM element is allowed,
> + * but as it is inserted by mac80211, we do not check for it.
> + */
> +static int is_valid_s1g_short_elem(const struct element *elem)
> +{
> + switch (elem->id) {
> + case WLAN_EID_FMS_DESCRIPTOR:
> + case WLAN_EID_RPS:
> + case WLAN_EID_SST:
> + case WLAN_EID_S1G_RELAY:
> + case WLAN_EID_PAGE_SLICE:
> + case WLAN_EID_VENDOR_SPECIFIC:
> + case WLAN_EID_MMIE:
> + case WLAN_EID_MIC:
> + return true;
> + default:
> + return false;
> + }
> +}
Is that really worth it? We don't have to protect userspace from
shooting it self into the foot _too_ much, just make sure that we don't
get into a mess in the kernel itself. As long as the elements are not
malformed, I'd argue we're fine from a kernel perspective?
This also prevents future updates and experimentation, and I see little
value in it?
> +static int nl80211_validate_s1g_short_conf(struct cfg80211_ap_settings *params)
> +{
> + struct cfg80211_s1g_short_beacon *sb = ¶ms->s1g_short_beacon;
> + u64 beacon_int = params->beacon_interval;
Why a u64, it's not a u64 in the params? and it makes the division
harder (well, really you wanted the remainder)
> + u32 short_int = sb->short_interval;
> +
> + /*
> + * As per IEEE80211 11.1.3.10.2, beacon_interval = n * short_interval
> + * where n is a positive integer. Meaning a BSS can be configured such
> + * that both the short beacon interval and long beacon interval are
> + * equivalent. This effectively means we aren't short beaconing. In
> + * this case, since the short beacon data is irellevent its probably a
typo: irrelevant
> + * misconfiguration and we should reject it.
> + */
> + if (sb->short_interval >= params->beacon_interval)
> + return -EINVAL;
> +
> + if (do_div(beacon_int, short_int))
> + return -EINVAL;
so that could just be
if (params->beacon_interval % sb->short_interval)
return -EINVAL;
no?
> +static int
> +nl80211_parse_s1g_short_beacon(struct cfg80211_registered_device *rdev,
> + struct nlattr *attrs,
> + struct cfg80211_s1g_short_beacon *sb)
> +{
> + struct nlattr *tb[NL80211_S1G_SHORT_BEACON_ATTR_MAX + 1];
> + int ret;
> +
> + if (!rdev->wiphy.bands[NL80211_BAND_S1GHZ])
> + return -EINVAL;
Maybe we should (instead?) check that it's operating as or being set up
for s1g? Not sure how easy that is, but I've actually wanted it in other
places before, I think.
> +
> + ret = nla_parse_nested(tb, NL80211_S1G_SHORT_BEACON_ATTR_MAX, attrs,
> + NULL, NULL);
> + if (ret)
> + return ret;
> +
> + /* Short beacon tail is optional (i.e might only include the TIM) */
> + if (!tb[NL80211_S1G_SHORT_BEACON_HEAD])
> + return -EINVAL;
> +
> + sb->update = false;
> + sb->short_head = nla_data(tb[NL80211_S1G_SHORT_BEACON_HEAD]);
> + sb->short_head_len = nla_len(tb[NL80211_S1G_SHORT_BEACON_HEAD]);
> + sb->short_tail_len = 0;
> +
> + if (tb[NL80211_S1G_SHORT_BEACON_TAIL]) {
> + sb->short_tail = nla_data(tb[NL80211_S1G_SHORT_BEACON_TAIL]);
> + sb->short_tail_len = nla_len(tb[NL80211_S1G_SHORT_BEACON_TAIL]);
> + }
> +
> + if (!tb[NL80211_S1G_SHORT_BEACON_INTERVAL] ||
> + !tb[NL80211_S1G_SHORT_BEACON_DTIM_PERIOD]) {
> + sb->update = true;
> + return 0;
> + }
> +
> + sb->short_interval =
> + nla_get_u32(tb[NL80211_S1G_SHORT_BEACON_INTERVAL]);
> + sb->short_dtim_period =
> + nla_get_u8(tb[NL80211_S1G_SHORT_BEACON_DTIM_PERIOD]);
> +
> + return 0;
> +}
> +
> static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
> {
> struct cfg80211_registered_device *rdev = info->user_ptr[0];
> @@ -6442,6 +6615,28 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
> goto out;
> }
>
> + if (info->attrs[NL80211_ATTR_S1G_SHORT_BEACON]) {
> + err = nl80211_parse_s1g_short_beacon(
> + rdev, info->attrs[NL80211_ATTR_S1G_SHORT_BEACON],
> + ¶ms->s1g_short_beacon);
> + if (err)
> + goto out;
> +
> + /*
> + * If we have only received the parameters to perform a
> + * short beacon update, return an error to usermode as
> + * the BSS has not yet been configured for short beaconing.
> + */
> + if (params->s1g_short_beacon.update) {
> + err = -EINVAL;
> + goto out;
> + }
> +
> + err = nl80211_validate_s1g_short_conf(params);
> + if (err)
> + goto out;
why not call the validation inside nl80211_parse_s1g_short_beacon()?
seems harder to misuse later then, and the order shouldn't matter much?
> @@ -6550,6 +6745,19 @@ static int nl80211_set_beacon(struct sk_buff *skb, struct genl_info *info)
> goto out;
> }
>
> + attr = info->attrs[NL80211_ATTR_S1G_SHORT_BEACON];
> + if (attr) {
> + err = nl80211_parse_s1g_short_beacon(rdev, attr,
> + ¶ms->s1g_short_beacon);
> + if (err)
> + goto out;
> +
> + if (!params->s1g_short_beacon.update) {
> + err = -EINVAL;
> + goto out;
> + }
> + }
And you already forgot the validation here, it seems?
Looks fine from the perspective of when/how you parse it, I think,
unless you were going to need channel switch and/or BSS color change.
johannes
