[PATCH] In crypto_add_alg(), 'exact' wants to be initialized to 0

[PATCH] In crypto_add_alg(), 'exact' wants to be initialized to 0

[Jesper Juhl]

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'.
As far as I can tell, what we want is for 'exact' to be initialized to
0 (zero/false).

Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
 crypto/crypto_user.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

  Compile tested only.

diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
index 16f8693..36a2af7 100644
--- a/crypto/crypto_user.c
+++ b/crypto/crypto_user.c
@@ -304,7 +304,7 @@ 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;
+	int exact = 0;
 	const char *name;
 	struct crypto_alg *alg;
 	struct crypto_user_alg *p = nlmsg_data(nlh);
-- 
1.7.8.4


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

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

[devendra.aaru]

On Sun, Jan 29, 2012 at 5:39 PM, Jesper Juhl <jj@chaosbits.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 :).
> As far as I can tell, what we want is for 'exact' to be initialized to
> 0 (zero/false).
>
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> ---
>  crypto/crypto_user.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
>  Compile tested only.
>
> diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
> index 16f8693..36a2af7 100644
> --- a/crypto/crypto_user.c
> +++ b/crypto/crypto_user.c
> @@ -304,7 +304,7 @@ 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;
> +       int exact = 0;
>        const char *name;
>        struct crypto_alg *alg;
>        struct crypto_user_alg *p = nlmsg_data(nlh);
> --
> 1.7.8.4
>
>
> --
> Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
> Don't top-post http://www.catb.org/jargon/html/T/top-post.html
> Plain text mails only, please.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[Jesper Juhl]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4981 bytes --]

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

> On Sun, Jan 29, 2012 at 5:39 PM, Jesper Juhl <jj@chaosbits.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@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

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

[Jesper Juhl]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5485 bytes --]

On Wed, 1 Feb 2012, Jesper Juhl wrote:

> On Wed, 1 Feb 2012, devendra.aaru wrote:
> 
> > On Sun, Jan 29, 2012 at 5:39 PM, Jesper Juhl <jj@chaosbits.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) {

Actually, we could just get rid of 'match' entirely as well and just do

                if (!strcmp(q->cra_driver_name, p->cru_driver_name)) {

>  			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);
>  
> 

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

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

[Steffen Klassert]

On Wed, Feb 01, 2012 at 09:21:39PM +0100, Jesper Juhl wrote:
> On Wed, 1 Feb 2012, devendra.aaru wrote:
> 
> > On Sun, Jan 29, 2012 at 5:39 PM, Jesper Juhl <jj@chaosbits.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.
> 

We get cru_driver_name from a netlink message that a user sends us.
So it depends pretty much on the user whether cru_driver_name is
set or not. Usually it is set when a user wants to instantiate
a certain algorithm driver, like "cbc(aes-asm)". If the user just
wants to instantiate the system default of an algorithm, he can
set cru_name (e.g. to "cbc(aes)") instead of cru_driver_name.

Your first patch is correct.

Thanks,

Steffen

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

[Jesper Juhl]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1855 bytes --]

On Thu, 2 Feb 2012, Steffen Klassert wrote:

> On Wed, Feb 01, 2012 at 09:21:39PM +0100, Jesper Juhl wrote:
> > On Wed, 1 Feb 2012, devendra.aaru wrote:
> > 
> > > On Sun, Jan 29, 2012 at 5:39 PM, Jesper Juhl <jj@chaosbits.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.
> > 
> 
> We get cru_driver_name from a netlink message that a user sends us.
> So it depends pretty much on the user whether cru_driver_name is
> set or not. Usually it is set when a user wants to instantiate
> a certain algorithm driver, like "cbc(aes-asm)". If the user just
> wants to instantiate the system default of an algorithm, he can
> set cru_name (e.g. to "cbc(aes)") instead of cru_driver_name.
> 
> Your first patch is correct.
> 
Thank you for the explanation.

Can I take that to mean that I can add your Acked-by: if/when I resend the 
patch?

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

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

[Steffen Klassert]

On Thu, Feb 02, 2012 at 03:42:43PM +0100, Jesper Juhl wrote:
> On Thu, 2 Feb 2012, Steffen Klassert wrote:
> > 
> > Your first patch is correct.
> > 
> Thank you for the explanation.
> 
> Can I take that to mean that I can add your Acked-by: if/when I resend the 
> patch?
> 

Sure,

Acked-by: Steffen Klassert <steffen.klassert@secunet.com>

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

[Herbert Xu]

On Fri, Feb 03, 2012 at 07:24:25AM +0100, Steffen Klassert wrote:
> On Thu, Feb 02, 2012 at 03:42:43PM +0100, Jesper Juhl wrote:
> > On Thu, 2 Feb 2012, Steffen Klassert wrote:
> > > 
> > > Your first patch is correct.
> > > 
> > Thank you for the explanation.
> > 
> > Can I take that to mean that I can add your Acked-by: if/when I resend the 
> > patch?
> > 
> 
> Sure,
> 
> Acked-by: Steffen Klassert <steffen.klassert@secunet.com>

Patch applied.  Thanks a lot!
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt