On 7/4/2024 2:28 PM, Johannes Berg wrote:
> On Thu, 2024-07-04 at 13:01 +0530, Sowmiya Sree Elavalagan wrote:
>>
>> I agree, but based on the current hostapd implementation, this flag NL80211_MBSSID_CONFIG_ATTR_EMA is set only when num_bss > 1. This flag will not be set when we do not have any non Tx BSS.
>>
>
> Sure, but "based on the current hostapd implementation" isn't really
> enough for the kernel to protect itself from userspace doing weird
> things that it isn't prepared to handle :-)
>
> It is, however, an argument for simply prohibiting it, I guess? If no
> userspace is going to do it anyway?
>
>> Even if this NL80211_MBSSID_CONFIG_ATTR_EMA is set when no TX BSS is present, can just fill beacon template in 0th index of ieee80211_ema_beacons structure, if mbssid_ies are not present.
>> Shall we handle this in mac80211 layer in ieee80211_beacon_get_ap_ema_list function like below
>>
>> ieee80211_beacon_get_ap_ema_list(struct ieee80211_hw *hw,
>> struct ieee80211_chanctx_conf *chanctx_conf)
>> {
>> ...
>>
>> - if (!beacon->mbssid_ies || !beacon->mbssid_ies->cnt)
>> - return NULL;
>> -
>> - ema = kzalloc(struct_size(ema, bcn, beacon->mbssid_ies->cnt),
>> + if (!beacon->mbssid_ies || !beacon->mbssid_ies->cnt) {
>> + ema = kzalloc(struct_size(ema, bcn, 1), GFP_ATOMIC);
>> + total_beacons = 1;
>> +
>> + } else {
>> + ema = kzalloc(struct_size(ema, bcn, beacon->mbssid_ies->cnt),
>> GFP_ATOMIC);
>> + total_beacons = beacon->mbssid_ies->cnt;
>> + }
>> +
>> if (!ema)
>> return NULL;
>>
>> - for (ema->cnt = 0; ema->cnt < beacon->mbssid_ies->cnt; ema->cnt++) {
>> + for (ema->cnt = 0; ema->cnt < total_beacons; ema->cnt++) {
>> .....
>>
>
> I don't know, is that really the only place? I didn't audit _all_ of it,
> just looked at the first plausible code path that might be broken ...
>
> Why can't we just prohibit it?
>
> johannes
Hi Johannes,
You are right, better to handle this in kernel. Shall we mandate num_elems and mbssid index check if EMA is enabled in nl80211_parse_mbssid_config function.
But we will have to revisit this when dynamic link addition is supported along with EMA.
Thanks,
Sowmiya Sree
