diff mbox series

Emit AuthBSS property

Message ID 20240401180747.1635441-1-rmekonnen@chromium.org
State Changes Requested
Headers show
Series Emit AuthBSS property | expand

Commit Message

Ruth Mekonnen April 1, 2024, 6:06 p.m. UTC 
This CL adds wpa_s->auth_bss and the getter AuthBSS to expose
the BSS the STA is trying to connect to at the AUTHENTICATING
state for SME devices and at the ASSOCIATING state for non-SME
devices. When the state is set to ASSOCIATED or DISCONNECTED
wpa_s->auth_bss set to null and emitted. The AuthBSS property
can be used by platforms to track connection attempt success
rates against APs.

Signed-off-by: Ruth Mekonnen <rmekonnen@chromium.org>
---
 wpa_supplicant/dbus/dbus_new.c          |  8 ++++++++
 wpa_supplicant/dbus/dbus_new.h          |  1 +
 wpa_supplicant/dbus/dbus_new_handlers.c | 27 +++++++++++++++++++++++++
 wpa_supplicant/dbus/dbus_new_handlers.h |  1 +
 wpa_supplicant/notify.c                 |  8 ++++++++
 wpa_supplicant/notify.h                 |  1 +
 wpa_supplicant/sme.c                    |  2 ++
 wpa_supplicant/wpa_supplicant.c         |  7 +++++++
 wpa_supplicant/wpa_supplicant_i.h       |  1 +
 9 files changed, 56 insertions(+)

Comments

Jouni Malinen Feb. 9, 2025, 11:16 a.m. UTC | #1 
On Mon, Apr 01, 2024 at 06:06:58PM +0000, Ruth Mekonnen wrote:
> This CL adds wpa_s->auth_bss and the getter AuthBSS to expose
> the BSS the STA is trying to connect to at the AUTHENTICATING
> state for SME devices and at the ASSOCIATING state for non-SME
> devices. When the state is set to ASSOCIATED or DISCONNECTED
> wpa_s->auth_bss set to null and emitted. The AuthBSS property
> can be used by platforms to track connection attempt success
> rates against APs.

This feels a bit unclear when there is already a CurrentBSS property
that seems to be used somewhat similarly. It would be helpful to update
doc/dbus.doxygen with clear description of the new AuthBSS property and
how it differs from the CurrentBSS property. At least for me, the name
AuthBSS seems to imply this has something to do with being
authenticated, but that is clearly not the case here since this is
cleared on not only when getting disconnected but also when getting
associated.

