Patchwork Skip PSK re-calculation when psk/ssid is not modified

login
register
mail settings
Submitter Masashi Honma
Date Oct. 28, 2013, 7:18 a.m.
Message ID <CAFk-A4n9WyFC4hhTiAQaxZ6ct5QvWdL3rSyKKrB64YU0ysRHAg@mail.gmail.com>
Download mbox | patch
Permalink /patch/286404/
State Changes Requested
Headers show

Comments

Masashi Honma - Oct. 28, 2013, 7:18 a.m.
Signed-hostap: Masashi Honma <masashi.honma@gmail.com>



Regards,
Masashi Honma.
Jouni Malinen - Nov. 9, 2013, 4:34 p.m.
On Mon, Oct 28, 2013 at 04:18:31PM +0900, Masashi Honma wrote:
> diff --git a/wpa_supplicant/ctrl_iface.c b/wpa_supplicant/ctrl_iface.c

> +    if (os_strcmp(name, "psk") == 0 && value[0] == '"' && ssid->ssid_len) {
> +        if (ssid->passphrase) {
> +            if (os_strlen(ssid->passphrase) !=
> +                os_strlen(value) - 2 ||
> +                os_memcmp(ssid->passphrase, &value[1],
> +                os_strlen(ssid->passphrase)) != 0)
> +                update_psk = 1;
> +        } else {
> +            update_psk = 1;
> +        }
> +    }
...

> -    if ((os_strcmp(name, "psk") == 0 &&
> -         value[0] == '"' && ssid->ssid_len) ||
> -        (os_strcmp(name, "ssid") == 0 && ssid->passphrase))
> +    if (update_psk)
>          wpa_config_update_psk(ssid);


Could you please clarify what is the use case that this is addressing?
If I understood the implementation correctly, this skips
wpa_config_update_psk() call in case someone tries to set psk or ssid to
the existing value of the field. Why would such corner cases need to be
handled? Wouldn't it be better to fix whatever is sending such pointless
commands to not do that? In other words, I would need to get quite a bit
better justification for this type of extra complexity in
wpa_supplicant.


PS.

The patch was severely whitespace damaged and as such, would require
manual editing from me to get it applied..
Masashi Honma - Nov. 11, 2013, 1:12 a.m.
2013/11/10 Jouni Malinen <j@w1.fi>:

> Could you please clarify what is the use case that this is addressing?

This patch intends to skip PSK re-calculation.
Because tiny device requires long time for PSK re-calculation.

> Wouldn't it be better to fix whatever is sending such pointless commands to
> not do that?

As you say, our Android application can save PSK and check modification.
But Android settings application can change the PSK also.
Then our application can not recoginze the modification.
And application can not get current PSK from wpa_supplicant via wpa_cli.

So we want to check it on wpa_supplicant.

> The patch was severely whitespace damaged

I don't know why it occured...
It is maybe mailer problem.
I will send the patch as attachment.

Regards,
Masashi Honma.

Patch

diff --git a/wpa_supplicant/ctrl_iface.c b/wpa_supplicant/ctrl_iface.c
index d0c0a01..153611a 100644
--- a/wpa_supplicant/ctrl_iface.c
+++ b/wpa_supplicant/ctrl_iface.c
@@ -2332,9 +2332,11 @@  static int wpa_supplicant_ctrl_iface_remove_network(
 static int wpa_supplicant_ctrl_iface_set_network(
     struct wpa_supplicant *wpa_s, char *cmd)
 {
-    int id;
+    int id, update_psk = 0;
     struct wpa_ssid *ssid;
     char *name, *value;
+    u8 *old_ssid = NULL;
+    size_t old_ssid_len = 0;

     /* cmd: "<network id> <variable name> <value>" */
     name = os_strchr(cmd, ' ');
@@ -2360,12 +2362,44 @@  static int wpa_supplicant_ctrl_iface_set_network(
         return -1;
     }

+    if (os_strcmp(name, "psk") == 0 && value[0] == '"' && ssid->ssid_len) {
+        if (ssid->passphrase) {
+            if (os_strlen(ssid->passphrase) !=
+                os_strlen(value) - 2 ||
+                os_memcmp(ssid->passphrase, &value[1],
+                os_strlen(ssid->passphrase)) != 0)
+                update_psk = 1;
+        } else {
+            update_psk = 1;
+        }
+    }
+
+    if (os_strcmp(name, "ssid") == 0 && ssid->passphrase) {
+        if (ssid->ssid_len) {
+            old_ssid = os_malloc(ssid->ssid_len);
+            if (old_ssid == NULL)
+                return -1;
+            os_memcpy(old_ssid, ssid->ssid, ssid->ssid_len);
+            old_ssid_len = ssid->ssid_len;
+        } else {
+            update_psk = 1;
+        }
+    }
+
     if (wpa_config_set(ssid, name, value, 0) < 0) {
         wpa_printf(MSG_DEBUG, "CTRL_IFACE: Failed to set network "
                "variable '%s'", name);
+        os_free(old_ssid);
         return -1;
     }

+    if (old_ssid) {
+        if (old_ssid_len != ssid->ssid_len ||
+            os_memcmp(old_ssid, ssid->ssid, ssid->ssid_len) != 0)
+            update_psk = 1;
+        os_free(old_ssid);
+    }
+
     if (os_strcmp(name, "bssid") != 0 &&
         os_strcmp(name, "priority") != 0)
         wpa_sm_pmksa_cache_flush(wpa_s->wpa, ssid);
@@ -2378,9 +2412,7 @@  static int wpa_supplicant_ctrl_iface_set_network(
         eapol_sm_invalidate_cached_session(wpa_s->eapol);
     }

-    if ((os_strcmp(name, "psk") == 0 &&
-         value[0] == '"' && ssid->ssid_len) ||
-        (os_strcmp(name, "ssid") == 0 && ssid->passphrase))
+    if (update_psk)
         wpa_config_update_psk(ssid);
     else if (os_strcmp(name, "priority") == 0)
         wpa_config_update_prio_list(wpa_s->conf);