linux-kernel - Re: [PATCH] In crypto_add_alg(), 'exact' wants to be initialized to 0

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 1 Feb 2012 21:21:39 +0100 (CET)
From:	Jesper Juhl <jj@...osbits.net>
To:	"devendra.aaru" <devendra.aaru@...il.com>
cc:	Herbert Xu <herbert@...dor.apana.org.au>,
	"David S. Miller" <davem@...emloft.net>,
	linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org,
	Steffen Klassert <steffen.klassert@...unet.com>
Subject: Re: [PATCH] In crypto_add_alg(), 'exact' wants to be initialized to
 0

On Wed, 1 Feb 2012, devendra.aaru wrote:

> On Sun, Jan 29, 2012 at 5:39 PM, Jesper Juhl <jj@...osbits.net> wrote:
> > We declare 'exact' without initializing it and then do:
> >
> > [...]
> >        if (strlen(p->cru_driver_name))
> >                exact = 1;
> >
> >        if (priority && !exact)
> >                return -EINVAL;
> >
> > [...]
> >
> > If the first 'if' is not true, then the second will test an
> > uninitialized 'exact'.
> 
> not needed . as the cru_driver_name will always be present :).

If that is indeed the case, and we are guaranteed that, then it would seem 
that a patch like the following would be what we want instead??

Please note that this patch is intended just for discussion, nothing else 
(which is why I left out a Signed-off-by on purpose), since I've not 
tested it beyond checking that it compiles, nor have I verified your claim 
that cru_driver_name will always be present.

Herbert, David, any input?


Subject: [PATCH] Simplify crypto_add_alg() and crypto_alg_match()

Since cru_driver_name will always be present, the test

	if (strlen(p->cru_driver_name))
		exact = 1;

will always be true. So there's no need to have the test at all, we
can just unconditionally assign 1 to 'exact'.

And if 'exact' is always 1, then we'll never take the true branch of

	if (priority && !exact)
		return -EINVAL;

so we may as well just remove those two lines completely.

At this point we may as well just remove 'exact' entirely and just use
a hardcoded 1 in the call to crypto_alg_match().

The 'name' variable is also entirely redundant since with
cru_driver_name always being present we'll always take the first
branch in

	if (strlen(p->cru_driver_name))
		name = p->cru_driver_name;
	else
		name = p->cru_name;

and then we may as well just use 'p->cru_driver_name' itself the one
place where it is needed in the call to crypto_alg_mod_lookup().

At this point all calls to the static crypto_alg crypto_alg_match()
function pass 1 as the second argument, so there's not really any
point any more in that function having that argument, so let's remove
it and the code that tests it.

And since strlen(p->cru_driver_name) will also always be true in that
function, we can simplify the assignment to 'match'.

---
 crypto/crypto_user.c |   33 ++++++++-------------------------
 1 files changed, 8 insertions(+), 25 deletions(-)

diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
index 16f8693..7760c22 100644
--- a/crypto/crypto_user.c
+++ b/crypto/crypto_user.c
@@ -38,23 +38,19 @@ struct crypto_dump_info {
 	u16 nlmsg_flags;
 };
 
-static struct crypto_alg *crypto_alg_match(struct crypto_user_alg *p, int exact)
+static struct crypto_alg *crypto_alg_match(struct crypto_user_alg *p)
 {
 	struct crypto_alg *q, *alg = NULL;
 
 	down_read(&crypto_alg_sem);
 
 	list_for_each_entry(q, &crypto_alg_list, cra_list) {
-		int match = 0;
+		int match;
 
 		if ((q->cra_flags ^ p->cru_type) & p->cru_mask)
 			continue;
 
-		if (strlen(p->cru_driver_name))
-			match = !strcmp(q->cra_driver_name,
-					p->cru_driver_name);
-		else if (!exact)
-			match = !strcmp(q->cra_name, p->cru_name);
+		match = !strcmp(q->cra_driver_name, p->cru_driver_name);
 
 		if (match) {
 			alg = q;
@@ -195,7 +191,7 @@ static int crypto_report(struct sk_buff *in_skb, struct nlmsghdr *in_nlh,
 	if (!p->cru_driver_name)
 		return -EINVAL;
 
-	alg = crypto_alg_match(p, 1);
+	alg = crypto_alg_match(p);
 	if (!alg)
 		return -ENOENT;
 
@@ -259,7 +255,7 @@ static int crypto_update_alg(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (priority && !strlen(p->cru_driver_name))
 		return -EINVAL;
 
-	alg = crypto_alg_match(p, 1);
+	alg = crypto_alg_match(p);
 	if (!alg)
 		return -ENOENT;
 
@@ -283,7 +279,7 @@ static int crypto_del_alg(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct crypto_alg *alg;
 	struct crypto_user_alg *p = nlmsg_data(nlh);
 
-	alg = crypto_alg_match(p, 1);
+	alg = crypto_alg_match(p);
 	if (!alg)
 		return -ENOENT;
 
@@ -304,28 +300,15 @@ static int crypto_del_alg(struct sk_buff *skb, struct nlmsghdr *nlh,
 static int crypto_add_alg(struct sk_buff *skb, struct nlmsghdr *nlh,
 			  struct nlattr **attrs)
 {
-	int exact;
-	const char *name;
 	struct crypto_alg *alg;
 	struct crypto_user_alg *p = nlmsg_data(nlh);
 	struct nlattr *priority = attrs[CRYPTOCFGA_PRIORITY_VAL];
 
-	if (strlen(p->cru_driver_name))
-		exact = 1;
-
-	if (priority && !exact)
-		return -EINVAL;
-
-	alg = crypto_alg_match(p, exact);
+	alg = crypto_alg_match(p);
 	if (alg)
 		return -EEXIST;
 
-	if (strlen(p->cru_driver_name))
-		name = p->cru_driver_name;
-	else
-		name = p->cru_name;
-
-	alg = crypto_alg_mod_lookup(name, p->cru_type, p->cru_mask);
+	alg = crypto_alg_mod_lookup(p->cru_driver_name, p->cru_type, p->cru_mask);
 	if (IS_ERR(alg))
 		return PTR_ERR(alg);
 
-- 
1.7.9


-- 
Jesper Juhl <jj@...osbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.