On Mon, Jul 14, 2025 at 03:00:11PM +0200, Johannes Berg wrote:
> 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.
>
Yea, forgot to describe the max attributes :( Will fix for next set.
> > +/*
> > + * 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?
In that case, would you have any opposition to using the regular
validate_beacon_head and validate_ie_attr for short beacon validation?
> 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?
The lack of validation is one of the issues I have with doing this
"correctly". I've made some changes here so I'll leave that to be
discussed in the forthcoming patchset.
lachlan
