2016-01-27 09:08:34

by Rui Wang

[permalink] [raw]
Subject: [PATCH v2 0/4] Fix sha1_mb failure and testing import()/export()

Hi All,

This patchset resulted from the failure when loading sha1_mb. It
is because ahash drivers are now required to implement import()
and export(). Also, now it seems beneficial to add a test case in
testmgr to test import()/export(), thus:

patch01 - patch03 fix the problems while loading sha1_mb.
patch04 adds a test case for import() and export(). A hash algo's
import()/export() can be tested by simply adding .partial = 1 to
its corresponding struct hash_testvec where .np > 1.

v2: Leverage template[i].np in the test case as suggested by Tim Chen.

Rui Wang (4):
crypto x86/sha1_mb: Fix load failure
crypto: mcryptd - Fix load failure
crypto: algif_hash - wait for crypto_ahash_init() to complete
crypto: testmgr - Add a test case for import()/export()

arch/x86/crypto/sha-mb/sha1_mb.c | 39 +++++++++++
crypto/algif_hash.c | 4 +-
crypto/mcryptd.c | 1 +
crypto/testmgr.c | 136 +++++++++++++++++++++++++++++++++++++++
crypto/testmgr.h | 4 +-
5 files changed, 182 insertions(+), 2 deletions(-)

--
1.8.3.1


2016-01-27 09:08:35

by Rui Wang

[permalink] [raw]
Subject: [PATCH v2 1/4] crypto x86/sha1_mb: Fix load failure

modprobe sha1_mb fails with the following message:

modprobe: ERROR: could not insert 'sha1_mb': No such device

It is because it needs to set its statesize and implement its
import() and export() interface.

Signed-off-by: Rui Wang <[email protected]>
---
arch/x86/crypto/sha-mb/sha1_mb.c | 39 +++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)

diff --git a/arch/x86/crypto/sha-mb/sha1_mb.c b/arch/x86/crypto/sha-mb/sha1_mb.c
index a841e97..deb6e81 100644
--- a/arch/x86/crypto/sha-mb/sha1_mb.c
+++ b/arch/x86/crypto/sha-mb/sha1_mb.c
@@ -762,6 +762,42 @@ static int sha1_mb_async_digest(struct ahash_request *req)
return crypto_ahash_digest(mcryptd_req);
}

