On Wed, 2022-11-16 at 15:06 +1300, Gilad Itzkovitch wrote:
>
> + * @NL80211_ATTR_SHORT_BEACON_HEAD: portion of the short beacon before the TIM IE.
> + * @NL80211_ATTR_SHORT_BEACON_TAIL: portion of the short beacon after the TIM.
I would tend to say "TIM element" these days, since the spec changed
this, but I guess it doesn't matter much.
> + * @NL80211_BSS_SHORT_BEACON_PERIOD: S1G short beacon period in TUs
> * @__NL80211_BSS_AFTER_LAST: internal
> * @NL80211_BSS_MAX: highest BSS attribute
> */
> @@ -4990,6 +5000,7 @@ enum nl80211_bss {
> NL80211_BSS_FREQUENCY_OFFSET,
> NL80211_BSS_MLO_LINK_ID,
> NL80211_BSS_MLD_ADDR,
> + NL80211_BSS_SHORT_BEACON_PERIOD,
You don't use this.
> +++ b/net/wireless/nl80211.c
> @@ -231,12 +231,18 @@ static int validate_beacon_head(const struct nlattr *attr,
> const struct ieee80211_mgmt *mgmt = (void *)data;
> unsigned int fixedlen, hdrlen;
> bool s1g_bcn;
> + bool s1g_short_bcn;
>
> if (len < offsetofend(typeof(*mgmt), frame_control))
> goto err;
>
> s1g_bcn = ieee80211_is_s1g_beacon(mgmt->frame_control);
> - if (s1g_bcn) {
> + s1g_short_bcn = ieee80211_is_s1g_short_beacon(mgmt->frame_control);
> + if (s1g_short_bcn) {
> + fixedlen = offsetof(struct ieee80211_ext,
> + u.s1g_short_beacon.variable);
> + hdrlen = offsetof(struct ieee80211_ext, u.s1g_short_beacon);
> + } else if (s1g_bcn) {
> fixedlen = offsetof(struct ieee80211_ext,
> u.s1g_beacon.variable);
> hdrlen = offsetof(struct ieee80211_ext, u.s1g_beacon);
Even the old code here had me worried a bit, and the new code doesn't
make this better - what if you try to set an S1G (short or not) beacon
when the interface isn't using S1G really?
> + [NL80211_ATTR_SHORT_BEACON_PERIOD] = { .type = NLA_U16 },
You can add a better range validation here, and then you don't need the
extra validation in the code later.
> + [NL80211_ATTR_SHORT_BEACON_HEAD] =
> + NLA_POLICY_VALIDATE_FN(NLA_BINARY, validate_beacon_head,
> + IEEE80211_MAX_DATA_LEN),
> + [NL80211_ATTR_SHORT_BEACON_TAIL] =
> + NLA_POLICY_VALIDATE_FN(NLA_BINARY, validate_ie_attr,
> + IEEE80211_MAX_DATA_LEN),
> +
nit: unnecessary blank line
> + if (info->attrs[NL80211_ATTR_SHORT_BEACON_PERIOD])
> + params->short_beacon_period =
> + nla_get_u32(info->attrs[NL80211_ATTR_SHORT_BEACON_PERIOD]) == 0 ?
> + 1 : nla_get_u32(info->attrs[NL80211_ATTR_SHORT_BEACON_PERIOD]);
i.e. you don't need the == 0 check if you just reject == 0.
Also, you're confusing the types - using NLA_U16 above but nla_get_u32()
here.
johannes
