[PATCH] WNM: Support for BSS Color Collision and BSS Color In Use of WNM event report

[Jouni Malinen]

On Tue, Feb 14, 2023 at 03:07:58PM -0800, Yi-Chia Hsieh wrote:
> Add support for WNM event report to handle BSS Color Collision and In Use event.

Thanks, applied with some fixes:

> diff --git a/src/ap/wnm_ap.c b/src/ap/wnm_ap.c
> +static void ieee802_11_rx_wnm_event_report(struct hostapd_data *hapd,
> +					   const u8 *addr, const u8 *buf,
> +					   size_t len)
> +{
> +	struct sta_info *sta;
> +	u8 dialog_token;
> +	struct wnm_event_report_element *report_ie;
> +
> +	if (len < 6) {
> +		wpa_printf(MSG_DEBUG, "WNM: Ignore too short WNM Event Report frame from "
> +			   MACSTR, MAC2STR(addr));
> +		return;
> +	}

I don't know where that limit of 6 is coming from, but the Event Report
element used here is quite a bit longer with the two eight octet fields
in the BSS color collision case.. In other words, this can result in
buffer read overflows.

> +	dialog_token = *buf++;

So this moves the start pointer by one without updating len..

> +	report_ie = (struct wnm_event_report_element *) buf;
> +
> +	if (report_ie->eid != WLAN_EID_EVENT_REPORT)
> +		return;
> +
> +	if (report_ie->status != WNM_STATUS_SUCCESSFUL)
> +		return;
> +
> +	wpa_printf(MSG_DEBUG, "WNM: Received WNM Event Report frame from "
> +		   MACSTR " dialog_token=%u autonomous=%s type=%d (%s)",
> +		   MAC2STR(addr), dialog_token, (!report_ie->token) ? "true" : "false",
> +		   report_ie->type, wnm_event_type2str(report_ie->type));
> +
> +	wpa_hexdump(MSG_MSGDUMP, "WNM: Event Report",
> +		    buf, len);

So that would also seem to read beyond the end of the buffer..

In addition, report_ie->len needs to be checked here as well for correct
functionality.

> +	switch (report_ie->type) {
> +	case WNM_EVENT_TYPE_BSS_COLOR_COLLISION:
> +		hostapd_switch_color(hapd->iface->bss[0],
> +				     report_ie->u.bss_color_collision.color_bitmap);

That color_bitmap was defined as u64 which is quite problematic both
from the view point of potentially unaligned 64-bit reads and byte order
issues.

This might also need to be updated to merge in reports from multiple
associated STAs and local information. This is something to consider for
additional patches in this area.

> +	case WNM_EVENT_TYPE_BSS_COLOR_INUSE:
> +		if (report_ie->u.bss_color_inuse.color > 0 &&
> +		    report_ie->u.bss_color_inuse.color < 64)
> +			hapd->color_collision_bitmap |=
> +				(1ULL << report_ie->u.bss_color_inuse.color);

This should also handle the canceling a color use report (color == 0). I
did not implement this now, but the correct behavior for this would
likely require maintaining a record of the reported color use(s) per STA
and clearing that whenever color == 0 is reported.
 
-- 
Jouni Malinen                                            PGP id EFC895FA