by Benjamin Berg

[permalink] [raw]

Subject: Re: [PATCH 03/15] wifi: cfg80211: add RNR with reporting AP information

Hi,

just noticed this patch has an off-by-one error and it drops the last
byte of the generated RNR.

On Tue, 2024-01-02 at 21:35 +0200, Miri Korenblit wrote:
> From: Benjamin Berg <[email protected]>
>
> If the reporting AP is part of the same MLD, then an entry in the RNR
> is
> required in order to discover it again from the BSS generated from
> the
> per-STA profile in the Multi-Link Probe Response.
>
> We need this because we do not have a direct concept of an MLD AP and
> just do the lookup from one to the other on the fly if needed. As
> such,
> we need to ensure that this lookup will work both ways.
>
> Fixes: 2481b5da9c6b ("wifi: cfg80211: handle BSS data contained in ML
> probe responses")
> Signed-off-by: Benjamin Berg <[email protected]>
> Reviewed-by: Johannes Berg <[email protected]>
> Signed-off-by: Miri Korenblit <[email protected]>
> ---
> net/wireless/scan.c | 134
> ++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 129 insertions(+), 5 deletions(-)
>
> diff --git a/net/wireless/scan.c b/net/wireless/scan.c
> index 42555753b947..dbb5885d40e7 100644
> --- a/net/wireless/scan.c
> +++ b/net/wireless/scan.c
> @@ -2614,6 +2614,103 @@ cfg80211_tbtt_info_for_mld_ap(const u8 *ie,
> size_t ielen, u8 mld_id, u8 link_id,
>         return 0;
> }
>
> +static struct element *
> +cfg80211_gen_reporter_rnr(struct cfg80211_bss *source_bss, bool
> is_mbssid,
> +                         bool same_mld, u8 link_id, u8
> bss_change_count,
> +                         gfp_t gfp)
> +{
> +       const struct cfg80211_bss_ies *ies;
> +       struct ieee80211_neighbor_ap_info ap_info;
> +       struct ieee80211_tbtt_info_ge_11 tbtt_info;
> +       u32 short_ssid;
> +       const struct element *elem;
> +       struct element *res;
> +
> +       /*
> +        * We only generate the RNR to permit ML lookups. For that we
> do not
> +        * need an entry for the corresponding transmitting BSS, lets
> just skip
> +        * it even though it would be easy to add.
> +        */
> +       if (!same_mld)
> +               return NULL;
> +
> +       /* We could use tx_data->ies if we change
> cfg80211_calc_short_ssid */
> +       rcu_read_lock();
> +       ies = rcu_dereference(source_bss->ies);
> +
> +       ap_info.tbtt_info_len = offsetofend(typeof(tbtt_info),
> mld_params);
> +       ap_info.tbtt_info_hdr =
> +                       u8_encode_bits(IEEE80211_TBTT_INFO_TYPE_TBTT,
> +
> IEEE80211_AP_INFO_TBTT_HDR_TYPE) |
> +                       u8_encode_bits(0,
> IEEE80211_AP_INFO_TBTT_HDR_COUNT);
> +
> +       ap_info.channel = ieee80211_frequency_to_channel(source_bss-
> >channel->center_freq);
> +
> +       /* operating class */
> +       elem =
> cfg80211_find_elem(WLAN_EID_SUPPORTED_REGULATORY_CLASSES,
> +                                 ies->data, ies->len);
> +       if (elem && elem->datalen >= 1) {
> +               ap_info.op_class = elem->data[0];
> +       } else {
> +               struct cfg80211_chan_def chandef;
> +
> +               /* The AP is not providing us with anything to work
> with. So
> +                * make up a somewhat reasonable operating class, but
> don't
> +                * bother with it too much as no one will ever use
> the
> +                * information.
> +                */
> +               cfg80211_chandef_create(&chandef, source_bss-
> >channel,
> +                                       NL80211_CHAN_NO_HT);
> +
> +               if (!ieee80211_chandef_to_operating_class(&chandef,
> +
> &ap_info.op_class))
> +                       goto out_unlock;
> +       }
> +
> +       /* Just set TBTT offset and PSD 20 to invalid/unknown */
> +       tbtt_info.tbtt_offset = 255;
> +       tbtt_info.psd_20 = IEEE80211_RNR_TBTT_PARAMS_PSD_RESERVED;
> +
> +       memcpy(tbtt_info.bssid, source_bss->bssid, ETH_ALEN);
> +       if (cfg80211_calc_short_ssid(ies, &elem, &short_ssid))
> +               goto out_unlock;
> +
> +       rcu_read_unlock();
> +
> +       tbtt_info.short_ssid = cpu_to_le32(short_ssid);
> +
> +       tbtt_info.bss_params = IEEE80211_RNR_TBTT_PARAMS_SAME_SSID;
> +
> +       if (is_mbssid) {
> +               tbtt_info.bss_params |=
> IEEE80211_RNR_TBTT_PARAMS_MULTI_BSSID;
> +               tbtt_info.bss_params |=
> IEEE80211_RNR_TBTT_PARAMS_TRANSMITTED_BSSID;
> +       }
> +
> +       tbtt_info.mld_params.mld_id = 0;
> +       tbtt_info.mld_params.params =
> +               le16_encode_bits(link_id,
> IEEE80211_RNR_MLD_PARAMS_LINK_ID) |
> +               le16_encode_bits(bss_change_count,
> +
> IEEE80211_RNR_MLD_PARAMS_BSS_CHANGE_COUNT);
> +
> +       res = kzalloc(struct_size(res, data,
> +                                 sizeof(ap_info) +
> ap_info.tbtt_info_len),
> +                     gfp);
> +       if (!res)
> +               return NULL;
> +
> +       /* Copy the data */
> +       res->id = WLAN_EID_REDUCED_NEIGHBOR_REPORT;
> +       res->datalen = sizeof(ap_info) + ap_info.tbtt_info_len;
> +       memcpy(res->data, &ap_info, sizeof(ap_info));
> +       memcpy(res->data + sizeof(ap_info), &tbtt_info,
> ap_info.tbtt_info_len);
> +
> +       return res;
> +
> +out_unlock:
> +       rcu_read_unlock();
> +       return NULL;
> +}
> +
> static void
> cfg80211_parse_ml_elem_sta_data(struct wiphy *wiphy,
>                                 struct
> cfg80211_inform_single_bss_data *tx_data,
> @@ -2627,13 +2724,14 @@ cfg80211_parse_ml_elem_sta_data(struct wiphy
> *wiphy,
>                 .source_bss = source_bss,
>                 .bss_source = BSS_SOURCE_STA_PROFILE,
>         };
> +       struct element *reporter_rnr = NULL;
>         struct ieee80211_multi_link_elem *ml_elem;
>         struct cfg80211_mle *mle;
>         u16 control;
>         u8 ml_common_len;
> -       u8 *new_ie;
> +       u8 *new_ie = NULL;
>         struct cfg80211_bss *bss;
> -       int mld_id;
> +       u8 mld_id, reporter_link_id, bss_change_count;
>         u16 seen_links = 0;
>         const u8 *pos;
>         u8 i;
> @@ -2655,8 +2753,14 @@ cfg80211_parse_ml_elem_sta_data(struct wiphy
> *wiphy,
>
>         ml_common_len = ml_elem->variable[0];
>
> -       /* length + MLD MAC address + link ID info + BSS Params
> Change Count */
> -       pos = ml_elem->variable + 1 + 6 + 1 + 1;
> +       /* length + MLD MAC address */
> +       pos = ml_elem->variable + 1 + 6;
> +
> +       reporter_link_id = pos[0];
> +       pos += 1;
> +
> +       bss_change_count = pos[0];
> +       pos += 1;
>
>         if (u16_get_bits(control,
> IEEE80211_MLC_BASIC_PRES_MED_SYNC_DELAY))
>                 pos += 2;
> @@ -2687,10 +2791,21 @@ cfg80211_parse_ml_elem_sta_data(struct wiphy
> *wiphy,
>         if (!mle)
>                 return;
>
> +       /* No point in doing anything if there is no per-STA profile
> */
> +       if (!mle->sta_prof[0])
> +               goto out;
> +
>         new_ie = kmalloc(IEEE80211_MAX_DATA_LEN, gfp);
>         if (!new_ie)
>                 goto out;
>
> +       reporter_rnr = cfg80211_gen_reporter_rnr(source_bss,
> +
> u16_get_bits(control,
> +
> IEEE80211_MLC_BASIC_PRES_MLD_ID),
> +                                                mld_id == 0,
> reporter_link_id,
> +                                                bss_change_count,
> +                                                gfp);
> +
>         for (i = 0; i < ARRAY_SIZE(mle->sta_prof) && mle-
> >sta_prof[i]; i++) {
>                 const struct ieee80211_neighbor_ap_info *ap_info;
>                 enum nl80211_band band;
> @@ -2800,7 +2915,15 @@ cfg80211_parse_ml_elem_sta_data(struct wiphy
> *wiphy,
>
>                 data.ielen += sizeof(*ml_elem) + ml_common_len;
>
> -               /* TODO: Add an RNR containing only the reporting AP
> */
> +               if (reporter_rnr && (use_for &
> NL80211_BSS_USE_FOR_NORMAL)) {
> +                       if (data.ielen + 1 + reporter_rnr->datalen >
> +                           IEEE80211_MAX_DATA_LEN)
> +                               continue;
> +
> +                       memcpy(new_ie + data.ielen, reporter_rnr,
> +                              1 + reporter_rnr->datalen);
> +                       data.ielen += 1 + reporter_rnr->datalen;

i.e. everywhere here it needs to add 2 (1 byte EID, 1 byte datalen).

> +               }
>
>                 bss = cfg80211_inform_single_bss_data(wiphy, &data,
> gfp);
>                 if (!bss)
> @@ -2809,6 +2932,7 @@ cfg80211_parse_ml_elem_sta_data(struct wiphy
> *wiphy,
>         }
>
> out:
> +       kfree(reporter_rnr);
>         kfree(new_ie);
>         kfree(mle);
> }

Benjamin