Re: [PATCH v4 1/3] station: network: rework ERP/FILS code path

Re: [PATCH v4 1/3] station: network: rework ERP/FILS code path

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

Hi James,

On 8/3/21 4:40 PM, James Prestwood wrote:
> This refactors some code to eliminate getting the ERP entry twice
> by simply returning it from network_has_erp_identity (now renamed
> to network_get_erp_cache). In addition this code was moved into
> station_build_handshake_rsn and properly cleaned up in case there
> was an error or if a FILS AKM was not chosen.
> ---
>   src/network.c | 16 +++++++++-------
>   src/network.h |  3 ++-
>   src/station.c | 33 +++++++++++++++++++--------------
>   3 files changed, 30 insertions(+), 22 deletions(-)
> 
> v4:
>   * Called erp_cache_put if cache entry needs to be removed
> 

Ok, all applied and I fixed up the linker-errors that resulted from these changes.

Regards,
-Denis

[PATCH v4 1/3] station: network: rework ERP/FILS code path

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 5181 bytes --]

This refactors some code to eliminate getting the ERP entry twice
by simply returning it from network_has_erp_identity (now renamed
to network_get_erp_cache). In addition this code was moved into
station_build_handshake_rsn and properly cleaned up in case there
was an error or if a FILS AKM was not chosen.
---
 src/network.c | 16 +++++++++-------
 src/network.h |  3 ++-
 src/station.c | 33 +++++++++++++++++++--------------
 3 files changed, 30 insertions(+), 22 deletions(-)

v4:
 * Called erp_cache_put if cache entry needs to be removed

diff --git a/src/network.c b/src/network.c
index b78a7cbf..4f3855e8 100644
--- a/src/network.c
+++ b/src/network.c
@@ -1056,7 +1056,7 @@ static bool match_bss(const void *a, const void *b)
 	return a == b;
 }
 
