diff mbox

[15/25] WNM: Add candidate list to BSS transition response

Message ID 1455548043-22427-30-git-send-email-ilan.peer@intel.com
State Accepted
Headers show

Commit Message

Ilan Peer Feb. 15, 2016, 2:53 p.m. UTC 
From: Avraham Stern <avraham.stern@intel.com>

Add the transition candidate list to BSS transition response.
The candidates preference is set using the regular wpa_supplicant
BSS selection logic.
If the BSS transition request is rejected and updated scan results
are not available, the list is not added.

Signed-off-by: Avraham Stern <avraham.stern@intel.com>
---
 src/common/ieee802_11_defs.h      |  18 ++++
 wpa_supplicant/events.c           |   8 +-
 wpa_supplicant/wnm_sta.c          | 183 +++++++++++++++++++++++++++++++++++++-
 wpa_supplicant/wpa_supplicant_i.h |   5 ++
 4 files changed, 209 insertions(+), 5 deletions(-)

Comments

Jouni Malinen Feb. 22, 2016, 8:23 a.m. UTC | #1 
On Mon, Feb 15, 2016 at 04:53:41PM +0200, Ilan Peer wrote:
> Add the transition candidate list to BSS transition response.
> The candidates preference is set using the regular wpa_supplicant
> BSS selection logic.
> If the BSS transition request is rejected and updated scan results
> are not available, the list is not added.

