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

[PATCH v2] 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 | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 88f33c0efb23..379bd1c7dd5b 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -91,10 +91,16 @@ struct aead_test_suite {
 	unsigned int einval_allowed : 1;
 
 	/*
-	 * Set if the algorithm intentionally ignores the last 8 bytes of the
-	 * AAD buffer during decryption.
+	 * Set if the algorithm includes a copy of the IV (last 8 bytes)
+	 * in the AAD buffer but does not include it in calculating the ICV
 	 */
-	unsigned int esp_aad : 1;
+	unsigned int skip_aad_iv : 1;
+
+	/*
+	 * Set if the algorithm includes a copy of the IV (last 8 bytes)
+	 * in the AAD buffer and does include it when calculating the ICV
+	 */
+	unsigned int auth_aad_iv : 1;
 };
 
 struct cipher_test_suite {
@@ -2167,14 +2173,20 @@ struct aead_extra_tests_ctx {
  * here means the full ciphertext including the authentication tag.  The
  * authentication tag (and hence also the ciphertext) is assumed to be nonempty.
  */
-static void mutate_aead_message(struct aead_testvec *vec, bool esp_aad)
+static void mutate_aead_message(struct aead_testvec *vec,
+				const struct aead_test_suite *suite)
 {
-	const unsigned int aad_tail_size = esp_aad ? 8 : 0;
+	const unsigned int aad_ivsize = 8;
+	const unsigned int aad_tail_size = suite->skip_aad_iv ? aad_ivsize : 0;
 	const unsigned int authsize = vec->clen - vec->plen;
 
 	if (prandom_u32() % 2 == 0 && vec->alen > aad_tail_size) {
 		 /* Mutate the AAD */
 		flip_random_bit((u8 *)vec->assoc, vec->alen - aad_tail_size);
+		if (suite->auth_aad_iv)
+			memcpy((u8 *)vec->iv,
+			       (vec->assoc + vec->alen - aad_ivsize),
+			       aad_ivsize);
 		if (prandom_u32() % 2 == 0)
 			return;
 	}
@@ -2208,6 +2220,10 @@ static void generate_aead_message(struct aead_request *req,
 	/* Generate the AAD. */
 	generate_random_bytes((u8 *)vec->assoc, vec->alen);
 
+	if (suite->auth_aad_iv && (vec->alen > ivsize))
+		memcpy(((u8 *)vec->assoc + vec->alen - ivsize), vec->iv,
+		       ivsize);
+
 	if (inauthentic && prandom_u32() % 2 == 0) {
 		/* Generate a random ciphertext. */
 		generate_random_bytes((u8 *)vec->ctext, vec->clen);
@@ -2242,7 +2258,7 @@ static void generate_aead_message(struct aead_request *req,
 		 * Mutate the authentic (ciphertext, AAD) pair to get an
 		 * inauthentic one.
 		 */
-		mutate_aead_message(vec, suite->esp_aad);
+		mutate_aead_message(vec, suite);
 	}
 	vec->novrfy = 1;
 	if (suite->einval_allowed)
@@ -5202,7 +5218,7 @@ static const struct alg_test_desc alg_test_descs[] = {
 			.aead = {
 				____VECS(aes_gcm_rfc4106_tv_template),
 				.einval_allowed = 1,
-				.esp_aad = 1,
+				.skip_aad_iv = 1,
 			}
 		}
 	}, {
@@ -5214,7 +5230,7 @@ static const struct alg_test_desc alg_test_descs[] = {
 			.aead = {
 				____VECS(aes_ccm_rfc4309_tv_template),
 				.einval_allowed = 1,
-				.esp_aad = 1,
+				.skip_aad_iv = 1,
 			}
 		}
 	}, {
@@ -5225,6 +5241,7 @@ static const struct alg_test_desc alg_test_descs[] = {
 			.aead = {
 				____VECS(aes_gcm_rfc4543_tv_template),
 				.einval_allowed = 1,
+				.auth_aad_iv = 1,
 			}
 		}
 	}, {
@@ -5240,7 +5257,7 @@ static const struct alg_test_desc alg_test_descs[] = {
 			.aead = {
 				____VECS(rfc7539esp_tv_template),
 				.einval_allowed = 1,
-				.esp_aad = 1,
+				.skip_aad_iv = 1,
 			}
 		}
 	}, {
-- 
2.25.1

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

[Eric Biggers]

On Tue, Mar 03, 2020 at 02:09:25PM +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 | 35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> index 88f33c0efb23..379bd1c7dd5b 100644
> --- a/crypto/testmgr.c
> +++ b/crypto/testmgr.c
> @@ -91,10 +91,16 @@ struct aead_test_suite {
>  	unsigned int einval_allowed : 1;
>  
>  	/*
> -	 * Set if the algorithm intentionally ignores the last 8 bytes of the
> -	 * AAD buffer during decryption.
> +	 * Set if the algorithm includes a copy of the IV (last 8 bytes)
> +	 * in the AAD buffer but does not include it in calculating the ICV
>  	 */
> -	unsigned int esp_aad : 1;
> +	unsigned int skip_aad_iv : 1;

"Authentication tag" would be easier to understand than "ICV" and would match
the rest of the code.  "ICV" is an idiosyncrasy used in certain RFCs only.

> +
> +	/*
> +	 * Set if the algorithm includes a copy of the IV (last 8 bytes)
> +	 * in the AAD buffer and does include it when calculating the ICV
> +	 */
> +	unsigned int auth_aad_iv : 1;
>  };
>  
>  struct cipher_test_suite {
> @@ -2167,14 +2173,20 @@ struct aead_extra_tests_ctx {
>   * here means the full ciphertext including the authentication tag.  The
>   * authentication tag (and hence also the ciphertext) is assumed to be nonempty.
>   */
> -static void mutate_aead_message(struct aead_testvec *vec, bool esp_aad)
> +static void mutate_aead_message(struct aead_testvec *vec,
> +				const struct aead_test_suite *suite)
>  {
> -	const unsigned int aad_tail_size = esp_aad ? 8 : 0;
> +	const unsigned int aad_ivsize = 8;

We should use the algorithm's actual IV size instead of hard-coding 8 bytes.

> +	const unsigned int aad_tail_size = suite->skip_aad_iv ? aad_ivsize : 0;
>  	const unsigned int authsize = vec->clen - vec->plen;
>  
>  	if (prandom_u32() % 2 == 0 && vec->alen > aad_tail_size) {
>  		 /* Mutate the AAD */
>  		flip_random_bit((u8 *)vec->assoc, vec->alen - aad_tail_size);
> +		if (suite->auth_aad_iv)
> +			memcpy((u8 *)vec->iv,
> +			       (vec->assoc + vec->alen - aad_ivsize),
> +			       aad_ivsize);

Why sync the IV copies here?  When 'auth_aad_iv', we assume the copy of the IV
in the AAD (which was just corrupted) is authenticated.  So we already know that
decryption should fail, regardless of the other IV copy.

Also, the code doesn't currently mutate vec->iv for any AEAD.  So mutating it
for one specific algorithm is a bit odd.  IMO, it would make more sense to do a
separate patch later that mutates vec->iv for all AEADs.

>  		if (prandom_u32() % 2 == 0)
>  			return;
>  	}
> @@ -2208,6 +2220,10 @@ static void generate_aead_message(struct aead_request *req,
>  	/* Generate the AAD. */
>  	generate_random_bytes((u8 *)vec->assoc, vec->alen);
>  
> +	if (suite->auth_aad_iv && (vec->alen > ivsize))
> +		memcpy(((u8 *)vec->assoc + vec->alen - ivsize), vec->iv,
> +		       ivsize);

Shouldn't this be >= ivsize, not > ivsize?  And doesn't the IV need to be synced
in both the skip_aad_iv and auth_aad_iv cases?

There are also unnecessary parentheses here; the memcpy() could be one line.


How about the following patch instead?

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index ccb3d60729fc..eea56fe8d1e8 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -91,10 +91,16 @@ struct aead_test_suite {
 	unsigned int einval_allowed : 1;
 
 	/*
-	 * Set if the algorithm intentionally ignores the last 8 bytes of the
-	 * AAD buffer during decryption.
+	 * 'aad_iv' is set if this algorithm requires that the IV be located at
+	 * the end of the AAD buffer, in addition to being given in the normal
+	 * way.  It's implementation-defined which IV copy the algorithm uses.
+	 *
+	 * 'aad_iv_auth' is set if the copy of the IV in the AAD buffer is
+	 * authenticated just like the rest of the AAD, i.e. if decryption with
+	 * the AAD IV bytes corrupted should fail.
 	 */
-	unsigned int esp_aad : 1;
+	unsigned int aad_iv : 1;
+	unsigned int aad_iv_auth : 1;
 };
 
 struct cipher_test_suite {
@@ -2167,9 +2173,12 @@ struct aead_extra_tests_ctx {
  * here means the full ciphertext including the authentication tag.  The
  * authentication tag (and hence also the ciphertext) is assumed to be nonempty.
  */
-static void mutate_aead_message(struct aead_testvec *vec, bool esp_aad)
+static void mutate_aead_message(struct aead_testvec *vec,
+				const struct aead_test_suite *suite,
+				unsigned int ivsize)
 {
-	const unsigned int aad_tail_size = esp_aad ? 8 : 0;
+	const unsigned int aad_tail_size =
+		(suite->aad_iv && !suite->aad_iv_auth) ? ivsize : 0;
 	const unsigned int authsize = vec->clen - vec->plen;
 
 	if (prandom_u32() % 2 == 0 && vec->alen > aad_tail_size) {
@@ -2207,6 +2216,8 @@ static void generate_aead_message(struct aead_request *req,
 
 	/* Generate the AAD. */
 	generate_random_bytes((u8 *)vec->assoc, vec->alen);
+	if (suite->aad_iv && vec->alen >= ivsize)
+		memcpy((u8 *)vec->assoc + vec->alen - ivsize, vec->iv, ivsize);
 
 	if (inauthentic && prandom_u32() % 2 == 0) {
 		/* Generate a random ciphertext. */
@@ -2242,7 +2253,7 @@ static void generate_aead_message(struct aead_request *req,
 		 * Mutate the authentic (ciphertext, AAD) pair to get an
 		 * inauthentic one.
 		 */
-		mutate_aead_message(vec, suite->esp_aad);
+		mutate_aead_message(vec, suite, ivsize);
 	}
 	vec->novrfy = 1;
 	if (suite->einval_allowed)
@@ -5229,7 +5240,7 @@ static const struct alg_test_desc alg_test_descs[] = {
 			.aead = {
 				____VECS(aes_gcm_rfc4106_tv_template),
 				.einval_allowed = 1,
-				.esp_aad = 1,
+				.aad_iv = 1,
 			}
 		}
 	}, {
@@ -5241,7 +5252,7 @@ static const struct alg_test_desc alg_test_descs[] = {
 			.aead = {
 				____VECS(aes_ccm_rfc4309_tv_template),
 				.einval_allowed = 1,
-				.esp_aad = 1,
+				.aad_iv = 1,
 			}
 		}
 	}, {
@@ -5252,6 +5263,8 @@ static const struct alg_test_desc alg_test_descs[] = {
 			.aead = {
 				____VECS(aes_gcm_rfc4543_tv_template),
 				.einval_allowed = 1,
+				.aad_iv = 1,
+				.aad_iv_auth = 1,
 			}
 		}
 	}, {
@@ -5267,7 +5280,7 @@ static const struct alg_test_desc alg_test_descs[] = {
 			.aead = {
 				____VECS(rfc7539esp_tv_template),
 				.einval_allowed = 1,
-				.esp_aad = 1,
+				.aad_iv = 1,
 			}
 		}
 	}, {

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

[Gilad Ben-Yossef]

Hi,

On Wed, Mar 4, 2020 at 2:06 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Mar 03, 2020 at 02:09:25PM +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 | 35 ++++++++++++++++++++++++++---------
> >  1 file changed, 26 insertions(+), 9 deletions(-)
> >
> > diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> > index 88f33c0efb23..379bd1c7dd5b 100644
> > --- a/crypto/testmgr.c
> > +++ b/crypto/testmgr.c
> > @@ -91,10 +91,16 @@ struct aead_test_suite {
> >       unsigned int einval_allowed : 1;
> >
> >       /*
> > -      * Set if the algorithm intentionally ignores the last 8 bytes of the
> > -      * AAD buffer during decryption.
> > +      * Set if the algorithm includes a copy of the IV (last 8 bytes)
> > +      * in the AAD buffer but does not include it in calculating the ICV
> >        */
> > -     unsigned int esp_aad : 1;
> > +     unsigned int skip_aad_iv : 1;
>
> "Authentication tag" would be easier to understand than "ICV" and would match
> the rest of the code.  "ICV" is an idiosyncrasy used in certain RFCs only.

Sure.

>
> > +
> > +     /*
> > +      * Set if the algorithm includes a copy of the IV (last 8 bytes)
> > +      * in the AAD buffer and does include it when calculating the ICV
> > +      */
> > +     unsigned int auth_aad_iv : 1;
> >  };
> >
> >  struct cipher_test_suite {
> > @@ -2167,14 +2173,20 @@ struct aead_extra_tests_ctx {
> >   * here means the full ciphertext including the authentication tag.  The
> >   * authentication tag (and hence also the ciphertext) is assumed to be nonempty.
> >   */
> > -static void mutate_aead_message(struct aead_testvec *vec, bool esp_aad)
> > +static void mutate_aead_message(struct aead_testvec *vec,
> > +                             const struct aead_test_suite *suite)
> >  {
> > -     const unsigned int aad_tail_size = esp_aad ? 8 : 0;
> > +     const unsigned int aad_ivsize = 8;
>
> We should use the algorithm's actual IV size instead of hard-coding 8 bytes.

Yes, I was following the original code example but I agree it would be
better to pass the IV size.

>
> > +     const unsigned int aad_tail_size = suite->skip_aad_iv ? aad_ivsize : 0;
> >       const unsigned int authsize = vec->clen - vec->plen;
> >
> >       if (prandom_u32() % 2 == 0 && vec->alen > aad_tail_size) {
> >                /* Mutate the AAD */
> >               flip_random_bit((u8 *)vec->assoc, vec->alen - aad_tail_size);
> > +             if (suite->auth_aad_iv)
> > +                     memcpy((u8 *)vec->iv,
> > +                            (vec->assoc + vec->alen - aad_ivsize),
> > +                            aad_ivsize);
>
> Why sync the IV copies here?  When 'auth_aad_iv', we assume the copy of the IV
> in the AAD (which was just corrupted) is authenticated.  So we already know that
> decryption should fail, regardless of the other IV copy.

Nope. We know there needs to be a copy of the IV in the AAD and we know the IV
should be included in calculating in the authentication tag. We don't know which
copy of the IV will be used by the implementation.

Case in point - the ccree driver actually currently uses the copy of
the IV passed via
req->iv for calculating the IV contribution to the authentication tag,
not the one in the AAD.

And what happens then if you don't do this copy than is that you get
an unexpected
decryption success where the test expects failure, because the driver
fed the HW the
none mutated copy of the IV from req->iv and not the mutated copy
found in the AAD.

> Also, the code doesn't currently mutate vec->iv for any AEAD.  So mutating it
> for one specific algorithm is a bit odd.  IMO, it would make more sense to do a
> separate patch later that mutates vec->iv for all AEADs.

That's fine, in that case we should avoid mutating either copies of
the IV at all
in the case of RFC 4543 just as we do with RFC 4106 and friends - as
indeed your patch
does.

> >               if (prandom_u32() % 2 == 0)
> >                       return;
> >       }
> > @@ -2208,6 +2220,10 @@ static void generate_aead_message(struct aead_request *req,
> >       /* Generate the AAD. */
> >       generate_random_bytes((u8 *)vec->assoc, vec->alen);
> >
> > +     if (suite->auth_aad_iv && (vec->alen > ivsize))
> > +             memcpy(((u8 *)vec->assoc + vec->alen - ivsize), vec->iv,
> > +                    ivsize);
>
> Shouldn't this be >= ivsize, not > ivsize?
Indeed.

> And doesn't the IV need to be synced
> in both the skip_aad_iv and auth_aad_iv cases?

Nope, because in the skip_aad_iv case we never mutate the IV, so no
point in copying.

>
> There are also unnecessary parentheses here; the memcpy() could be one line.
>
>
> How about the following patch instead?

Works for me.

Tested-by: Gilad Ben-Yossef <gilad@benyossef.com?

Thanks!
Gilad

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

[Eric Biggers]

On Wed, Mar 04, 2020 at 03:48:47PM +0200, Gilad Ben-Yossef wrote:
> 
> >
> > > +     const unsigned int aad_tail_size = suite->skip_aad_iv ? aad_ivsize : 0;
> > >       const unsigned int authsize = vec->clen - vec->plen;
> > >
> > >       if (prandom_u32() % 2 == 0 && vec->alen > aad_tail_size) {
> > >                /* Mutate the AAD */
> > >               flip_random_bit((u8 *)vec->assoc, vec->alen - aad_tail_size);
> > > +             if (suite->auth_aad_iv)
> > > +                     memcpy((u8 *)vec->iv,
> > > +                            (vec->assoc + vec->alen - aad_ivsize),
> > > +                            aad_ivsize);
> >
> > Why sync the IV copies here?  When 'auth_aad_iv', we assume the copy of the IV
> > in the AAD (which was just corrupted) is authenticated.  So we already know that
> > decryption should fail, regardless of the other IV copy.
> 
> Nope. We know there needs to be a copy of the IV in the AAD and we know the IV
> should be included in calculating in the authentication tag. We don't know which
> copy of the IV will be used by the implementation.
> 
> Case in point - the ccree driver actually currently uses the copy of
> the IV passed via
> req->iv for calculating the IV contribution to the authentication tag,
> not the one in the AAD.
> 
> And what happens then if you don't do this copy than is that you get
> an unexpected
> decryption success where the test expects failure, because the driver
> fed the HW the
> none mutated copy of the IV from req->iv and not the mutated copy
> found in the AAD.

Okay, well in that case I don't see any difference between the two flags.  This
is because changing the IV *must* affect the authentication tag for *any* AEAD
algorithm, otherwise it's not actually authenticated encryption.

So why don't we just use a single flag 'aad_iv' that's set on rfc4106, rfc4309,
rfc4543, and rfc7539esp?  This flag would mean that the last bytes of the AAD
buffer must contain another copy of the IV for the behavior to be well-defined.

> > > @@ -2208,6 +2220,10 @@ static void generate_aead_message(struct aead_request *req,
> > >       /* Generate the AAD. */
> > >       generate_random_bytes((u8 *)vec->assoc, vec->alen);
> > >
> > > +     if (suite->auth_aad_iv && (vec->alen > ivsize))
> > > +             memcpy(((u8 *)vec->assoc + vec->alen - ivsize), vec->iv,
> > > +                    ivsize);
> >
> > Shouldn't this be >= ivsize, not > ivsize?
> Indeed.
> 
> > And doesn't the IV need to be synced
> > in both the skip_aad_iv and auth_aad_iv cases?
> 
> Nope, because in the skip_aad_iv case we never mutate the IV, so no
> point in copying.

But even if the IV isn't mutated, both copies still need to be the same, right?
We seem to have concluded that the behavior should be implementation-defined
when they're different, so that's the logical consequence...

- Eric

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

[Eric Biggers]

On Wed, Mar 04, 2020 at 11:58:53AM -0800, Eric Biggers wrote:
> On Wed, Mar 04, 2020 at 03:48:47PM +0200, Gilad Ben-Yossef wrote:
> > 
> > >
> > > > +     const unsigned int aad_tail_size = suite->skip_aad_iv ? aad_ivsize : 0;
> > > >       const unsigned int authsize = vec->clen - vec->plen;
> > > >
> > > >       if (prandom_u32() % 2 == 0 && vec->alen > aad_tail_size) {
> > > >                /* Mutate the AAD */
> > > >               flip_random_bit((u8 *)vec->assoc, vec->alen - aad_tail_size);
> > > > +             if (suite->auth_aad_iv)
> > > > +                     memcpy((u8 *)vec->iv,
> > > > +                            (vec->assoc + vec->alen - aad_ivsize),
> > > > +                            aad_ivsize);
> > >
> > > Why sync the IV copies here?  When 'auth_aad_iv', we assume the copy of the IV
> > > in the AAD (which was just corrupted) is authenticated.  So we already know that
> > > decryption should fail, regardless of the other IV copy.
> > 
> > Nope. We know there needs to be a copy of the IV in the AAD and we know the IV
> > should be included in calculating in the authentication tag. We don't know which
> > copy of the IV will be used by the implementation.
> > 
> > Case in point - the ccree driver actually currently uses the copy of
> > the IV passed via
> > req->iv for calculating the IV contribution to the authentication tag,
> > not the one in the AAD.
> > 
> > And what happens then if you don't do this copy than is that you get
> > an unexpected
> > decryption success where the test expects failure, because the driver
> > fed the HW the
> > none mutated copy of the IV from req->iv and not the mutated copy
> > found in the AAD.
> 
> Okay, well in that case I don't see any difference between the two flags.  This
> is because changing the IV *must* affect the authentication tag for *any* AEAD
> algorithm, otherwise it's not actually authenticated encryption.
> 
> So why don't we just use a single flag 'aad_iv' that's set on rfc4106, rfc4309,
> rfc4543, and rfc7539esp?  This flag would mean that the last bytes of the AAD
> buffer must contain another copy of the IV for the behavior to be well-defined.
> 
> > > > @@ -2208,6 +2220,10 @@ static void generate_aead_message(struct aead_request *req,
> > > >       /* Generate the AAD. */
> > > >       generate_random_bytes((u8 *)vec->assoc, vec->alen);
> > > >
> > > > +     if (suite->auth_aad_iv && (vec->alen > ivsize))
> > > > +             memcpy(((u8 *)vec->assoc + vec->alen - ivsize), vec->iv,
> > > > +                    ivsize);
> > >
> > > Shouldn't this be >= ivsize, not > ivsize?
> > Indeed.
> > 
> > > And doesn't the IV need to be synced
> > > in both the skip_aad_iv and auth_aad_iv cases?
> > 
> > Nope, because in the skip_aad_iv case we never mutate the IV, so no
> > point in copying.
> 
> But even if the IV isn't mutated, both copies still need to be the same, right?
> We seem to have concluded that the behavior should be implementation-defined
> when they're different, so that's the logical consequence...
> 

I sent out new patches -- can you review and test them?
https://lkml.kernel.org/r/20200304224405.152829-1-ebiggers@kernel.org

Thanks,

- Eric