| 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
| ||
|
Date: Wed, 1 Feb 2012 21:36:13 +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, Jesper Juhl wrote:
> 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) {
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@...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.
Powered by blists - more mailing lists