> +static int wnm_nei_rep_add_bss(struct wpa_supplicant *wpa_s,

> +	ie = wpa_bss_get_ie(bss, WLAN_EID_VHT_OPERATION);
> +	if (ie) {
> +		vht_oper = (struct ieee80211_vht_operation *)(ie + 2);
> +
> +		switch (vht_oper->vht_op_info_chwidth) {

This could result in buffer overflow since ie[1] was not checked to be
large enough. I'm just noting this here, but there are similar buffer
overflow through number of these patches. Whenever using information
received from an external entity, all the length fields need to be
checked to be valid before reading data.

> +		case 2:
> +			vht = VHT_CHANWIDTH_80MHZ;
> +			break;
> +		case 3:
> +			vht = VHT_CHANWIDTH_160MHZ;
> +			break;
> +		case 4:
> +			vht = VHT_CHANWIDTH_80P80MHZ;
> +			break;
> +		default:
> +			vht = 0;
> +		}
> +	}
> +
> +	if (ieee80211_freq_to_channel_ext(bss->freq, sec_chan, vht, &op_class,
> +					  &chan) == NUM_HOSTAPD_MODES) {

Why is that conversion from vht_oper->vht_op_info_chwidth to vht used
here when the Channel Width field in the VHT Operation element is using
those VHT_CHANWIDTH_* values? The values 2, 3, and 4 here do not match
the values used in this field and the correct thing to do would seem to
be simply to set vht = vht_oper->vht_op_info_chwidth.
Ilan Peer Feb. 22, 2016, 9:35 a.m. UTC | #2 
> -----Original Message-----
> From: Jouni Malinen [mailto:j@w1.fi]
> Sent: Monday, February 22, 2016 10:24
> To: Peer, Ilan
> Cc: hostap@lists.infradead.org; Stern, Avraham
> Subject: Re: [PATCH 15/25] WNM: Add candidate list to BSS transition
> response
> 
> On Mon, Feb 15, 2016 at 04:53:41PM +0200, Ilan Peer wrote:
> > Add the transition candidate list to BSS transition response.
> > The candidates preference is set using the regular wpa_supplicant BSS
> > selection logic.
> > If the BSS transition request is rejected and updated scan results are
> > not available, the list is not added.
> 
> > +static int wnm_nei_rep_add_bss(struct wpa_supplicant *wpa_s,
> 
> > +	ie = wpa_bss_get_ie(bss, WLAN_EID_VHT_OPERATION);
> > +	if (ie) {
> > +		vht_oper = (struct ieee80211_vht_operation *)(ie + 2);
> > +
> > +		switch (vht_oper->vht_op_info_chwidth) {
> 
> This could result in buffer overflow since ie[1] was not checked to be large
> enough. I'm just noting this here, but there are similar buffer overflow
> through number of these patches. Whenever using information received
> from an external entity, all the length fields need to be checked to be valid
> before reading data.
> 

Agree. 

> > +		case 2:
> > +			vht = VHT_CHANWIDTH_80MHZ;
> > +			break;
> > +		case 3:
> > +			vht = VHT_CHANWIDTH_160MHZ;
> > +			break;
> > +		case 4:
> > +			vht = VHT_CHANWIDTH_80P80MHZ;
> > +			break;
> > +		default:
> > +			vht = 0;
> > +		}
> > +	}
> > +
> > +	if (ieee80211_freq_to_channel_ext(bss->freq, sec_chan, vht,
> &op_class,
> > +					  &chan) == NUM_HOSTAPD_MODES)
> {
> 
> Why is that conversion from vht_oper->vht_op_info_chwidth to vht used
> here when the Channel Width field in the VHT Operation element is using
> those VHT_CHANWIDTH_* values? The values 2, 3, and 4 here do not match
> the values used in this field and the correct thing to do would seem to be
> simply to set vht = vht_oper->vht_op_info_chwidth.
> 

This is indeed wrong. I think that we got this confused with the values defined in Table 8-151 (HT/VHT Operation Information subfields) as defined in P802.11REVmc D4.3.

Shall I resubmit this patch with the fixes?

Thanks in advance,

Ilan.
Jouni Malinen Feb. 22, 2016, 10:13 a.m. UTC | #3 
On Mon, Feb 22, 2016 at 09:35:18AM +0000, Peer, Ilan wrote:
> > Why is that conversion from vht_oper->vht_op_info_chwidth to vht used
> > here when the Channel Width field in the VHT Operation element is using
> > those VHT_CHANWIDTH_* values? The values 2, 3, and 4 here do not match
> > the values used in this field and the correct thing to do would seem to be
> > simply to set vht = vht_oper->vht_op_info_chwidth.
> > 
> 
> This is indeed wrong. I think that we got this confused with the values defined in Table 8-151 (HT/VHT Operation Information subfields) as defined in P802.11REVmc D4.3.
> 
> Shall I resubmit this patch with the fixes?

No need, I've already fixed it. I think I'm now mostly done with the
review and cleanup of this series. The current snapshot is in the
pending branch of hostap.git. Please let me know if you happen to notice
something odd there; otherwise, I'm likely to push these into the master
branch later today.
Ilan Peer Feb. 22, 2016, 12:09 p.m. UTC | #4 
> -----Original Message-----
> From: Jouni Malinen [mailto:j@w1.fi]
> Sent: Monday, February 22, 2016 12:13
> To: Peer, Ilan
> Cc: hostap@lists.infradead.org; Stern, Avraham
> Subject: Re: [PATCH 15/25] WNM: Add candidate list to BSS transition
> response
> 
> On Mon, Feb 22, 2016 at 09:35:18AM +0000, Peer, Ilan wrote:
> > > Why is that conversion from vht_oper->vht_op_info_chwidth to vht
> > > used here when the Channel Width field in the VHT Operation element
> > > is using those VHT_CHANWIDTH_* values? The values 2, 3, and 4 here
> > > do not match the values used in this field and the correct thing to
> > > do would seem to be simply to set vht = vht_oper->vht_op_info_chwidth.
> > >
> >
> > This is indeed wrong. I think that we got this confused with the values
> defined in Table 8-151 (HT/VHT Operation Information subfields) as defined in
> P802.11REVmc D4.3.
> >
> > Shall I resubmit this patch with the fixes?
> 
> No need, I've already fixed it. I think I'm now mostly done with the review and
> cleanup of this series. The current snapshot is in the pending branch of
> hostap.git. Please let me know if you happen to notice something odd there;
> otherwise, I'm likely to push these into the master branch later today.
> 

I'll try to get to it sometime today. 

Thanks,

Ilan.
diff mbox

Patch

diff --git a/src/common/ieee802_11_defs.h b/src/common/ieee802_11_defs.h
index 9503bf5..9aea648 100644
--- a/src/common/ieee802_11_defs.h
+++ b/src/common/ieee802_11_defs.h
@@ -94,8 +94,13 @@ 
 #define WLAN_CAPABILITY_PBCC BIT(6)
 #define WLAN_CAPABILITY_CHANNEL_AGILITY BIT(7)
 #define WLAN_CAPABILITY_SPECTRUM_MGMT BIT(8)
+#define WLAN_CAPABILITY_QOS BIT(9)
 #define WLAN_CAPABILITY_SHORT_SLOT_TIME BIT(10)
+#define WLAN_CAPABILITY_APSD BIT(11)
+#define WLAN_CAPABILITY_RADIO_MEASUREMENT BIT(12)
 #define WLAN_CAPABILITY_DSSS_OFDM BIT(13)
+#define WLAN_CAPABILITY_DELAYED_BLOCK_ACK BIT(14)
+#define WLAN_CAPABILITY_IMM_BLOCK_ACK BIT(15)
 
 /* Status codes (IEEE 802.11-2007, 7.3.1.9, Table 7-23) */
 #define WLAN_STATUS_SUCCESS 0
@@ -1514,4 +1519,17 @@  enum phy_type {
 	PHY_TYPE_VHT = 9,
 };
 
+
+/* 802.11-2012 spec, chapter 8.4.2.39 "Neighbor Report element" */
+#define	NEI_REP_BSSID_INFO_SECURITY BIT(2)
+#define	NEI_REP_BSSID_INFO_KEY_SCOPE BIT(3)
+#define	NEI_REP_BSSID_INFO_SPECTRUM_MGMT BIT(4)
+#define	NEI_REP_BSSID_INFO_QOS BIT(5)
+#define	NEI_REP_BSSID_INFO_APSD BIT(6)
+#define	NEI_REP_BSSID_INFO_RM BIT(7)
+#define	NEI_REP_BSSID_INFO_DELAYED_BA BIT(8)
+#define	NEI_REP_BSSID_INFO_IMM_BA BIT(9)
+#define	NEI_REP_BSSID_INFO_MOBILITY_DOMAIN BIT(10)
+#define	NEI_REP_BSSID_INFO_HT BIT(11)
+
 #endif /* IEEE802_11_DEFS_H */
diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c
index de4ef91..b0f1fad 100644
--- a/wpa_supplicant/events.c
+++ b/wpa_supplicant/events.c
@@ -816,10 +816,10 @@  static int addr_in_list(const u8 *addr, const u8 *list, size_t num)
 }
 
 
-static struct wpa_ssid * wpa_scan_res_match(struct wpa_supplicant *wpa_s,
-					    int i, struct wpa_bss *bss,
-					    struct wpa_ssid *group,
-					    int only_first_ssid)
+struct wpa_ssid *wpa_scan_res_match(struct wpa_supplicant *wpa_s,
+				    int i, struct wpa_bss *bss,
+				    struct wpa_ssid *group,
+				    int only_first_ssid)
 {
 	u8 wpa_ie_len, rsn_ie_len;
 	int wpa;
diff --git a/wpa_supplicant/wnm_sta.c b/wpa_supplicant/wnm_sta.c
index 7270b88..e76a43e 100644
--- a/wpa_supplicant/wnm_sta.c
+++ b/wpa_supplicant/wnm_sta.c
@@ -574,12 +574,190 @@  compare_scan_neighbor_results(struct wpa_supplicant *wpa_s)
 }
 
 
+static int wpa_bss_ies_eq(struct wpa_bss *a, struct wpa_bss *b, u8 eid)
+{
+	const u8 *ie_a, *ie_b;
+
+	if (!a || !b)
+		return 0;
+
+	ie_a = wpa_bss_get_ie(a, eid);
+	ie_b = wpa_bss_get_ie(b, eid);
+
+	if (!ie_a || !ie_b || ie_a[1] != ie_b[1])
+		return 0;
+
+	return !os_memcmp(ie_a, ie_b, ie_a[1]);
+}
+
+
+static u32 wnm_get_bss_info(struct wpa_supplicant *wpa_s, struct wpa_bss *bss)
+{
+	u32 info = 0;
+
+	/* AP reachability unknown */
+	WPA_PUT_LE16((u8 *)&info, 2);
+
+	/* Leave the security and key scope bits unset to indicate that the
+	 * security information is not available */
+
+	if (bss->caps & WLAN_CAPABILITY_SPECTRUM_MGMT)
+		info |= NEI_REP_BSSID_INFO_SPECTRUM_MGMT;
+	if (bss->caps & WLAN_CAPABILITY_QOS)
+		info |= NEI_REP_BSSID_INFO_QOS;
+	if (bss->caps & WLAN_CAPABILITY_APSD)
+		info |= NEI_REP_BSSID_INFO_APSD;
+	if (bss->caps & WLAN_CAPABILITY_RADIO_MEASUREMENT)
+		info |= NEI_REP_BSSID_INFO_RM;
+	if (bss->caps & WLAN_CAPABILITY_DELAYED_BLOCK_ACK)
+		info |= NEI_REP_BSSID_INFO_DELAYED_BA;
+	if (bss->caps & WLAN_CAPABILITY_IMM_BLOCK_ACK)
+		info |= NEI_REP_BSSID_INFO_IMM_BA;
+
+	if (wpa_bss_ies_eq(bss, wpa_s->current_bss, WLAN_EID_MOBILITY_DOMAIN))
+		info |= NEI_REP_BSSID_INFO_MOBILITY_DOMAIN;
+
+	if (wpa_bss_ies_eq(bss, wpa_s->current_bss, WLAN_EID_HT_CAP))
+		info |= NEI_REP_BSSID_INFO_HT;
+
+	return info;
+}
+
+static int wnm_add_nei_rep(u8 *buf, size_t len, u8 *bssid, u32 bss_info,
+			   u8 op_class, u8 chan, u8 phy_type, u8 pref)
+{
+	u8 *pos = buf;
+
+	if (len < 18) {
+		wpa_printf(MSG_DEBUG,
+			   "WNM: Not enough room for Neighbor report IE");
+		return -1;
+	}
+
+	*pos++ = WLAN_EID_NEIGHBOR_REPORT;
+	/* length: 13 for basic neighbor report + 3 for preference subelement */
+	*pos++ = 16;
+	os_memcpy(pos, bssid, ETH_ALEN);
+	pos += 6;
+	os_memcpy(pos, &bss_info, sizeof(bss_info));
+	pos += 4;
+	*pos++ = op_class;
+	*pos++ = chan;
+	*pos++ = phy_type;
+	*pos++ = WNM_NEIGHBOR_BSS_TRANSITION_CANDIDATE;
+	*pos++ = 1;
+	*pos++ = pref;
+	return pos - buf;
+}
+
+static int wnm_nei_rep_add_bss(struct wpa_supplicant *wpa_s,
+			       struct wpa_bss *bss, u8 *buf, size_t len,
+			       u8 pref)
+{
+	const u8 *ie;
+	u8 op_class, chan;
+	int sec_chan = 0, vht = 0;
+	enum phy_type phy_type;
+	u32 info;
+	struct ieee80211_ht_operation *ht_oper = NULL;
+	struct ieee80211_vht_operation *vht_oper = NULL;
+
+	ie = wpa_bss_get_ie(bss, WLAN_EID_HT_OPERATION);
+	if (ie) {
+		ht_oper = (struct ieee80211_ht_operation *)(ie + 2);
+
+		if (ht_oper->ht_param & HT_INFO_HT_PARAM_SECONDARY_CHNL_ABOVE)
+			sec_chan = 1;
+		else if (ht_oper->ht_param &
+			 HT_INFO_HT_PARAM_SECONDARY_CHNL_BELOW)
+			sec_chan = -1;
+	}
+
+	ie = wpa_bss_get_ie(bss, WLAN_EID_VHT_OPERATION);
+	if (ie) {
+		vht_oper = (struct ieee80211_vht_operation *)(ie + 2);
+
+		switch (vht_oper->vht_op_info_chwidth) {
+		case 2:
+			vht = VHT_CHANWIDTH_80MHZ;
+			break;
+		case 3:
+			vht = VHT_CHANWIDTH_160MHZ;
+			break;
+		case 4:
+			vht = VHT_CHANWIDTH_80P80MHZ;
+			break;
+		default:
+			vht = 0;
+		}
+	}
+
+	if (ieee80211_freq_to_channel_ext(bss->freq, sec_chan, vht, &op_class,
+					  &chan) == NUM_HOSTAPD_MODES) {
+		wpa_printf(MSG_DEBUG,
+			   "WNM: Cannot determine operating class and channel");
+		return -1;
+	}
+
+	phy_type = ieee80211_get_phy_type(bss->freq, (ht_oper != NULL),
+					  (vht_oper != NULL));
+	if (phy_type == PHY_TYPE_UNSPECIFIED) {
+		wpa_printf(MSG_DEBUG,
+			   "WNM: Cannot determine BSS phy type for Neighbor Report");
+		return -1;
+	}
+
+	info = wnm_get_bss_info(wpa_s, bss);
+
+	return wnm_add_nei_rep(buf, len, bss->bssid, info, op_class, chan,
+			       phy_type, pref);
+}
+
+
+static int wnm_add_cand_list(struct wpa_supplicant *wpa_s, u8 *buf, size_t len)
+{
+	u8 *pos = buf;
+	unsigned int i, pref = 255;
+	struct os_reltime now;
+
+	/*
+	 * TODO: Define when scan results are no longer valid for the candidate
+	 * list.
+	 */
+	os_get_reltime(&now);
+	if (os_reltime_expired(&now, &wpa_s->last_scan, 10))
+		return 0;
+
+	wpa_printf(MSG_DEBUG,
+		   "WNM: Adding candidate list to BSS Transition Management Response");
+	for (i = 0; i < wpa_s->last_scan_res_used && pref; i++) {
+		struct wpa_bss *bss = wpa_s->last_scan_res[i];
+		int res;
+
+		if (wpa_scan_res_match(wpa_s, i, bss, wpa_s->current_ssid, 1)) {
+			res = wnm_nei_rep_add_bss(wpa_s, bss, pos, len, pref--);
+			if (res < 0)
+				return pos - buf;
+
+			pos += res;
+			len -= res;
+		}
+	}
+
+	wpa_hexdump(MSG_DEBUG,
+		    "WNM: BSS Transition Management Response candidate list",
+		    buf, pos - buf);
+
+	return pos - buf;
+}
+
+
 static void wnm_send_bss_transition_mgmt_resp(
 	struct wpa_supplicant *wpa_s, u8 dialog_token,
 	enum bss_trans_mgmt_status_code status, u8 delay,
 	const u8 *target_bssid)
 {
-	u8 buf[1000], *pos;
+	u8 buf[2000], *pos;
 	struct ieee80211_mgmt *mgmt;
 	size_t len;
 	int res;
@@ -619,6 +797,9 @@  static void wnm_send_bss_transition_mgmt_resp(
 		pos += ETH_ALEN;
 	}
 
+	if (status == WNM_BSS_TM_ACCEPT)
+		pos += wnm_add_cand_list(wpa_s, pos, buf + sizeof(buf) - pos);
+
 #ifdef CONFIG_MBO
 	if (status != WNM_BSS_TM_ACCEPT) {
 		pos += wpas_mbo_ie_bss_trans_reject(wpa_s, pos,
diff --git a/wpa_supplicant/wpa_supplicant_i.h b/wpa_supplicant/wpa_supplicant_i.h
index 2ab7026..ac45fcf 100644
--- a/wpa_supplicant/wpa_supplicant_i.h
+++ b/wpa_supplicant/wpa_supplicant_i.h
@@ -1252,4 +1252,9 @@  struct hostapd_hw_modes *get_mode(struct hostapd_hw_modes *modes,
 
 void wpa_bss_tmp_disallow(struct wpa_supplicant *wpa_s, u8 *bssid, u16 sec);
 int wpa_is_bss_tmp_disallowed(struct wpa_supplicant *wpa_s, u8 *bssid);
+
+struct wpa_ssid *wpa_scan_res_match(struct wpa_supplicant *wpa_s,
+				    int i, struct wpa_bss *bss,
+				    struct wpa_ssid *group,
+				    int only_first_ssid);
 #endif /* WPA_SUPPLICANT_I_H */