-bool network_has_erp_identity(struct network *network)
+struct erp_cache_entry *network_get_erp_cache(struct network *network)
 {
 	struct erp_cache_entry *cache;
 	struct l_settings *settings;
@@ -1066,16 +1066,16 @@ bool network_has_erp_identity(struct network *network)
 
 	settings = network_get_settings(network);
 	if (!settings)
-		return false;
+		return NULL;
 
 	check_id = l_settings_get_string(settings, "Security", "EAP-Identity");
 	if (!check_id)
-		return false;
+		return NULL;
 
 	cache = erp_cache_get(network_get_ssid(network));
 	if (!cache) {
 		l_free(check_id);
-		return false;
+		return NULL;
 	}
 
 	identity = erp_cache_entry_get_identity(cache);
@@ -1083,17 +1083,19 @@ bool network_has_erp_identity(struct network *network)
 	ret = strcmp(check_id, identity) == 0;
 
 	l_free(check_id);
-	erp_cache_put(cache);
 
 	/*
 	 * The settings file must have change out from under us. In this
 	 * case we want to remove the ERP entry because it is no longer
 	 * valid.
 	 */
-	if (!ret)
+	if (!ret) {
+		erp_cache_put(cache);
 		erp_cache_remove(identity);
+		return NULL;
+	}
 
-	return ret;
+	return cache;
 }
 
 const struct l_queue_entry *network_bss_list_get_entries(
diff --git a/src/network.h b/src/network.h
index 58c26e6b..2cca7ffb 100644
--- a/src/network.h
+++ b/src/network.h
@@ -29,6 +29,7 @@ struct station;
 struct network;
 struct scan_bss;
 struct handshake_state;
+struct erp_cache_entry;
 
 void network_connected(struct network *network);
 void network_disconnected(struct network *network);
@@ -87,7 +88,7 @@ void network_blacklist_add(struct network *network, struct scan_bss *bss);
 const struct iovec *network_get_extra_ies(struct network *network,
 						size_t *num_elems);
 
-bool network_has_erp_identity(struct network *network);
+struct erp_cache_entry *network_get_erp_cache(struct network *network);
 
 const struct l_queue_entry *network_bss_list_get_entries(
 						struct network *network);
diff --git a/src/station.c b/src/station.c
index e914abb3..5414283c 100644
--- a/src/station.c
+++ b/src/station.c
@@ -743,7 +743,7 @@ static int station_build_handshake_rsn(struct handshake_state *hs,
 {
 	enum security security = network_get_security(network);
 	bool add_mde = false;
-	bool fils_hint = false;
+	struct erp_cache_entry *erp_cache = NULL;
 
 	struct ie_rsn_info bss_info;
 	uint8_t rsne_buf[256];
@@ -764,10 +764,10 @@ static int station_build_handshake_rsn(struct handshake_state *hs,
 	 * wiphy may select FILS if supported by the AP.
 	 */
 	if (security == SECURITY_8021X && hs->support_fils)
-		fils_hint = network_has_erp_identity(network);
+		erp_cache = network_get_erp_cache(network);
 
 	info.akm_suites = wiphy_select_akm(wiphy, bss, security,
-							&bss_info, fils_hint);
+						&bss_info, erp_cache != NULL);
 
 	/*
 	 * Special case for OWE. With OWE we still need to build up the
@@ -848,6 +848,19 @@ build_ie:
 	if (IE_AKM_IS_FT(info.akm_suites))
 		add_mde = true;
 
+	/*
+	 * If FILS was chosen, the ERP cache has been verified to exist. Take
+	 * a reference now so it remains valid (in case of expiration) until
+	 * FILS starts.
+	 */
+	if (hs->akm_suite & (IE_RSN_AKM_SUITE_FILS_SHA256 |
+				IE_RSN_AKM_SUITE_FILS_SHA384 |
+				IE_RSN_AKM_SUITE_FT_OVER_FILS_SHA256 |
+				IE_RSN_AKM_SUITE_FT_OVER_FILS_SHA384))
+		hs->erp_cache = erp_cache;
+	else if (erp_cache)
+		erp_cache_put(erp_cache);
+
 open_network:
 	if (security == SECURITY_NONE)
 		/* Perform FT association if available */
@@ -867,6 +880,9 @@ open_network:
 	return 0;
 
 not_supported:
+	if (erp_cache)
+		erp_cache_put(erp_cache);
+
 	return -ENOTSUP;
 }
 
@@ -889,17 +905,6 @@ static struct handshake_state *station_handshake_setup(struct station *station,
 	if (network_handshake_setup(network, hs) < 0)
 		goto not_supported;
 
-	/*
-	 * If FILS was chosen, the ERP cache has been verified to exist. We
-	 * wait to get it until here because@this point so there are no
-	 * failure paths before fils_sm_new
-	 */
-	if (hs->akm_suite & (IE_RSN_AKM_SUITE_FILS_SHA256 |
-				IE_RSN_AKM_SUITE_FILS_SHA384 |
-				IE_RSN_AKM_SUITE_FT_OVER_FILS_SHA256 |
-				IE_RSN_AKM_SUITE_FT_OVER_FILS_SHA384))
-		hs->erp_cache = erp_cache_get(network_get_ssid(network));
-
 	return hs;
 
 not_supported:
-- 
2.31.1

[PATCH v4 2/3] handshake: unref erp_cache when handshake is freed

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 765 bytes --]

This makes the erp_cache ownership more consisten rather than
relying on the ERP state to free the cache.
---
 src/handshake.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/handshake.c b/src/handshake.c
index 2bce6b8c..5f6b25f4 100644
--- a/src/handshake.c
+++ b/src/handshake.c
@@ -41,6 +41,7 @@
 #include "src/ie.h"
 #include "src/util.h"
 #include "src/handshake.h"
+#include "src/erp.h"
 
 static inline unsigned int n_ecc_groups()
 {
@@ -106,6 +107,9 @@ void handshake_state_free(struct handshake_state *s)
 	l_free(s->mde);
 	l_free(s->fte);
 
+	if (s->erp_cache)
+		erp_cache_put(s->erp_cache);
+
 	if (s->passphrase) {
 		explicit_bzero(s->passphrase, strlen(s->passphrase));
 		l_free(s->passphrase);
-- 
2.31.1

[PATCH v4 3/3] erp: take cache ref in erp_new

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 521 bytes --]

Since the erp_state is holding a pointer to the ERP cache, as
well as calling erp_cache_put on free, it should take a reference
for symmetry.
---
 src/erp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/erp.c b/src/erp.c
index 4db32feb..59e6033c 100644
--- a/src/erp.c
+++ b/src/erp.c
@@ -336,6 +336,7 @@ struct erp_state *erp_new(struct erp_cache_entry *cache,
 	erp->tx_packet = tx_packet;
 	erp->user_data = user_data;
 	erp->cache = cache;
+	cache->ref++;
 
 	return erp;
 }
-- 
2.31.1