| Message ID | 20200225154834.25108-2-gilad@benyossef.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: testmgr - AEAD extra tests fixes | expand |
On Tue, Feb 25, 2020 at 05:48:33PM +0200, Gilad Ben-Yossef wrote: > Use generic algs to produce inauthentic AEAD messages, > otherwise we are running the risk of using an untested > code to produce the test messages. > > As this code is only used in developer only extended tests > any cycles/runtime costs are negligible. > > Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com> > Cc: Eric Biggers <ebiggers@kernel.org> It's intentional to use the same implementation to generate the inauthentic AEAD messages, because it allows the inauthentic AEAD input tests to run even if the generic implementation is unavailable. > @@ -2337,8 +2338,42 @@ static int test_aead_inauthentic_inputs(struct aead_extra_tests_ctx *ctx) > { > unsigned int i; > int err; > + struct crypto_aead *tfm = ctx->tfm; > + const char *algname = crypto_aead_alg(tfm)->base.cra_name; > + const char *driver = ctx->driver; > + const char *generic_driver = ctx->test_desc->generic_driver; > + char _generic_driver[CRYPTO_MAX_ALG_NAME]; > + struct crypto_aead *generic_tfm = NULL; > + struct aead_request *generic_req = NULL; > + > + if (!generic_driver) { > + err = build_generic_driver_name(algname, _generic_driver); > + if (err) > + return err; > + generic_driver = _generic_driver; > + } > + > + if (!strcmp(generic_driver, driver) == 0) { > + /* Already the generic impl? */ > + > + generic_tfm = crypto_alloc_aead(generic_driver, 0, 0); I think you meant the condition to be 'if (strcmp(generic_driver, driver) != 0)' and for the comment to be "Not already the generic impl?". > + if (IS_ERR(generic_tfm)) { > + err = PTR_ERR(generic_tfm); > + pr_err("alg: aead: error allocating %s (generic impl of %s): %d\n", > + generic_driver, algname, err); > + return err; > + } This means the test won't run if the generic implementation is unavailable. Is there any particular reason to impose that requirement? You mentioned a concern about the implementation being "untested", but it actually already passed test_aead() before getting to test_aead_extra(). We could also just move test_aead_inauthentic_inputs() to below test_aead_vs_generic_impl() so that it runs last. - Eric
On Tue, Feb 25, 2020 at 11:45:51AM -0800, Eric Biggers wrote: > On Tue, Feb 25, 2020 at 05:48:33PM +0200, Gilad Ben-Yossef wrote: > > Use generic algs to produce inauthentic AEAD messages, > > otherwise we are running the risk of using an untested > > code to produce the test messages. > > > > As this code is only used in developer only extended tests > > any cycles/runtime costs are negligible. > > > > Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com> > > Cc: Eric Biggers <ebiggers@kernel.org> > > It's intentional to use the same implementation to generate the inauthentic AEAD > messages, because it allows the inauthentic AEAD input tests to run even if the > generic implementation is unavailable. > > > @@ -2337,8 +2338,42 @@ static int test_aead_inauthentic_inputs(struct aead_extra_tests_ctx *ctx) > > { > > unsigned int i; > > int err; > > + struct crypto_aead *tfm = ctx->tfm; > > + const char *algname = crypto_aead_alg(tfm)->base.cra_name; > > + const char *driver = ctx->driver; > > + const char *generic_driver = ctx->test_desc->generic_driver; > > + char _generic_driver[CRYPTO_MAX_ALG_NAME]; > > + struct crypto_aead *generic_tfm = NULL; > > + struct aead_request *generic_req = NULL; > > + > > + if (!generic_driver) { > > + err = build_generic_driver_name(algname, _generic_driver); > > + if (err) > > + return err; > > + generic_driver = _generic_driver; > > + } > > + > > + if (!strcmp(generic_driver, driver) == 0) { > > + /* Already the generic impl? */ > > + > > + generic_tfm = crypto_alloc_aead(generic_driver, 0, 0); > > I think you meant the condition to be 'if (strcmp(generic_driver, driver) != 0)' > and for the comment to be "Not already the generic impl?". > > > + if (IS_ERR(generic_tfm)) { > > + err = PTR_ERR(generic_tfm); > > + pr_err("alg: aead: error allocating %s (generic impl of %s): %d\n", > > + generic_driver, algname, err); > > + return err; > > + } > > This means the test won't run if the generic implementation is unavailable. > Is there any particular reason to impose that requirement? > > You mentioned a concern about the implementation being "untested", but it > actually already passed test_aead() before getting to test_aead_extra(). > > We could also just move test_aead_inauthentic_inputs() to below > test_aead_vs_generic_impl() so that it runs last. > Also: if we did make the inauthentic input tests use the generic implementation, then it would be better to move them into test_aead_vs_generic_impl() so that we don't duplicate the code that allocates a tfm and request for the generic implementation. But to me it makes more sense to keep them separate, since a generic implementation is not needed to run the inauthentic input tests. - Eric
On Tue, Feb 25, 2020 at 9:45 PM Eric Biggers <ebiggers@kernel.org> wrote: > > On Tue, Feb 25, 2020 at 05:48:33PM +0200, Gilad Ben-Yossef wrote: > > Use generic algs to produce inauthentic AEAD messages, > > otherwise we are running the risk of using an untested > > code to produce the test messages. > > > > As this code is only used in developer only extended tests > > any cycles/runtime costs are negligible. > > > > Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com> > > Cc: Eric Biggers <ebiggers@kernel.org> > > It's intentional to use the same implementation to generate the inauthentic AEAD > messages, because it allows the inauthentic AEAD input tests to run even if the > generic implementation is unavailable. That is a good. We can simply revert to the same implementation if the generic one is not available. > > > @@ -2337,8 +2338,42 @@ static int test_aead_inauthentic_inputs(struct aead_extra_tests_ctx *ctx) > > { > > unsigned int i; > > int err; > > + struct crypto_aead *tfm = ctx->tfm; > > + const char *algname = crypto_aead_alg(tfm)->base.cra_name; > > + const char *driver = ctx->driver; > > + const char *generic_driver = ctx->test_desc->generic_driver; > > + char _generic_driver[CRYPTO_MAX_ALG_NAME]; > > + struct crypto_aead *generic_tfm = NULL; > > + struct aead_request *generic_req = NULL; > > + > > + if (!generic_driver) { > > + err = build_generic_driver_name(algname, _generic_driver); > > + if (err) > > + return err; > > + generic_driver = _generic_driver; > > + } > > + > > + if (!strcmp(generic_driver, driver) == 0) { > > + /* Already the generic impl? */ > > + > > + generic_tfm = crypto_alloc_aead(generic_driver, 0, 0); > > I think you meant the condition to be 'if (strcmp(generic_driver, driver) != 0)' > and for the comment to be "Not already the generic impl?". Yes, of course. Silly me, > > > + if (IS_ERR(generic_tfm)) { > > + err = PTR_ERR(generic_tfm); > > + pr_err("alg: aead: error allocating %s (generic impl of %s): %d\n", > > + generic_driver, algname, err); > > + return err; > > + } > > This means the test won't run if the generic implementation is unavailable. > Is there any particular reason to impose that requirement? > > You mentioned a concern about the implementation being "untested", but it > actually already passed test_aead() before getting to test_aead_extra(). > The impetus to write this patch came from my experience debugging a test failure with the ccree driver. At some point while tweaking around I got into a situation where the test was succeeding (that is, declaring the message inauthentic) not because the mutation was being detected but because the generation of the origin was producing a bogus ICV. At that point it seemed to me that it would be safer to "isolate" the original AEAD messages generation from the code that was being teste. > We could also just move test_aead_inauthentic_inputs() to below > test_aead_vs_generic_impl() so that it runs last. This would probably be better, although I think that this stage also generates inauthentic messages from time to time, no? At any rate, I don't have strong feelings about it either way. I defer to your judgment whether it is worth it to add a fallback to use the same implementation and fix what needs fixing or drop the patch altogether if you think this isn't worth the trouble - just let me know. Thanks, Gilad
On Wed, Feb 26, 2020 at 04:42:45PM +0200, Gilad Ben-Yossef wrote: > > The impetus to write this patch came from my experience debugging a > test failure with the ccree driver. > At some point while tweaking around I got into a situation where the > test was succeeding (that is, declaring the message inauthentic) not > because the mutation was being detected but because the generation of > the origin was producing a bogus ICV. That's being fixed by your patch 2/2 though, right? > At that point it seemed to me that it would be safer to "isolate" the > original AEAD messages generation from the code that was being teste. > > > We could also just move test_aead_inauthentic_inputs() to below > > test_aead_vs_generic_impl() so that it runs last. > > This would probably be better, although I think that this stage also > generates inauthentic messages from time to time, no? That's correct, but in test_aead_vs_generic_impl() the generic implementation is used to generate the test vectors. > At any rate, I don't have strong feelings about it either way. I defer > to your judgment whether it is worth it to add a fallback to use the > same implementation and fix what needs fixing or drop the patch > altogether if you think this isn't worth the trouble - just let me > know. I just want to avoid adding complexity that isn't worthwhile. Beyond your patch 2, how about we just do: diff --git a/crypto/testmgr.c b/crypto/testmgr.c index 79b431545249a9..2ab48d4d317250 100644 --- a/crypto/testmgr.c +++ b/crypto/testmgr.c @@ -2564,11 +2564,11 @@ static int test_aead_extra(const char *driver, goto out; } - err = test_aead_inauthentic_inputs(ctx); + err = test_aead_vs_generic_impl(ctx); if (err) goto out; - err = test_aead_vs_generic_impl(ctx); + err = test_aead_inauthentic_inputs(ctx); out: kfree(ctx->vec.key); kfree(ctx->vec.iv); Then the dedicated tests for inauthentic inputs wouldn't be run until the fuzz tests vs. generic have already passed. - Eric
diff --git a/crypto/testmgr.c b/crypto/testmgr.c index 88f33c0efb23..cf565b063cdf 100644 --- a/crypto/testmgr.c +++ b/crypto/testmgr.c @@ -2314,12 +2314,13 @@ static void generate_random_aead_testvec(struct aead_request *req, } static void try_to_generate_inauthentic_testvec( - struct aead_extra_tests_ctx *ctx) + struct aead_extra_tests_ctx *ctx, + struct aead_request *req) { int i; for (i = 0; i < 10; i++) { - generate_random_aead_testvec(ctx->req, &ctx->vec, + generate_random_aead_testvec(req, &ctx->vec, &ctx->test_desc->suite.aead, ctx->maxkeysize, ctx->maxdatasize, ctx->vec_name, @@ -2337,8 +2338,42 @@ static int test_aead_inauthentic_inputs(struct aead_extra_tests_ctx *ctx) { unsigned int i; int err; + struct crypto_aead *tfm = ctx->tfm; + const char *algname = crypto_aead_alg(tfm)->base.cra_name; + const char *driver = ctx->driver; + const char *generic_driver = ctx->test_desc->generic_driver; + char _generic_driver[CRYPTO_MAX_ALG_NAME]; + struct crypto_aead *generic_tfm = NULL; + struct aead_request *generic_req = NULL; + + if (!generic_driver) { + err = build_generic_driver_name(algname, _generic_driver); + if (err) + return err; + generic_driver = _generic_driver; + } + + if (!strcmp(generic_driver, driver) == 0) { + /* Already the generic impl? */ + + generic_tfm = crypto_alloc_aead(generic_driver, 0, 0); + if (IS_ERR(generic_tfm)) { + err = PTR_ERR(generic_tfm); + pr_err("alg: aead: error allocating %s (generic impl of %s): %d\n", + generic_driver, algname, err); + return err; + } + + generic_req = aead_request_alloc(generic_tfm, GFP_KERNEL); + if (!generic_req) + goto free_tfm; + } for (i = 0; i < fuzz_iterations * 8; i++) { + struct aead_request *req; + + req = generic_req ? generic_req : ctx->req; + /* * Since this part of the tests isn't comparing the * implementation to another, there's no point in testing any @@ -2348,7 +2383,7 @@ static int test_aead_inauthentic_inputs(struct aead_extra_tests_ctx *ctx) * if the algorithm keeps rejecting the generated keys, don't * retry forever; just continue on. */ - try_to_generate_inauthentic_testvec(ctx); + try_to_generate_inauthentic_testvec(ctx, req); if (ctx->vec.novrfy) { generate_random_testvec_config(&ctx->cfg, ctx->cfgname, sizeof(ctx->cfgname)); @@ -2356,11 +2391,16 @@ static int test_aead_inauthentic_inputs(struct aead_extra_tests_ctx *ctx) ctx->vec_name, &ctx->cfg, ctx->req, ctx->tsgls); if (err) - return err; + goto out; } cond_resched(); } - return 0; + +out: + aead_request_free(generic_req); +free_tfm: + crypto_free_aead(generic_tfm); + return err; } /*
Use generic algs to produce inauthentic AEAD messages, otherwise we are running the risk of using an untested code to produce the test messages. As this code is only used in developer only extended tests any cycles/runtime costs are negligible. Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com> Cc: Eric Biggers <ebiggers@kernel.org> --- crypto/testmgr.c | 50 +++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 5 deletions(-)