> diff --git a/wpa_supplicant/dbus/dbus_new_handlers.c b/wpa_supplicant/dbus/dbus_new_handlers.c
> +dbus_bool_t wpas_dbus_getter_auth_bss(
> +{
> +	if (wpa_s->auth_bss && wpa_s->dbus_new_path)
> +		os_snprintf(bss_obj_path, WPAS_DBUS_OBJECT_PATH_MAX,
> +			    "%s/" WPAS_DBUS_NEW_BSSIDS_PART "/%u",
> +			    wpa_s->dbus_new_path, wpa_s->auth_bss->id);

This could be used to dereference wpa_s->auth_bss based on an external
operation. That would mean that there needs to be clear protection
making sure that wpa_s->auth_bss can never point to freed memory. It was
not obvious from the proposed changes that this would be the case. For
example, wpa_bss_remove() could be used to clear wpa_s->auth_bss if it
happens to point to a BSS entry that is being freed.

> diff --git a/wpa_supplicant/wpa_supplicant_i.h b/wpa_supplicant/wpa_supplicant_i.h
> @@ -725,6 +725,7 @@ struct wpa_supplicant {
> +	struct wpa_bss *auth_bss;
>  	struct wpa_bss *current_bss;

It would likely be better to mark auth_bss const since it is not really
supposed to be used to modify anything in the BSS entry.
diff mbox series

Patch

diff --git a/wpa_supplicant/dbus/dbus_new.c b/wpa_supplicant/dbus/dbus_new.c
index 8bd6a9a43..2ed62350d 100644
--- a/wpa_supplicant/dbus/dbus_new.c
+++ b/wpa_supplicant/dbus/dbus_new.c
@@ -2362,6 +2362,9 @@  void wpas_dbus_signal_prop_changed(struct wpa_supplicant *wpa_s,
 	case WPAS_DBUS_PROP_STATE:
 		prop = "State";
 		break;
+	case WPAS_DBUS_PROP_AUTH_BSS:
+		prop = "AuthBSS";
+		break;
 	case WPAS_DBUS_PROP_CURRENT_BSS:
 		prop = "CurrentBSS";
 		break;
@@ -3830,6 +3833,11 @@  static const struct wpa_dbus_property_desc wpas_dbus_interface_properties[] = {
 	  NULL,
 	  NULL
 	},
+	{ "AuthBSS", WPAS_DBUS_NEW_IFACE_INTERFACE, "o",
+	  wpas_dbus_getter_auth_bss,
+	  NULL,
+	  NULL
+	},
 	{ "CurrentNetwork", WPAS_DBUS_NEW_IFACE_INTERFACE, "o",
 	  wpas_dbus_getter_current_network,
 	  NULL,
diff --git a/wpa_supplicant/dbus/dbus_new.h b/wpa_supplicant/dbus/dbus_new.h
index 952bb422a..0eaefade9 100644
--- a/wpa_supplicant/dbus/dbus_new.h
+++ b/wpa_supplicant/dbus/dbus_new.h
@@ -27,6 +27,7 @@  enum wpas_dbus_prop {
 	WPAS_DBUS_PROP_SCANNING,
 	WPAS_DBUS_PROP_STATE,
 	WPAS_DBUS_PROP_CURRENT_BSS,
+	WPAS_DBUS_PROP_AUTH_BSS,
 	WPAS_DBUS_PROP_CURRENT_NETWORK,
 	WPAS_DBUS_PROP_CURRENT_AUTH_MODE,
 	WPAS_DBUS_PROP_BSSS,
diff --git a/wpa_supplicant/dbus/dbus_new_handlers.c b/wpa_supplicant/dbus/dbus_new_handlers.c
index 3897d98f4..2052ccef4 100644
--- a/wpa_supplicant/dbus/dbus_new_handlers.c
+++ b/wpa_supplicant/dbus/dbus_new_handlers.c
@@ -4170,6 +4170,33 @@  dbus_bool_t wpas_dbus_getter_current_bss(
 						&bss_obj_path, error);
 }
 
+/**
+ * wpas_dbus_getter_auth_bss - Get auth bss object path
+ * @iter: Pointer to incoming dbus message iter
+ * @error: Location to store error on failure
+ * @user_data: Function specific data
+ * Returns: TRUE on success, FALSE on failure
+ *
+ * Getter for "AuthBSS" property.
+ */
+dbus_bool_t wpas_dbus_getter_auth_bss(
+	const struct wpa_dbus_property_desc *property_desc,
+	DBusMessageIter *iter, DBusError *error, void *user_data)
+{
+	struct wpa_supplicant *wpa_s = user_data;
+	char path_buf[WPAS_DBUS_OBJECT_PATH_MAX], *bss_obj_path = path_buf;
+
+	if (wpa_s->auth_bss && wpa_s->dbus_new_path)
+		os_snprintf(bss_obj_path, WPAS_DBUS_OBJECT_PATH_MAX,
+			    "%s/" WPAS_DBUS_NEW_BSSIDS_PART "/%u",
+			    wpa_s->dbus_new_path, wpa_s->auth_bss->id);
+	else
+		os_snprintf(bss_obj_path, WPAS_DBUS_OBJECT_PATH_MAX, "/");
+
+	return wpas_dbus_simple_property_getter(iter, DBUS_TYPE_OBJECT_PATH,
+						&bss_obj_path, error);
+}
+
 
 /**
  * wpas_dbus_getter_current_network - Get current network object path
diff --git a/wpa_supplicant/dbus/dbus_new_handlers.h b/wpa_supplicant/dbus/dbus_new_handlers.h
index acd6af7ff..642697091 100644
--- a/wpa_supplicant/dbus/dbus_new_handlers.h
+++ b/wpa_supplicant/dbus/dbus_new_handlers.h
@@ -189,6 +189,7 @@  DECLARE_ACCESSOR(wpas_dbus_getter_bridge_ifname);
 DECLARE_ACCESSOR(wpas_dbus_setter_bridge_ifname);
 DECLARE_ACCESSOR(wpas_dbus_getter_config_file);
 DECLARE_ACCESSOR(wpas_dbus_getter_current_bss);
+DECLARE_ACCESSOR(wpas_dbus_getter_auth_bss);
 DECLARE_ACCESSOR(wpas_dbus_getter_current_network);
 DECLARE_ACCESSOR(wpas_dbus_getter_current_auth_mode);
 DECLARE_ACCESSOR(wpas_dbus_getter_bsss);
diff --git a/wpa_supplicant/notify.c b/wpa_supplicant/notify.c
index 3ea1cbfaf..f3fbc8530 100644
--- a/wpa_supplicant/notify.c
+++ b/wpa_supplicant/notify.c
@@ -212,6 +212,14 @@  void wpas_notify_bssid_changed(struct wpa_supplicant *wpa_s)
 	wpas_dbus_signal_prop_changed(wpa_s, WPAS_DBUS_PROP_CURRENT_BSS);
 }
 
+void wpas_notify_auth_bssid_changed(struct wpa_supplicant *wpa_s)
+{
+	if (wpa_s->p2p_mgmt)
+		return;
+
+	wpas_dbus_signal_prop_changed(wpa_s, WPAS_DBUS_PROP_AUTH_BSS);
+}
+
 
 void wpas_notify_mac_address_changed(struct wpa_supplicant *wpa_s)
 {
diff --git a/wpa_supplicant/notify.h b/wpa_supplicant/notify.h
index d560d0b9b..1faefa982 100644
--- a/wpa_supplicant/notify.h
+++ b/wpa_supplicant/notify.h
@@ -35,6 +35,7 @@  void wpas_notify_bss_tm_status(struct wpa_supplicant *wpa_s);
 void wpas_notify_network_changed(struct wpa_supplicant *wpa_s);
 void wpas_notify_ap_scan_changed(struct wpa_supplicant *wpa_s);
 void wpas_notify_bssid_changed(struct wpa_supplicant *wpa_s);
+void wpas_notify_auth_bssid_changed(struct wpa_supplicant *wpa_s);
 void wpas_notify_mac_address_changed(struct wpa_supplicant *wpa_s);
 void wpas_notify_auth_changed(struct wpa_supplicant *wpa_s);
 void wpas_notify_network_enabled_changed(struct wpa_supplicant *wpa_s,
diff --git a/wpa_supplicant/sme.c b/wpa_supplicant/sme.c
index a7ad5ecf4..8fb4e7b99 100644
--- a/wpa_supplicant/sme.c
+++ b/wpa_supplicant/sme.c
@@ -1142,6 +1142,8 @@  no_fils:
 	eapol_sm_notify_portValid(wpa_s->eapol, false);
 	wpa_clear_keys(wpa_s, bss->bssid);
 	wpa_supplicant_set_state(wpa_s, WPA_AUTHENTICATING);
+	wpa_s->auth_bss = bss;
+	wpas_notify_auth_bssid_changed(wpa_s);
 	if (old_ssid != wpa_s->current_ssid)
 		wpas_notify_network_changed(wpa_s);
 
diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c
index b9938d8dd..106f73286 100644
--- a/wpa_supplicant/wpa_supplicant.c
+++ b/wpa_supplicant/wpa_supplicant.c
@@ -1083,6 +1083,11 @@  void wpa_supplicant_set_state(struct wpa_supplicant *wpa_s,
 
 	if (wpa_s->wpa_state != old_state) {
 		wpas_notify_state_changed(wpa_s, wpa_s->wpa_state, old_state);
+		if (wpa_s->auth_bss && (wpa_s->wpa_state == WPA_DISCONNECTED ||
+		    wpa_s->wpa_state == WPA_ASSOCIATED)) {
+			wpa_s->auth_bss = NULL;
+			wpas_notify_auth_bssid_changed(wpa_s);
+		}
 
 		/*
 		 * Notify the P2P Device interface about a state change in one
@@ -4195,6 +4200,8 @@  static void wpas_start_assoc_cb(struct wpa_radio_work *work, int deinit)
 	wpa_s->current_ssid = ssid;
 
 	wpa_supplicant_set_state(wpa_s, WPA_ASSOCIATING);
+	wpa_s->auth_bss = bss;
+	wpas_notify_auth_bssid_changed(wpa_s);
 	if (bss) {
 		params.ssid = bss->ssid;
 		params.ssid_len = bss->ssid_len;
diff --git a/wpa_supplicant/wpa_supplicant_i.h b/wpa_supplicant/wpa_supplicant_i.h
index b90d92a69..8ff6e2a07 100644
--- a/wpa_supplicant/wpa_supplicant_i.h
+++ b/wpa_supplicant/wpa_supplicant_i.h
@@ -725,6 +725,7 @@  struct wpa_supplicant {
 			   * before this has been cleared */
 	struct wpa_ssid *current_ssid;
 	struct wpa_ssid *last_ssid;
+	struct wpa_bss *auth_bss;
 	struct wpa_bss *current_bss;
 	int ap_ies_from_associnfo;
 	unsigned int assoc_freq;