+static int sha1_mb_async_export(struct ahash_request *req, void *out)
+{
+ struct ahash_request *mcryptd_req = ahash_request_ctx(req);
+ struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
+ struct sha1_mb_ctx *ctx = crypto_ahash_ctx(tfm);
+ struct mcryptd_ahash *mcryptd_tfm = ctx->mcryptd_tfm;
+
+ memcpy(mcryptd_req, req, sizeof(*req));
+ ahash_request_set_tfm(mcryptd_req, &mcryptd_tfm->base);
+ return crypto_ahash_export(mcryptd_req, out);
+}
+
+static int sha1_mb_async_import(struct ahash_request *req, const void *in)
+{
+ struct ahash_request *mcryptd_req = ahash_request_ctx(req);
+ struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
+ struct sha1_mb_ctx *ctx = crypto_ahash_ctx(tfm);
+ struct mcryptd_ahash *mcryptd_tfm = ctx->mcryptd_tfm;
+ struct crypto_shash *child = mcryptd_ahash_child(mcryptd_tfm);
+ struct mcryptd_hash_request_ctx *rctx;
+ struct shash_desc *desc;
+ int err;
+
+ memcpy(mcryptd_req, req, sizeof(*req));
+ ahash_request_set_tfm(mcryptd_req, &mcryptd_tfm->base);
+ rctx = ahash_request_ctx(mcryptd_req);
+ desc = &rctx->desc;
+ desc->tfm = child;
+ desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
+
+ err = crypto_shash_init(desc);
+ if (err)
+ return err;
+ return crypto_ahash_import(mcryptd_req, in);
+}
+
static int sha1_mb_async_init_tfm(struct crypto_tfm *tfm)
{
struct mcryptd_ahash *mcryptd_tfm;
@@ -796,8 +832,11 @@ static struct ahash_alg sha1_mb_async_alg = {
.final = sha1_mb_async_final,
.finup = sha1_mb_async_finup,
.digest = sha1_mb_async_digest,
+ .export = sha1_mb_async_export,
+ .import = sha1_mb_async_import,
.halg = {
.digestsize = SHA1_DIGEST_SIZE,
+ .statesize = sizeof(struct sha1_hash_ctx),
.base = {
.cra_name = "sha1",
.cra_driver_name = "sha1_mb",
--
1.8.3.1

2016-01-27 09:08:37

by Rui Wang

[permalink] [raw]
Subject: [PATCH v2 3/4] crypto: algif_hash - wait for crypto_ahash_init() to complete

hash_sendmsg/sendpage() need to wait for the completion
of crypto_ahash_init() otherwise it can cause panic.

Signed-off-by: Rui Wang <[email protected]>
---
crypto/algif_hash.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
index b4c24fe..83dc095 100644
--- a/crypto/algif_hash.c
+++ b/crypto/algif_hash.c
@@ -49,7 +49,8 @@ static int hash_sendmsg(struct socket *sock, struct msghdr *msg,

lock_sock(sk);
if (!ctx->more) {
- err = crypto_ahash_init(&ctx->req);
+ err = af_alg_wait_for_completion(crypto_ahash_init(&ctx->req),
+ &ctx->completion);
if (err)
goto unlock;
}
@@ -120,6 +121,7 @@ static ssize_t hash_sendpage(struct socket *sock, struct page *page,
} else {
if (!ctx->more) {
err = crypto_ahash_init(&ctx->req);
+ err = af_alg_wait_for_completion(err, &ctx->completion);
if (err)
goto unlock;
}
--
1.8.3.1

2016-01-27 09:28:48

by Rui Wang

[permalink] [raw]
Subject: [PATCH v2 4/4] crypto: testmgr - Add a test case for import()/export()

Modify __test_hash() so that hash import/export can be tested
from within the kernel by simply adding .partial = 1 to a hash
algo's struct hash_testvec where .np > 1.

v2: Leverage template[i].np as suggested by Tim Chen

Signed-off-by: Rui Wang <[email protected]>
---
crypto/testmgr.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
crypto/testmgr.h | 4 +-
2 files changed, 140 insertions(+), 1 deletion(-)

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index ae8c57fd..3d0c65a 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -198,6 +198,62 @@ static int wait_async_op(struct tcrypt_result *tr, int ret)
return ret;
}

+static int ahash_partial_update(struct ahash_request **preq,
+ struct crypto_ahash *tfm, struct hash_testvec *template,
+ void *hash_buff, int k, int temp, struct scatterlist *sg,
+ const char *algo, char *result, struct tcrypt_result *tresult)
+{
+ char *state;
+ struct ahash_request *req;
+ int statesize, ret = -EINVAL;
+
+ req = *preq;
+ statesize = crypto_ahash_statesize(
+ crypto_ahash_reqtfm(req));
+ state = kmalloc(statesize, GFP_KERNEL);
+ if (!state) {
+ pr_err("alt: hash: Failed to alloc state for %s\n", algo);
+ ahash_request_free(req);
+ goto out_nostate;
+ }
+ ret = crypto_ahash_export(req, state);
+ if (ret) {
+ pr_err("alt: hash: Failed to export() for %s\n", algo);
+ goto out;
+ }
+ ahash_request_free(req);
+ req = ahash_request_alloc(tfm, GFP_KERNEL);
+ if (!req) {
+ pr_err("alg: hash: Failed to alloc request for %s\n", algo);
+ goto out_noreq;
+ }
+ ahash_request_set_callback(req,
+ CRYPTO_TFM_REQ_MAY_BACKLOG,
+ tcrypt_complete, tresult);
+
+ memcpy(hash_buff, template->plaintext + temp,
+ template->tap[k]);
+ sg_init_one(&sg[0], hash_buff, template->tap[k]);
+ ahash_request_set_crypt(req, sg, result, template->tap[k]);
+ ret = crypto_ahash_import(req, state);
+ if (ret) {
+ pr_err("alg: hash: Failed to import() for %s\n", algo);
+ goto out;
+ }
+ ret = wait_async_op(tresult, crypto_ahash_update(req));
+ if (ret)
+ goto out;
+ *preq = req;
+ ret = 0;
+ goto out_noreq;
+out:
+ ahash_request_free(req);
+out_noreq:
+ kfree(state);
+out_nostate:
+ return ret;
+}
+
static int __test_hash(struct crypto_ahash *tfm, struct hash_testvec *template,
unsigned int tcount, bool use_digest,
const int align_offset)
@@ -385,6 +441,87 @@ static int __test_hash(struct crypto_ahash *tfm, struct hash_testvec *template,
}
}

+ /* partial update exercise */
+ j = 0;
+ for (i = 0; i < tcount; i++) {
+ /* alignment tests are only done with continuous buffers */
+ if (align_offset != 0)
+ break;
+
+ if (template[i].np < 2)
+ continue;
+
+ if (!template[i].partial)
+ continue;
+
+ j++;
+ memset(result, 0, MAX_DIGEST_SIZE);
+
+ ret = -EINVAL;
+ hash_buff = xbuf[0];
+ memcpy(hash_buff, template[i].plaintext,
+ template[i].tap[0]);
+ sg_init_one(&sg[0], hash_buff, template[i].tap[0]);
+
+ if (template[i].ksize) {
+ crypto_ahash_clear_flags(tfm, ~0);
+ if (template[i].ksize > MAX_KEYLEN) {
+ pr_err("alg: hash: setkey failed on test %d for %s: key size %d > %d\n",
+ j, algo, template[i].ksize, MAX_KEYLEN);
+ ret = -EINVAL;
+ goto out;
+ }
+ memcpy(key, template[i].key, template[i].ksize);
+ ret = crypto_ahash_setkey(tfm, key, template[i].ksize);
+ if (ret) {
+ pr_err("alg: hash: setkey failed on test %d for %s: ret=%d\n",
+ j, algo, -ret);
+ goto out;
+ }
+ }
+
+ ahash_request_set_crypt(req, sg, result, template[i].tap[0]);
+ ret = wait_async_op(&tresult, crypto_ahash_init(req));
+ if (ret) {
+ pr_err("alt: hash: init failed on test %d for %s: ret=%d\n",
+ j, algo, -ret);
+ goto out;
+ }
+ ret = wait_async_op(&tresult, crypto_ahash_update(req));
+ if (ret) {
+ pr_err("alt: hash: update failed on test %d for %s: ret=%d\n",
+ j, algo, -ret);
+ goto out;
+ }
+
+ temp = template[i].tap[0];
+ for (k = 1; k < template[i].np; k++) {
+ ret = ahash_partial_update(&req, tfm, &template[i],
+ hash_buff, k, temp, &sg[0], algo, result,
+ &tresult);
+ if (ret) {
+ pr_err("hash: partial update failed on test %d for %s: ret=%d\n",
+ j, algo, -ret);
+ goto out_noreq;
+ }
+ temp += template[i].tap[k];
+ }
+ ret = wait_async_op(&tresult, crypto_ahash_final(req));
+ if (ret) {
+ pr_err("alt: hash: final failed on test %d for %s: ret=%d\n",
+ j, algo, -ret);
+ goto out;
+ }
+ if (memcmp(result, template[i].digest,
+ crypto_ahash_digestsize(tfm))) {
+ pr_err("alg: hash: Partial Test %d failed for %s\n",
+ j, algo);
+ hexdump(result, crypto_ahash_digestsize(tfm));
+ ret = -EINVAL;
+ goto out;
+ }
+ }
+
ret = 0;

out:
diff --git a/crypto/testmgr.h b/crypto/testmgr.h
index da0a8fd..451e7eb 100644
--- a/crypto/testmgr.h
+++ b/crypto/testmgr.h
@@ -44,6 +44,7 @@ struct hash_testvec {
unsigned short psize;
unsigned char np;
unsigned char ksize;
+ unsigned char partial;
};

/*
@@ -772,7 +773,8 @@ static struct hash_testvec sha1_tv_template[] = {
.digest = "\x97\x01\x11\xc4\xe7\x7b\xcc\x88\xcc\x20"
"\x45\x9c\x02\xb6\x9b\x4a\xa8\xf5\x82\x17",
.np = 4,
- .tap = { 63, 64, 31, 5 }
+ .tap = { 63, 64, 31, 5 },
+ .partial = 1,
}, {
.plaintext = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+-",
.psize = 64,
--
1.8.3.1

2016-01-27 09:08:36

by Rui Wang

[permalink] [raw]
Subject: [PATCH v2 2/4] crypto: mcryptd - Fix load failure

mcryptd_create_hash() fails by returning -EINVAL, causing any
driver using mcryptd to fail to load. It is because it needs
to set its statesize properly.

Signed-off-by: Rui Wang <[email protected]>
---
crypto/mcryptd.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/crypto/mcryptd.c b/crypto/mcryptd.c
index f78d4fc..c4eb9da 100644
--- a/crypto/mcryptd.c
+++ b/crypto/mcryptd.c
@@ -522,6 +522,7 @@ static int mcryptd_create_hash(struct crypto_template *tmpl, struct rtattr **tb,
inst->alg.halg.base.cra_flags = type;

inst->alg.halg.digestsize = salg->digestsize;
+ inst->alg.halg.statesize = salg->statesize;
inst->alg.halg.base.cra_ctxsize = sizeof(struct mcryptd_hash_ctx);

inst->alg.halg.base.cra_init = mcryptd_hash_init_tfm;
--
1.8.3.1

2016-02-01 08:16:47

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] crypto: mcryptd - Fix load failure

On Wed, Jan 27, 2016 at 05:08:36PM +0800, Rui Wang wrote:
> mcryptd_create_hash() fails by returning -EINVAL, causing any
> driver using mcryptd to fail to load. It is because it needs
> to set its statesize properly.
>
> Signed-off-by: Rui Wang <[email protected]>

Applied.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2016-02-01 08:17:09

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] crypto: algif_hash - wait for crypto_ahash_init() to complete

On Wed, Jan 27, 2016 at 05:08:37PM +0800, Rui Wang wrote:
> hash_sendmsg/sendpage() need to wait for the completion
> of crypto_ahash_init() otherwise it can cause panic.
>
> Signed-off-by: Rui Wang <[email protected]>

Applied to crypto.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2016-02-01 08:17:57

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH v2 1/4] crypto x86/sha1_mb: Fix load failure

On Wed, Jan 27, 2016 at 05:08:35PM +0800, Rui Wang wrote:
>
> +static int sha1_mb_async_import(struct ahash_request *req, const void *in)
> +{
> + struct ahash_request *mcryptd_req = ahash_request_ctx(req);
> + struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> + struct sha1_mb_ctx *ctx = crypto_ahash_ctx(tfm);
> + struct mcryptd_ahash *mcryptd_tfm = ctx->mcryptd_tfm;
> + struct crypto_shash *child = mcryptd_ahash_child(mcryptd_tfm);
> + struct mcryptd_hash_request_ctx *rctx;
> + struct shash_desc *desc;
> + int err;
> +
> + memcpy(mcryptd_req, req, sizeof(*req));
> + ahash_request_set_tfm(mcryptd_req, &mcryptd_tfm->base);
> + rctx = ahash_request_ctx(mcryptd_req);
> + desc = &rctx->desc;
> + desc->tfm = child;
> + desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
> +
> + err = crypto_shash_init(desc);
> + if (err)
> + return err;

What is this desc for?

> + return crypto_ahash_import(mcryptd_req, in);
> +}

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2016-02-01 08:21:37

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] crypto: testmgr - Add a test case for import()/export()

On Wed, Jan 27, 2016 at 05:08:38PM +0800, Rui Wang wrote:
>
> diff --git a/crypto/testmgr.h b/crypto/testmgr.h
> index da0a8fd..451e7eb 100644
> --- a/crypto/testmgr.h
> +++ b/crypto/testmgr.h
> @@ -44,6 +44,7 @@ struct hash_testvec {
> unsigned short psize;
> unsigned char np;
> unsigned char ksize;
> + unsigned char partial;

Why not make it unconditional?

Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2016-02-02 14:16:33

by Rui Wang

[permalink] [raw]
Subject: Re: [PATCH v2 1/4] crypto x86/sha1_mb: Fix load failure

On Monday, February 1, 2016 4:18 PM, Herbert Xu wrote:
>
> On Wed, Jan 27, 2016 at 05:08:35PM +0800, Rui Wang wrote:
>>
>> +static int sha1_mb_async_import(struct ahash_request *req, const void
>> +*in) {
>> + struct ahash_request *mcryptd_req = ahash_request_ctx(req);
>> + struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
>> + struct sha1_mb_ctx *ctx = crypto_ahash_ctx(tfm);
>> + struct mcryptd_ahash *mcryptd_tfm = ctx->mcryptd_tfm;
>> + struct crypto_shash *child = mcryptd_ahash_child(mcryptd_tfm);
>> + struct mcryptd_hash_request_ctx *rctx;
>> + struct shash_desc *desc;
>> + int err;
>> +
>> + memcpy(mcryptd_req, req, sizeof(*req));
>> + ahash_request_set_tfm(mcryptd_req, &mcryptd_tfm->base);
>> + rctx = ahash_request_ctx(mcryptd_req);
>> + desc = &rctx->desc;
>> + desc->tfm = child;
>> + desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
>> +
>> + err = crypto_shash_init(desc);
>> + if (err)
>> + return err;
>
> What is this desc for?

Hi Herbert,

Yeah I just realized that the call to crypto_shash_init() isn't necessary
here. What it does is overwritten by crypto_ahash_import(). But this desc
still needs to be initialized here because it's newly allocated by
ahash_request_alloc(). We eventually calls the shash version of import()
which needs desc as an argument. The real context to be imported is then
derived from shash_desc_ctx(desc).

desc is a sub-field of struct mcryptd_hash_request_ctx, which is again a
sub-field of the bigger blob allocated by ahash_request_alloc(). The entire
blob's size is set in sha1_mb_async_init_tfm(). So a better version is as
follows:

(just removed the call to crypto_shash_init())

>From 4bcb73adbef99aada94c49f352063619aa24d43d Mon Sep 17 00:00:00 2001
From: Rui Wang <[email protected]>
Date: Mon, 14 Dec 2015 17:22:13 +0800
Subject: [PATCH v2 1/4] crypto x86/sha1_mb: Fix load failure

modprobe sha1_mb fails with the following message:

modprobe: ERROR: could not insert 'sha1_mb': No such device

It is because it needs to set its statesize and implement its
import() and export() interface.

v2: remove redundant call to crypto_shash_init()

Signed-off-by: Rui Wang <[email protected]>
---
arch/x86/crypto/sha-mb/sha1_mb.c | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)

diff --git a/arch/x86/crypto/sha-mb/sha1_mb.c b/arch/x86/crypto/sha-mb/sha1_mb.c
index a841e97..a8a0224 100644
--- a/arch/x86/crypto/sha-mb/sha1_mb.c
+++ b/arch/x86/crypto/sha-mb/sha1_mb.c
@@ -762,6 +762,38 @@ static int sha1_mb_async_digest(struct ahash_request *req)
return crypto_ahash_digest(mcryptd_req);
}

+static int sha1_mb_async_export(struct ahash_request *req, void *out)
+{
+ struct ahash_request *mcryptd_req = ahash_request_ctx(req);
+ struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
+ struct sha1_mb_ctx *ctx = crypto_ahash_ctx(tfm);
+ struct mcryptd_ahash *mcryptd_tfm = ctx->mcryptd_tfm;
+
+ memcpy(mcryptd_req, req, sizeof(*req));
+ ahash_request_set_tfm(mcryptd_req, &mcryptd_tfm->base);
+ return crypto_ahash_export(mcryptd_req, out);
+}
+
+static int sha1_mb_async_import(struct ahash_request *req, const void *in)
+{
+ struct ahash_request *mcryptd_req = ahash_request_ctx(req);
+ struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
+ struct sha1_mb_ctx *ctx = crypto_ahash_ctx(tfm);
+ struct mcryptd_ahash *mcryptd_tfm = ctx->mcryptd_tfm;
+ struct crypto_shash *child = mcryptd_ahash_child(mcryptd_tfm);
+ struct mcryptd_hash_request_ctx *rctx;
+ struct shash_desc *desc;
+
+ memcpy(mcryptd_req, req, sizeof(*req));
+ ahash_request_set_tfm(mcryptd_req, &mcryptd_tfm->base);
+ rctx = ahash_request_ctx(mcryptd_req);
+ desc = &rctx->desc;
+ desc->tfm = child;
+ desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
+
+ return crypto_ahash_import(mcryptd_req, in);
+}
+
static int sha1_mb_async_init_tfm(struct crypto_tfm *tfm)
{
struct mcryptd_ahash *mcryptd_tfm;
@@ -796,8 +828,11 @@ static struct ahash_alg sha1_mb_async_alg = {
.final = sha1_mb_async_final,
.finup = sha1_mb_async_finup,
.digest = sha1_mb_async_digest,
+ .export = sha1_mb_async_export,
+ .import = sha1_mb_async_import,
.halg = {
.digestsize = SHA1_DIGEST_SIZE,
+ .statesize = sizeof(struct sha1_hash_ctx),
.base = {
.cra_name = "sha1",
.cra_driver_name = "sha1_mb",
--
1.8.3.1

2016-02-02 14:36:34

by Rui Wang

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] crypto: testmgr - Add a test case for import()/export()

On Mon, Feb 1, 2016 4:22 PM Herbert Xu wrote:
>
> On Wed, Jan 27, 2016 at 05:08:38PM +0800, Rui Wang wrote:
> >
> > diff --git a/crypto/testmgr.h b/crypto/testmgr.h index
> > da0a8fd..451e7eb 100644
> > --- a/crypto/testmgr.h
> > +++ b/crypto/testmgr.h
> > @@ -44,6 +44,7 @@ struct hash_testvec {
> > unsigned short psize;
> > unsigned char np;
> > unsigned char ksize;
> > + unsigned char partial;
>
> Why not make it unconditional?
>

I initially made it unconditional, but then I found that it can easily
hang the machine during boot due to any import/export bug in any of
the hash drivers. So I used this .partial flag to guard against this
risk. Only when an author is confident that his driver can do this
test, should he add this flag. What do you think?

Thanks
Rui

2016-02-02 14:44:48

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] crypto: testmgr - Add a test case for import()/export()

On Tue, Feb 02, 2016 at 10:16:34PM +0800, Rui Wang wrote:
>
> I initially made it unconditional, but then I found that it can easily
> hang the machine during boot due to any import/export bug in any of
> the hash drivers. So I used this .partial flag to guard against this
> risk. Only when an author is confident that his driver can do this
> test, should he add this flag. What do you think?

Well if they're buggy they may crash anyway. Considering that all
the buggy drivers have probably been disabled for the time being I'd
say let's make it unconditional.

Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2016-02-02 15:03:40

by Rui Wang

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] crypto: testmgr - Add a test case for import()/export()

On Tue, Feb 2, 2016 10:45 PM Herbert Xu wrote:
>
> On Tue, Feb 02, 2016 at 10:16:34PM +0800, Rui Wang wrote:
> >
> > I initially made it unconditional, but then I found that it can easily
> > hang the machine during boot due to any import/export bug in any of
> > the hash drivers. So I used this .partial flag to guard against this
> > risk. Only when an author is confident that his driver can do this
> > test, should he add this flag. What do you think?
>
> Well if they're buggy they may crash anyway. Considering that all the buggy
> drivers have probably been disabled for the time being I'd say let's make it
> unconditional.

You are right the ahash drivers are already disabled. The shash drivers using
cryptd or mcryptd are probably OK by now. I'll do some tests.

Thanks
Rui

2016-02-03 10:46:34

by Rui Wang

[permalink] [raw]
Subject: [PATCH v3 4/4] crypto: testmgr - Add a test case for import()/export()

Modify __test_hash() so that hash import/export can be tested
from within the kernel. The test is unconditionally done when
a struct hash_testvec has its .np > 1.

v3: make the test unconditional
v2: Leverage template[i].np as suggested by Tim Chen

Signed-off-by: Rui Wang <[email protected]>
---
crypto/testmgr.c | 133 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 133 insertions(+)

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index ae8c57fd..3afae37 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -198,6 +198,61 @@ static int wait_async_op(struct tcrypt_result *tr, int ret)
return ret;
}

+static int ahash_partial_update(struct ahash_request **preq,
+ struct crypto_ahash *tfm, struct hash_testvec *template,
+ void *hash_buff, int k, int temp, struct scatterlist *sg,
+ const char *algo, char *result, struct tcrypt_result *tresult)
+{
+ char *state;
+ struct ahash_request *req;
+ int statesize, ret = -EINVAL;
+
+ req = *preq;
+ statesize = crypto_ahash_statesize(
+ crypto_ahash_reqtfm(req));
+ state = kmalloc(statesize, GFP_KERNEL);
+ if (!state) {
+ pr_err("alt: hash: Failed to alloc state for %s\n", algo);
+ goto out_nostate;
+ }
+ ret = crypto_ahash_export(req, state);
+ if (ret) {
+ pr_err("alt: hash: Failed to export() for %s\n", algo);
+ goto out;
+ }
+ ahash_request_free(req);
+ req = ahash_request_alloc(tfm, GFP_KERNEL);
+ if (!req) {
+ pr_err("alg: hash: Failed to alloc request for %s\n", algo);
+ goto out_noreq;
+ }
+ ahash_request_set_callback(req,
+ CRYPTO_TFM_REQ_MAY_BACKLOG,
+ tcrypt_complete, tresult);
+
+ memcpy(hash_buff, template->plaintext + temp,
+ template->tap[k]);
+ sg_init_one(&sg[0], hash_buff, template->tap[k]);
+ ahash_request_set_crypt(req, sg, result, template->tap[k]);
+ ret = crypto_ahash_import(req, state);
+ if (ret) {
+ pr_err("alg: hash: Failed to import() for %s\n", algo);
+ goto out;
+ }
+ ret = wait_async_op(tresult, crypto_ahash_update(req));
+ if (ret)
+ goto out;
+ *preq = req;
+ ret = 0;
+ goto out_noreq;
+out:
+ ahash_request_free(req);
+out_noreq:
+ kfree(state);
+out_nostate:
+ return ret;
+}
+
static int __test_hash(struct crypto_ahash *tfm, struct hash_testvec *template,
unsigned int tcount, bool use_digest,
const int align_offset)
@@ -385,6 +440,84 @@ static int __test_hash(struct crypto_ahash *tfm, struct hash_testvec *template,
}
}

+ /* partial update exercise */
+ j = 0;
+ for (i = 0; i < tcount; i++) {
+ /* alignment tests are only done with continuous buffers */
+ if (align_offset != 0)
+ break;
+
+ if (template[i].np < 2)
+ continue;
+
+ j++;
+ memset(result, 0, MAX_DIGEST_SIZE);
+
+ ret = -EINVAL;
+ hash_buff = xbuf[0];
+ memcpy(hash_buff, template[i].plaintext,
+ template[i].tap[0]);
+ sg_init_one(&sg[0], hash_buff, template[i].tap[0]);
+
+ if (template[i].ksize) {
+ crypto_ahash_clear_flags(tfm, ~0);
+ if (template[i].ksize > MAX_KEYLEN) {
+ pr_err("alg: hash: setkey failed on test %d for %s: key size %d > %d\n",
+ j, algo, template[i].ksize, MAX_KEYLEN);
+ ret = -EINVAL;
+ goto out;
+ }
+ memcpy(key, template[i].key, template[i].ksize);
+ ret = crypto_ahash_setkey(tfm, key, template[i].ksize);
+ if (ret) {
+ pr_err("alg: hash: setkey failed on test %d for %s: ret=%d\n",
+ j, algo, -ret);
+ goto out;
+ }
+ }
+
+ ahash_request_set_crypt(req, sg, result, template[i].tap[0]);
+ ret = wait_async_op(&tresult, crypto_ahash_init(req));
+ if (ret) {
+ pr_err("alt: hash: init failed on test %d for %s: ret=%d\n",
+ j, algo, -ret);
+ goto out;
+ }
+ ret = wait_async_op(&tresult, crypto_ahash_update(req));
+ if (ret) {
+ pr_err("alt: hash: update failed on test %d for %s: ret=%d\n",
+ j, algo, -ret);
+ goto out;
+ }
+
+ temp = template[i].tap[0];
+ for (k = 1; k < template[i].np; k++) {
+ ret = ahash_partial_update(&req, tfm, &template[i],
+ hash_buff, k, temp, &sg[0], algo, result,
+ &tresult);
+ if (ret) {
+ pr_err("hash: partial update failed on test %d for %s: ret=%d\n",
+ j, algo, -ret);
+ goto out_noreq;
+ }
+ temp += template[i].tap[k];
+ }
+ ret = wait_async_op(&tresult, crypto_ahash_final(req));
+ if (ret) {
+ pr_err("alt: hash: final failed on test %d for %s: ret=%d\n",
+ j, algo, -ret);
+ goto out;
+ }
+ if (memcmp(result, template[i].digest,
+ crypto_ahash_digestsize(tfm))) {
+ pr_err("alg: hash: Partial Test %d failed for %s\n",
+ j, algo);
+ hexdump(result, crypto_ahash_digestsize(tfm));
+ ret = -EINVAL;
+ goto out;
+ }
+ }
+
ret = 0;

out:
--
1.8.3.1

2016-02-06 07:47:01

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH v2 1/4] crypto x86/sha1_mb: Fix load failure

On Tue, Feb 02, 2016 at 09:56:45PM +0800, Rui Wang wrote:
>
> >From 4bcb73adbef99aada94c49f352063619aa24d43d Mon Sep 17 00:00:00 2001
> From: Rui Wang <[email protected]>
> Date: Mon, 14 Dec 2015 17:22:13 +0800
> Subject: [PATCH v2 1/4] crypto x86/sha1_mb: Fix load failure
>
> modprobe sha1_mb fails with the following message:
>
> modprobe: ERROR: could not insert 'sha1_mb': No such device
>
> It is because it needs to set its statesize and implement its
> import() and export() interface.
>
> v2: remove redundant call to crypto_shash_init()
>
> Signed-off-by: Rui Wang <[email protected]>

Applied.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2016-02-06 07:47:49

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH v3 4/4] crypto: testmgr - Add a test case for import()/export()

On Wed, Feb 03, 2016 at 06:26:57PM +0800, Rui Wang wrote:
> Modify __test_hash() so that hash import/export can be tested
> from within the kernel. The test is unconditionally done when
> a struct hash_testvec has its .np > 1.
>
> v3: make the test unconditional
> v2: Leverage template[i].np as suggested by Tim Chen
>
> Signed-off-by: Rui Wang <[email protected]>

Applied.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt