[PATCH 0/2] crypto: testmgr - AEAD extra tests fixes

[PATCH 0/2] crypto: testmgr - AEAD extra tests fixes

[Gilad Ben-Yossef]

Fix issues found while debugging ccree driver failures with the new
AEAD extra tests code.

Cc: Geert Uytterhoeven <geert+renesas@glider.be>

Gilad Ben-Yossef (2):
  crypto: testmgr - use generic algs making test vecs
  crypto: testmgr - sync both RFC4106 IV copies

 crypto/testmgr.c | 70 ++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 65 insertions(+), 5 deletions(-)

-- 
2.25.0

[PATCH 1/2] crypto: testmgr - use generic algs making test vecs

[Gilad Ben-Yossef]

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

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;
 }
 
 /*
-- 
2.25.0

[PATCH 2/2] crypto: testmgr - sync both RFC4106 IV copies

[Gilad Ben-Yossef]

RFC4106 AEAD ciphers the AAD is the concatenation of associated
authentication data || IV || plaintext or ciphertext but the
random AEAD message generation in testmgr extended tests did
not obey this requirements producing messages with undefined
behaviours. Fix it by syncing the copies if needed.

Since this only relevant for developer only extended tests any
additional cycles/run time costs are negligible.

This fixes extended AEAD test failures with the ccree driver
caused by illegal input.

Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Eric Biggers <ebiggers@kernel.org>
---
 crypto/testmgr.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index cf565b063cdf..288f349a0cae 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -95,6 +95,11 @@ struct aead_test_suite {
 	 * AAD buffer during decryption.
 	 */
 	unsigned int esp_aad : 1;
+
+	/*
+	 * Set if the algorithm requires the IV to trail the AAD buffer.
+	 */
+	unsigned int iv_aad : 1;
 };
 
 struct cipher_test_suite {
@@ -2207,6 +2212,10 @@ static void generate_aead_message(struct aead_request *req,
 
 	/* Generate the AAD. */
 	generate_random_bytes((u8 *)vec->assoc, vec->alen);
+	/* For RFC4106 algs, a copy of the IV is part of the AAD */
+	if (suite->iv_aad)
+		memcpy(((u8 *)vec->assoc + vec->alen - ivsize), vec->iv,
+		       ivsize);
 
 	if (inauthentic && prandom_u32() % 2 == 0) {
 		/* Generate a random ciphertext. */
@@ -2247,6 +2256,14 @@ static void generate_aead_message(struct aead_request *req,
 	vec->novrfy = 1;
 	if (suite->einval_allowed)
 		vec->crypt_error = -EINVAL;
+
+	/*
+	 * For RFC4106 algs, the IV is embedded as part of the AAD
+	 * and we might have mutated the AAD so sync the copies
+	 */
+	if (suite->iv_aad)
+		memcpy((u8 *)vec->iv, (vec->assoc + vec->alen - ivsize),
+		       ivsize);
 }
 
 /*
@@ -5243,6 +5260,7 @@ static const struct alg_test_desc alg_test_descs[] = {
 				____VECS(aes_gcm_rfc4106_tv_template),
 				.einval_allowed = 1,
 				.esp_aad = 1,
+				.iv_aad = 1,
 			}
 		}
 	}, {
@@ -5255,6 +5273,7 @@ static const struct alg_test_desc alg_test_descs[] = {
 				____VECS(aes_ccm_rfc4309_tv_template),
 				.einval_allowed = 1,
 				.esp_aad = 1,
+				.iv_aad = 1,
 			}
 		}
 	}, {
@@ -5265,6 +5284,7 @@ static const struct alg_test_desc alg_test_descs[] = {
 			.aead = {
 				____VECS(aes_gcm_rfc4543_tv_template),
 				.einval_allowed = 1,
+				.iv_aad = 1,
 			}
 		}
 	}, {
-- 
2.25.0

Re: [PATCH 1/2] crypto: testmgr - use generic algs making test vecs

[Eric Biggers]

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

Re: [PATCH 2/2] crypto: testmgr - sync both RFC4106 IV copies

[Eric Biggers]

On Tue, Feb 25, 2020 at 05:48:34PM +0200, Gilad Ben-Yossef wrote:
> RFC4106 AEAD ciphers the AAD is the concatenation of associated
> authentication data || IV || plaintext or ciphertext but the
> random AEAD message generation in testmgr extended tests did
> not obey this requirements producing messages with undefined
> behaviours. Fix it by syncing the copies if needed.
> 
> Since this only relevant for developer only extended tests any
> additional cycles/run time costs are negligible.
> 
> This fixes extended AEAD test failures with the ccree driver
> caused by illegal input.
> 
> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Eric Biggers <ebiggers@kernel.org>
> ---
>  crypto/testmgr.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> index cf565b063cdf..288f349a0cae 100644
> --- a/crypto/testmgr.c
> +++ b/crypto/testmgr.c
> @@ -95,6 +95,11 @@ struct aead_test_suite {
>       /*
>        * Set if the algorithm intentionally ignores the last 8 bytes of the
>        * AAD buffer during decryption.
>        */
>       unsigned int esp_aad : 1;
> +
> +	/*
> +	 * Set if the algorithm requires the IV to trail the AAD buffer.
> +	 */
> +	unsigned int iv_aad : 1;
>  };

What's the difference between esp_aad and iv_aad?  Are you sure we need another
flag and not just use the existing flag?

If they're both needed, please document them properly.  You're currently setting
them both on some algorithms, which based on the current comments is a logical
contradiction because esp_aad is documented to mean that the last 8 bytes are
ignored while iv_aad is documented to mean that these bytes must be the IV.

>  
>  struct cipher_test_suite {
> @@ -2207,6 +2212,10 @@ static void generate_aead_message(struct aead_request *req,
>  
>  	/* Generate the AAD. */
>  	generate_random_bytes((u8 *)vec->assoc, vec->alen);
> +	/* For RFC4106 algs, a copy of the IV is part of the AAD */
> +	if (suite->iv_aad)
> +		memcpy(((u8 *)vec->assoc + vec->alen - ivsize), vec->iv,
> +		       ivsize);

What guarantees that vec->alen >= ivsize?

- Eric

Re: [PATCH 1/2] crypto: testmgr - use generic algs making test vecs

[Eric Biggers]

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

Re: [PATCH 1/2] crypto: testmgr - use generic algs making test vecs

[Gilad Ben-Yossef]

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



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

values of β will give rise to dom!

Re: [PATCH 2/2] crypto: testmgr - sync both RFC4106 IV copies

[Gilad Ben-Yossef]

On Tue, Feb 25, 2020 at 10:02 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Feb 25, 2020 at 05:48:34PM +0200, Gilad Ben-Yossef wrote:
> > RFC4106 AEAD ciphers the AAD is the concatenation of associated
> > authentication data || IV || plaintext or ciphertext but the
> > random AEAD message generation in testmgr extended tests did
> > not obey this requirements producing messages with undefined
> > behaviours. Fix it by syncing the copies if needed.
> >
> > Since this only relevant for developer only extended tests any
> > additional cycles/run time costs are negligible.
> >
> > This fixes extended AEAD test failures with the ccree driver
> > caused by illegal input.
> >
> > Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
> > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Cc: Eric Biggers <ebiggers@kernel.org>
> > ---
> >  crypto/testmgr.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> > index cf565b063cdf..288f349a0cae 100644
> > --- a/crypto/testmgr.c
> > +++ b/crypto/testmgr.c
> > @@ -95,6 +95,11 @@ struct aead_test_suite {
> >       /*
> >        * Set if the algorithm intentionally ignores the last 8 bytes of the
> >        * AAD buffer during decryption.
> >        */
> >       unsigned int esp_aad : 1;
> > +
> > +     /*
> > +      * Set if the algorithm requires the IV to trail the AAD buffer.
> > +      */
> > +     unsigned int iv_aad : 1;
> >  };
>
> What's the difference between esp_aad and iv_aad?  Are you sure we need another
> flag and not just use the existing flag?

Yes, because we have 3 distinct states -
1. No IV in the AAD buffer - "normal"  AEAD.
2. There is a copy of the IV in the AAD buffer and it is NOT used for
ICV computation purposes - RFC4106 and RFC 4309.
3, There is a copy of the IV in the AAD buffer and it is used for ICV
computation purposes - RFC 4543.

3 states needs at least 2 bits.

> If they're both needed, please document them properly.  You're currently setting

I will add a remark explaining this.
I chose to keep the "esp_aad" name, since it was there before, but
possibly this is not a good choice in light of your comments so will
change that too.

> them both on some algorithms, which based on the current comments is a logical
> contradiction because esp_aad is documented to mean that the last 8 bytes are
> ignored while iv_aad is documented to mean that these bytes must be the IV.

I believe it isn't a contradiction after all. Consider -

RFC 4106 needs to have a copy of the IV in the AAD buffer which is
identical to the one being passed via the normal mechanism in the API.
If they are not identical, we have no way to know which copy of the IV
is being used by an implementation as part of the AES-GCM nonce and so
the generated message may be different from the one being used by the
generic implementation. which results in encryption test failing when
compared to the generic implementation  results, even though there is
nothing wrong with the tested implementation and indeed the previous
compassion against the precomputed test vectors passes.
This is what happened with the ccree driver. Note that this has
nothing to do with mutating the message - this is an encryption test
failing.
This is the iv_aad flag, which will need to be true in this case.

On the other hand when testing decryption and mutating the AEAD
message, we need to know not to mutate the IV copy in the AAD buffer,
since it is ignored.
This is the esp_aad flag, which will also needs to be true in this case.

Last but not least, RFC 4543 need a copy of the IV in the AAD buffer
However, this copy DOES get hashed as part of the ICV computation, so
-
We need iv_aad flag to be true to let us know we need to copy the IV.
However, we need esp_aad to be false since it's fine and even
desirable to mutate the IV copy in the AAD buffer.

You are correct though that I can make the 2nd copy of the IV post
mutation dependant on esp_aad not being set, though...


I hope this explains this mess better.
It certainly took me a while to figure out what is going on...


>
> >
> >  struct cipher_test_suite {
> > @@ -2207,6 +2212,10 @@ static void generate_aead_message(struct aead_request *req,
> >
> >       /* Generate the AAD. */
> >       generate_random_bytes((u8 *)vec->assoc, vec->alen);
> > +     /* For RFC4106 algs, a copy of the IV is part of the AAD */
> > +     if (suite->iv_aad)
> > +             memcpy(((u8 *)vec->assoc + vec->alen - ivsize), vec->iv,
> > +                    ivsize);
>
> What guarantees that vec->alen >= ivsize?

You are right, I need to check for that.
However, if it isn't this can't be a legal RFC4106 message and we will
fail encryption .

Thanks!
Gilad


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

values of β will give rise to dom!

Re: [PATCH 1/2] crypto: testmgr - use generic algs making test vecs

[Eric Biggers]

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