My apologies for such late notice, but I observed this only recently
and wanted to point it out.
if (!try_module_get(THIS_MODULE))
goto err;
@@ -101,33 +106,73 @@ static int cryptomgr_schedule_probe(struct crypto_larval *larval)
memcpy(param->template, name, len);
- name = p + 1;
- len = 0;
- for (p = name; *p; p++) {
- for (; isalnum(*p) || *p == '-' || *p == '_' || *p == '('; p++)
- ;
+ i = 0;
+ for (;;) {
+ int notnum = 0;
- if (*p != ')')
- goto err_free_param;
+ name = ++p;
+ len = 0;
+
+ for (; isalnum(*p) || *p == '-' || *p == '_'; p++)
+ notnum |= !isdigit(*p);
+
+ if (*p == '(') {
+ int recursion = 0;
+
+ for (;;) {
+ if (!*++p)
+ goto err_free_param;
+ if (*p == '(')
+ recursion++;
+ else if (*p == ')' && !recursion--)
Shouldn't p be incremented after this check? Otherwise, it will
still be pointing to ')' after breaking from this for-loop and we will
end up copying "hmac(sha1" instead of "hmac(sha1)", right?
Also, I think we will prematurely break from top for-loop
because of check, "if (*p == ')') break;" which is done further down...
+ break;
+ }
+
+ notnum = 1;
+ }
len = p - name;
+ if (!len)
+ goto err_free_param;
+
+ if (notnum) {
+ param->attrs[i].alg.attr.rta_len =
+ sizeof(param->attrs[i].alg);
+ param->attrs[i].alg.attr.rta_type = CRYPTOA_ALG;
+ memcpy(param->attrs[i].alg.data.name, name, len);
+ } else {
+ param->attrs[i].nu32.attr.rta_len =
+ sizeof(param->attrs[i].nu32);
+ param->attrs[i].nu32.attr.rta_type = CRYPTOA_U32;
+ param->attrs[i].nu32.data.num =
+ simple_strtol(name, NULL, 0);
+ }
+
+ param->tb[i + 1] = ¶m->attrs[i].attr;
+ i++;
+
+ if (WARN_ON(i >= CRYPTO_MAX_ATTRS))
+ goto err_free_param;
+
+ if (*p == ')')
+ break;
+
+ if (*p != ',')
+ goto err_free_param;
}
Regards,
Joy
Sorry, I really messed up my Subject header.
This comment was for [PATCH 2/8] [CRYPTO] aead: Add support for
multiple template parameters in the patchset.
The second patch in patch set, not the eighth.
Joy
On Thu, 2007-09-27 at 17:25 -0500, Joy Latten wrote:
> My apologies for such late notice, but I observed this only recently
> and wanted to point it out.
>
>
> if (!try_module_get(THIS_MODULE))
> goto err;
> @@ -101,33 +106,73 @@ static int cryptomgr_schedule_probe(struct crypto_larval *larval)
>
> memcpy(param->template, name, len);
>
> - name = p + 1;
> - len = 0;
> - for (p = name; *p; p++) {
> - for (; isalnum(*p) || *p == '-' || *p == '_' || *p == '('; p++)
> - ;
> + i = 0;
> + for (;;) {
> + int notnum = 0;
>
> - if (*p != ')')
> - goto err_free_param;
> + name = ++p;
> + len = 0;
> +
> + for (; isalnum(*p) || *p == '-' || *p == '_'; p++)
> + notnum |= !isdigit(*p);
> +
> + if (*p == '(') {
> + int recursion = 0;
> +
> + for (;;) {
> + if (!*++p)
> + goto err_free_param;
> + if (*p == '(')
> + recursion++;
> + else if (*p == ')' && !recursion--)
>
> Shouldn't p be incremented after this check? Otherwise, it will
> still be pointing to ')' after breaking from this for-loop and we will
> end up copying "hmac(sha1" instead of "hmac(sha1)", right?
> Also, I think we will prematurely break from top for-loop
> because of check, "if (*p == ')') break;" which is done further down...
>
> + break;
> + }
> +
> + notnum = 1;
> + }
>
> len = p - name;
> + if (!len)
> + goto err_free_param;
> +
> + if (notnum) {
> + param->attrs[i].alg.attr.rta_len =
> + sizeof(param->attrs[i].alg);
> + param->attrs[i].alg.attr.rta_type = CRYPTOA_ALG;
> + memcpy(param->attrs[i].alg.data.name, name, len);
> + } else {
> + param->attrs[i].nu32.attr.rta_len =
> + sizeof(param->attrs[i].nu32);
> + param->attrs[i].nu32.attr.rta_type = CRYPTOA_U32;
> + param->attrs[i].nu32.data.num =
> + simple_strtol(name, NULL, 0);
> + }
> +
> + param->tb[i + 1] = ¶m->attrs[i].attr;
> + i++;
> +
> + if (WARN_ON(i >= CRYPTO_MAX_ATTRS))
> + goto err_free_param;
> +
> + if (*p == ')')
> + break;
> +
> + if (*p != ',')
> + goto err_free_param;
> }
>
>
> Regards,
> Joy
> -
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 27, 2007 at 05:25:45PM -0500, Joy Latten wrote:
>
> Shouldn't p be incremented after this check? Otherwise, it will
> still be pointing to ')' after breaking from this for-loop and we will
> end up copying "hmac(sha1" instead of "hmac(sha1)", right?
> Also, I think we will prematurely break from top for-loop
> because of check, "if (*p == ')') break;" which is done further down...
Good catch, I've checked in the following patch.
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/crypto/cryptomgr.c b/crypto/cryptomgr.c
index c83884f..e5e3cf8 100644
--- a/crypto/cryptomgr.c
+++ b/crypto/cryptomgr.c
@@ -129,6 +129,7 @@ static int cryptomgr_schedule_probe(struct crypto_larval *larval)
}
notnum = 1;
+ p++;
}
len = p - name;
@@ -151,7 +152,7 @@ static int cryptomgr_schedule_probe(struct crypto_larval *larval)
param->tb[i + 1] = ¶m->attrs[i].attr;
i++;
- if (WARN_ON(i >= CRYPTO_MAX_ATTRS))
+ if (i >= CRYPTO_MAX_ATTRS)
goto err_free_param;
if (*p == ')')