[PATCH 0/3] crypto: AEAD fuzz tests and doc improvement

[PATCH 0/3] crypto: AEAD fuzz tests and doc improvement

[Eric Biggers]

- Make the AEAD fuzz tests avoid implementation-defined behavior for
  rfc4106, rfc4309, rfc4543, and rfc7539esp.  This replaces
  "[PATCH v2] crypto: testmgr - sync both RFC4106 IV copies"

- Adjust the order of the AEAD fuzz tests to be more logical.

- Improve the documentation for the AEAD scatterlist layout.

(I was also going to add a patch that makes the inauthentic AEAD tests
start mutating the IVs, but it turns out that "ccm" needs special
handling so I've left that for later.)

Eric Biggers (3):
  crypto: testmgr - use consistent IV copies for AEADs that need it
  crypto: testmgr - do comparison tests before inauthentic input tests
  crypto: aead - improve documentation for scatterlist layout

 crypto/testmgr.c      | 28 +++++++++++++++----------
 include/crypto/aead.h | 48 ++++++++++++++++++++++++-------------------
 2 files changed, 44 insertions(+), 32 deletions(-)

-- 
2.25.1

[PATCH 1/3] crypto: testmgr - use consistent IV copies for AEADs that need it

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

rfc4543 was missing from the list of algorithms that may treat the end
of the AAD buffer specially.

Also, with rfc4106, rfc4309, rfc4543, and rfc7539esp, the end of the AAD
buffer is actually supposed to contain a second copy of the IV, and
we've concluded that if the IV copies don't match the behavior is
implementation-defined.  So, the fuzz tests can't easily test that case.

So, make the fuzz tests only use inputs where the two IV copies match.

Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
Fixes: 40153b10d91c ("crypto: testmgr - fuzz AEADs against their generic implementation")
Cc: Stephan Mueller <smueller@chronox.de>
Originally-from: Gilad Ben-Yossef <gilad@benyossef.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/testmgr.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 88f33c0efb23..0a10dbde27ef 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -91,10 +91,11 @@ 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 this algorithm requires that the IV be located at the end of
+	 * the AAD buffer, in addition to being given in the normal way.  The
+	 * behavior when the two IV copies differ is implementation-defined.
 	 */
-	unsigned int esp_aad : 1;
+	unsigned int aad_iv : 1;
 };
 
 struct cipher_test_suite {
@@ -2167,9 +2168,10 @@ 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, bool aad_iv,
+				unsigned int ivsize)
 {
-	const unsigned int aad_tail_size = esp_aad ? 8 : 0;
+	const unsigned int aad_tail_size = aad_iv ? ivsize : 0;
 	const unsigned int authsize = vec->clen - vec->plen;
 
 	if (prandom_u32() % 2 == 0 && vec->alen > aad_tail_size) {
@@ -2207,6 +2209,9 @@ 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)
+		/* Avoid implementation-defined behavior. */
+		memcpy((u8 *)vec->assoc + vec->alen - ivsize, vec->iv, ivsize);
 
 	if (inauthentic && prandom_u32() % 2 == 0) {
 		/* Generate a random ciphertext. */
@@ -2242,7 +2247,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->aad_iv, ivsize);
 	}
 	vec->novrfy = 1;
 	if (suite->einval_allowed)
@@ -5202,7 +5207,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,
 			}
 		}
 	}, {
@@ -5214,7 +5219,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,
 			}
 		}
 	}, {
@@ -5225,6 +5230,7 @@ static const struct alg_test_desc alg_test_descs[] = {
 			.aead = {
 				____VECS(aes_gcm_rfc4543_tv_template),
 				.einval_allowed = 1,
+				.aad_iv = 1,
 			}
 		}
 	}, {
@@ -5240,7 +5246,7 @@ static const struct alg_test_desc alg_test_descs[] = {
 			.aead = {
 				____VECS(rfc7539esp_tv_template),
 				.einval_allowed = 1,
-				.esp_aad = 1,
+				.aad_iv = 1,
 			}
 		}
 	}, {
-- 
2.25.1

[PATCH 2/3] crypto: testmgr - do comparison tests before inauthentic input tests

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Do test_aead_vs_generic_impl() before test_aead_inauthentic_inputs() so
that any differences with the generic driver are detected before getting
to the inauthentic input tests, which intentionally use only the driver
being tested (so that they run even if a generic driver is unavailable).

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/testmgr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 0a10dbde27ef..428a5f8bc80f 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -2512,11 +2512,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);
-- 
2.25.1

