Hi Tejus,
Replying to Andy's email in order to inline comment on the patch..
On Tue, 2014-05-27 at 10:55 -0700, Andy Grover wrote:
> On 05/27/2014 12:02 AM, Tejas Vaykole wrote:
> > Hi,
> >
> > Initially the LIO target was not able to handle CHAP_A in list
> > correctly.This patch handles it correctly.
> > Let me know if any changes are required.
>
> Hi Tejas,
>
> Please include patch inline next time is possible, instead of as an
> attachment.
>
Yes, please.
> +/*
> + * check_algorithm - Verifies CHAP_A string contains correct
> + * algorithm value. Presently only MD5 is
> + * supported.
> + *
> + * return values - CHAP_DIGEST_UNKNOWN for unknown value and
> + * CHAP_DIGEST_MD5 for MD5.
> + *
> + */
Please use the docbook style comments. See:
Documentation/kernel-doc-nano-HOWTO.txt
> +static int check_algorithm(const char *a_str)
> +{
> + char *tmp = NULL;
> + char *token = NULL;
> + tmp = kmalloc(strlen(a_str)+1, GFP_KERNEL);
Use kstrdup here..
> + if (tmp == NULL) {
> + pr_err("Memory allocation Failed\n");
> + return CHAP_DIGEST_UNKNOWN;
> + }
Use if (!foo) style conditionals..
> + strcpy(tmp, a_str);
> + token = strsep(&tmp , "=");
> + while (token != NULL) {
Drop unnecessary "!= NULL' for while (foo) {}
> + token = strsep(&tmp , ",");
> + if (token == NULL)
> + return CHAP_DIGEST_UNKNOWN;
Ditto here for if (!foo) style conditionals
> + if (!strcmp(token, "5")) {
> + pr_err("MD5 Algorithm\n");
> + return CHAP_DIGEST_MD5;
> + }
> + }
> + return CHAP_DIGEST_UNKNOWN;
> +}
>
> Use strncpy over strcpy.
> Free alloced memory, or else memory leak.
>
> +
> static struct iscsi_chap *chap_server_open(
> - struct iscsi_conn *conn,
> - struct iscsi_node_auth *auth,
> - const char *a_str,
> - char *aic_str,
> - unsigned int *aic_len)
> + struct iscsi_conn *conn,
> + struct iscsi_node_auth *auth,
> + const char *a_str,
> + char *aic_str,
> + unsigned int *aic_len)
> {
> + int ret;
> struct iscsi_chap *chap;
> -
> if (!(auth->naf_flags & NAF_USERID_SET) ||
> - !(auth->naf_flags & NAF_PASSWORD_SET)) {
>
> Your editor has modified some whitespace. Don't include whitespace fixes
> in substantive patches please.
>
> Also, I'm sure I'm missing something, but how exactly does this fix the
> issue you described in your previous email? The current code in
> chap_server_open returns NULL if not "CHAP_A=5", which results in
> "security negotiation failed", doesn't it? And the new code doesn't
> change this behavior as far as I can see.
>
Yes, always include a detailed commit log of what bug the patch
addresses, along with an explanation of why it's correct..
(Adding more of the original patch that did not appear in Andy's
response)
@@ -93,34 +126,35 @@ static struct iscsi_chap *chap_server_open(
return NULL;
chap = conn->auth_protocol;
- /*
- * We only support MD5 MDA presently.
- */
- if (strncmp(a_str, "CHAP_A=5", 8)) {
- pr_err("CHAP_A is not MD5.\n");
+ ret = check_algorithm(a_str);
+ switch (ret) {
+ case CHAP_DIGEST_MD5:
+ pr_debug("[server] Got CHAP_A=5\n");
+ /*
+ * Send back CHAP_A set to MD5.
+ */
+ *aic_len = sprintf(aic_str, "CHAP_A=5");
+ *aic_len += 1;
+ chap->digest_type = CHAP_DIGEST_MD5;
+ pr_debug("[server] Sending CHAP_A=%d\n", chap->digest_type);
+ /*
+ * Set Identifier.
+ */
+ chap->id = conn->tpg->tpg_chap_id++;
+ *aic_len += sprintf(aic_str + *aic_len, "CHAP_I=%d", chap->id);
+ *aic_len += 1;
+ pr_debug("[server] Sending CHAP_I=%d\n", chap->id);
+ /*
+ * Generate Challenge.
+ */
+ chap_gen_challenge(conn, 1, aic_str, aic_len);
+ return chap;
+ break;
+ case CHAP_DIGEST_UNKNOWN:
+ default:
+ pr_err("Unknown CHAP_A.\n");
return NULL;
}
- pr_debug("[server] Got CHAP_A=5\n");
- /*
- * Send back CHAP_A set to MD5.
- */
- *aic_len = sprintf(aic_str, "CHAP_A=5");
- *aic_len += 1;
- chap->digest_type = CHAP_DIGEST_MD5;
- pr_debug("[server] Sending CHAP_A=%d\n", chap->digest_type);
- /*
- * Set Identifier.
- */
- chap->id = conn->tpg->tpg_chap_id++;
- *aic_len += sprintf(aic_str + *aic_len, "CHAP_I=%d", chap->id);
- *aic_len += 1;
- pr_debug("[server] Sending CHAP_I=%d\n", chap->id);
- /*
- * Generate Challenge.
- */
- chap_gen_challenge(conn, 1, aic_str, aic_len);
-
- return chap;
}
The generation of the CHAP_A + CHAP_I + CHAP_C values for the login
response are independent of the actual algorithm selected. That said,
this code should be common to all algorithms, and not specific to
CHAP_DIGEST_MD5 to avoid duplication if/when another algorithm is ever
supported.
diff --git a/drivers/target/iscsi/iscsi_target_auth.h b/drivers/target/iscsi/iscsi_target_auth.h
index 2f463c0..97b4527 100644
--- a/drivers/target/iscsi/iscsi_target_auth.h
+++ b/drivers/target/iscsi/iscsi_target_auth.h
@@ -1,10 +1,11 @@
#ifndef _ISCSI_CHAP_H_
#define _ISCSI_CHAP_H_
+#define CHAP_DIGEST_UNKNOWN 0
#define CHAP_DIGEST_MD5 5
#define CHAP_DIGEST_SHA 6
-#define CHAP_CHALLENGE_LENGTH 16
+#define CHAP_CHALLENGE_LENGTH 16
Ditto here, avoid random whitespace changes in patches that fix bugs.
--nab
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
