[PATCH] hostapd: Disable HT MCS and set basic MCS per BSS

[Jouni Malinen]

On Tue, Jun 30, 2020 at 12:03:56PM +0300, Shay Bar wrote:
> Add the ability to disable specific supported HT MCS's per BSS via the new
> (can be per bss) disabled_ht_mcs config.
> This will overwrite the configured HT capab supported_mcs_set.
> 
> Add the ability to set specific basic MCS's (can be per BSS)
> If absent (default) - no basic MCS's are set.

This is also whitespace damaged and does not apply.

> diff --git a/hostapd/hostapd.conf b/hostapd/hostapd.conf
> +# Disable supported HT MCS's per BSS
> +# List of MCS's to disable
> +# If absent (default) MCS's are as configured in the HT capab supported_mcs_set
> +#disabled_ht_mcs=0 1

That comment is a bit confusing.. Wouldn't the default be to enable all
HT-MCSs supported by the hardware?

> diff --git a/src/ap/ap_config.h b/src/ap/ap_config.h
> @@ -314,6 +314,9 @@ struct hostapd_bss_config {
> +       int *disabled_ht_mcs;
> +       int *basic_ht_mcs;

I did not see any addition for freeing the memory allocated for this..
That memory leak needs to be fixed in hostapd_config_free_bss().

> diff --git a/src/ap/ieee802_11_ht.c b/src/ap/ieee802_11_ht.c
> @@ -40,6 +41,18 @@ u8 * hostapd_eid_ht_capabilities(struct hostapd_data *hapd, u8 *eid)
>         os_memcpy(cap->supported_mcs_set, hapd->iface->current_mode->mcs_set,
>                   16);
> 
> +       if (disabled_ht_mcs) {
> +               int i = 0;
> +
> +               while (disabled_ht_mcs[i] != -1) {
> +                       u8 *supp_mcs_set = cap->supported_mcs_set;
> +
> +                       while (*supp_mcs_set)
> +                               *supp_mcs_set++ &= ~(1 << disabled_ht_mcs[i]);
> +                       i++;
> +               }
> +       }

How is that while loop support to work, i.e., where is it supposed to
find the zero that terminates the look? This is pointing to the binary
HT Capabilities element and there is no zero termination guaranteed for
the subfields.. Isn't this supposed to only apply for the Rx MCS Bitmask
subfield? And is that ANDing really support to be done in that manner? I
thought the disabled_ht_mcs[] values would be MCS indexes while the Rx
MCS Bitmask is a subfield of 77 bits.. (See the loop below for
basic_mcs_set[] using quite different mapping of the values.)

> @@ -83,6 +96,7 @@ u8 * hostapd_eid_ht_operation(struct hostapd_data *hapd, u8 *eid)
> +       int *basic = hapd->conf->basic_ht_mcs;

> +       if (basic) {
> +               while (*basic != -1) {
> +                       oper->basic_mcs_set[*basic / 8] |= (1 << (*basic % 8));
> +                       basic++;
> +               }
> +       }

Should this have bounds checking to make sure *basic does not have a
value larger than 76 (i.e., maximum defined HT-MCS value)? At minimum,
this needs to have checks to prevent writing beyond the end of the
basic_mcs_set[] array.

-- 
Jouni Malinen                                            PGP id EFC895FA