[PATCH 3/3] crypto: aead - improve documentation for scatterlist layout

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Properly document the scatterlist layout for AEAD ciphers.

Reported-by: Gilad Ben-Yossef <gilad@benyossef.com>
Cc: Stephan Mueller <smueller@chronox.de>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 include/crypto/aead.h | 48 ++++++++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/include/crypto/aead.h b/include/crypto/aead.h
index 1b3ebe8593c0..62c68550aab6 100644
--- a/include/crypto/aead.h
+++ b/include/crypto/aead.h
@@ -43,27 +43,33 @@
  *
  * Memory Structure:
  *
- * To support the needs of the most prominent user of AEAD ciphers, namely
- * IPSEC, the AEAD ciphers have a special memory layout the caller must adhere
- * to.
- *
- * The scatter list pointing to the input data must contain:
- *
- * * for RFC4106 ciphers, the concatenation of
- *   associated authentication data || IV || plaintext or ciphertext. Note, the
- *   same IV (buffer) is also set with the aead_request_set_crypt call. Note,
- *   the API call of aead_request_set_ad must provide the length of the AAD and
- *   the IV. The API call of aead_request_set_crypt only points to the size of
- *   the input plaintext or ciphertext.
- *
- * * for "normal" AEAD ciphers, the concatenation of
- *   associated authentication data || plaintext or ciphertext.
- *
- * It is important to note that if multiple scatter gather list entries form
- * the input data mentioned above, the first entry must not point to a NULL
- * buffer. If there is any potential where the AAD buffer can be NULL, the
- * calling code must contain a precaution to ensure that this does not result
- * in the first scatter gather list entry pointing to a NULL buffer.
+ * The source scatterlist must contain the concatenation of
+ * associated data || plaintext or ciphertext.
+ *
+ * The destination scatterlist has the same layout, except that the plaintext
+ * (resp. ciphertext) will grow (resp. shrink) by the authentication tag size
+ * during encryption (resp. decryption).
+ *
+ * In-place encryption/decryption is enabled by using the same scatterlist
+ * pointer for both the source and destination.
+ *
+ * Even in the out-of-place case, space must be reserved in the destination for
+ * the associated data, even though it won't be written to.  This makes the
+ * in-place and out-of-place cases more consistent.  It is permissible for the
+ * "destination" associated data to alias the "source" associated data.
+ *
+ * As with the other scatterlist crypto APIs, zero-length scatterlist elements
+ * are not allowed in the used part of the scatterlist.  Thus, if there is no
+ * associated data, the first element must point to the plaintext/ciphertext.
+ *
+ * To meet the needs of IPsec, a special quirk applies to rfc4106, rfc4309,
+ * rfc4543, and rfc7539esp ciphers.  For these ciphers, the final 'ivsize' bytes
+ * of the associated data buffer must contain a second copy of the IV.  This is
+ * in addition to the copy passed to aead_request_set_crypt().  These two IV
+ * copies must not differ; different implementations of the same algorithm may
+ * behave differently in that case.  Note that the algorithm might not actually
+ * treat the IV as associated data; nevertheless the length passed to
+ * aead_request_set_ad() must include it.
  */
 
 struct crypto_aead;
-- 
2.25.1

Re: [PATCH 0/3] crypto: AEAD fuzz tests and doc improvement

[Gilad Ben-Yossef]

On Thu, Mar 5, 2020 at 12:44 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> - Make the AEAD fuzz tests avoid implementation-defined behavior for
>   rfc4106, rfc4309, rfc4543, and rfc7539esp.  This replaces
>   "[PATCH v2] crypto: testmgr - sync both RFC4106 IV copies"
>
> - Adjust the order of the AEAD fuzz tests to be more logical.
>
> - Improve the documentation for the AEAD scatterlist layout.
>
> (I was also going to add a patch that makes the inauthentic AEAD tests
> start mutating the IVs, but it turns out that "ccm" needs special
> handling so I've left that for later.)

For the whole series:
Tested-by: Gilad Ben-Yossef <gilad@benyossef.com>

Thank you Eric!
Gilad

>
> Eric Biggers (3):
>   crypto: testmgr - use consistent IV copies for AEADs that need it
>   crypto: testmgr - do comparison tests before inauthentic input tests
>   crypto: aead - improve documentation for scatterlist layout
>
>  crypto/testmgr.c      | 28 +++++++++++++++----------
>  include/crypto/aead.h | 48 ++++++++++++++++++++++++-------------------
>  2 files changed, 44 insertions(+), 32 deletions(-)
>
> --
> 2.25.1
>


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

values of β will give rise to dom!