[PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing

[PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing

[Pascal van Leeuwen]

The probability of hitting specific input length corner cases relevant
for certain hardware driver(s) (specifically: inside-secure) was found
to be too low. Additionally, for authenc AEADs, the probability of
generating a *legal* key blob approached zero, i.e. most vectors
generated would only test the proper generation of a key blob error.

This patch address both of these issues by improving the random
generation of data lengths (for the key, but also for the ICV output
and the AAD and plaintext inputs), making the random generation
individually tweakable on a per-ciphersuite basis.

Finally, this patch enables fuzz testing for AEAD ciphersuites that do
not have a regular testsuite defined as it no longer depends on that
regular testsuite for figuring out the key size.

Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>
---
 crypto/testmgr.c | 269 +++++++++++++++++++++++++++++++++++++++++++------
 crypto/testmgr.h | 298 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 535 insertions(+), 32 deletions(-)

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 2ba0c48..9c856d3 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -84,11 +84,24 @@ int alg_test(const char *driver, const char *alg, u32 type, u32 mask)
 #define ENCRYPT 1
 #define DECRYPT 0
 
+struct len_range_set {
+	const struct len_range_sel *lensel;
+	unsigned int count;
+};
+
 struct aead_test_suite {
 	const struct aead_testvec *vecs;
 	unsigned int count;
 };
 
+struct aead_test_params {
+	struct len_range_set ckeylensel;
+	struct len_range_set akeylensel;
+	struct len_range_set authsizesel;
+	struct len_range_set aadlensel;
+	struct len_range_set ptxtlensel;
+};
+
 struct cipher_test_suite {
 	const struct cipher_testvec *vecs;
 	unsigned int count;
@@ -143,6 +156,10 @@ struct alg_test_desc {
 		struct akcipher_test_suite akcipher;
 		struct kpp_test_suite kpp;
 	} suite;
+
+	union {
+		struct aead_test_params aead;
+	} params;
 };
 
 static void hexdump(unsigned char *buf, unsigned int len)
@@ -189,9 +206,6 @@ static void testmgr_free_buf(char *buf[XBUFSIZE])
 	__testmgr_free_buf(buf, 0);
 }
 
-#define TESTMGR_POISON_BYTE	0xfe
-#define TESTMGR_POISON_LEN	16
-
 static inline void testmgr_poison(void *addr, size_t len)
 {
 	memset(addr, TESTMGR_POISON_BYTE, len);
@@ -2035,6 +2049,19 @@ static int test_aead_vec(const char *driver, int enc,
 }
 
 #ifdef CONFIG_CRYPTO_MANAGER_EXTRA_TESTS
+/* Select a random length value from a list of range specs */
+int random_lensel(const struct len_range_set *lens)
+{
+	u32 i, sel = prandom_u32() % 1000;
+
+	for (i = 0; i < lens->count; i++)
+		if (sel < lens->lensel[i].threshold)
+			return (prandom_u32() % (lens->lensel[i].len_hi  -
+						 lens->lensel[i].len_lo + 1)) +
+				lens->lensel[i].len_lo;
+	return -1;
+}
+
 /*
  * Generate an AEAD test vector from the given implementation.
  * Assumes the buffers in 'vec' were already allocated.
@@ -2043,44 +2070,83 @@ static void generate_random_aead_testvec(struct aead_request *req,
 					 struct aead_testvec *vec,
 					 unsigned int maxkeysize,
 					 unsigned int maxdatasize,
+					 const struct aead_test_params *lengths,
 					 char *name, size_t max_namelen)
 {
 	struct crypto_aead *tfm = crypto_aead_reqtfm(req);
 	const unsigned int ivsize = crypto_aead_ivsize(tfm);
 	unsigned int maxauthsize = crypto_aead_alg(tfm)->maxauthsize;
-	unsigned int authsize;
+	int authsize, clen, alen;
 	unsigned int total_len;
 	int i;
 	struct scatterlist src[2], dst;
 	u8 iv[MAX_IVLEN];
 	DECLARE_CRYPTO_WAIT(wait);
 
-	/* Key: length in [0, maxkeysize], but usually choose maxkeysize */
-	vec->klen = maxkeysize;
-	if (prandom_u32() % 4 == 0)
-		vec->klen = prandom_u32() % (maxkeysize + 1);
-	generate_random_bytes((u8 *)vec->key, vec->klen);
+	alen = random_lensel(&lengths->akeylensel);
+	clen = random_lensel(&lengths->ckeylensel);
+	if ((alen >= 0) && (clen >= 0)) {
+		/* Corect blob header. TBD: Do we care about corrupting this? */
+#ifdef __LITTLE_ENDIAN
+		memcpy((void *)vec->key, "\x08\x00\x01\x00\x00\x00\x00\x10", 8);
+#else
+		memcpy((void *)vec->key, "\x00\x08\x00\x01\x00\x00\x00\x10", 8);
+#endif
+
+		/* Generate keys based on length templates */
+		generate_random_bytes((u8 *)(vec->key + 8), alen);
+		generate_random_bytes((u8 *)(vec->key + 8 + alen),
+				      clen);
+
+		vec->klen = 8 + alen + clen;
+	} else {
+		if (clen >= 0)
+			maxkeysize = clen;
+
+		vec->klen = maxkeysize;
+
+		/*
+		 * Key: length in [0, maxkeysize],
+		 * but usually choose maxkeysize
+		 */
+		if (prandom_u32() % 4 == 0)
+			vec->klen = prandom_u32() % (maxkeysize + 1);
+		generate_random_bytes((u8 *)vec->key, vec->klen);
+	}
 	vec->setkey_error = crypto_aead_setkey(tfm, vec->key, vec->klen);
 
 	/* IV */
 	generate_random_bytes((u8 *)vec->iv, ivsize);
 
-	/* Tag length: in [0, maxauthsize], but usually choose maxauthsize */
-	authsize = maxauthsize;
-	if (prandom_u32() % 4 == 0)
-		authsize = prandom_u32() % (maxauthsize + 1);
+	authsize = random_lensel(&lengths->authsizesel);
+	if (authsize < 0) {
+		/*
+		 * Tag length: in [0, maxauthsize],
+		 * but usually choose maxauthsize
+		 */
+		authsize = maxauthsize;
+		if (prandom_u32() % 4 == 0)
+			authsize = prandom_u32() % (maxauthsize + 1);
+	}
 	if (WARN_ON(authsize > maxdatasize))
 		authsize = maxdatasize;
-	maxdatasize -= authsize;
 	vec->setauthsize_error = crypto_aead_setauthsize(tfm, authsize);
 
 	/* Plaintext and associated data */
-	total_len = generate_random_length(maxdatasize);
-	if (prandom_u32() % 4 == 0)
-		vec->alen = 0;
-	else
-		vec->alen = generate_random_length(total_len);
-	vec->plen = total_len - vec->alen;
+	alen = random_lensel(&lengths->aadlensel);
+	clen = random_lensel(&lengths->ptxtlensel);
+	maxdatasize -= authsize;
+	if ((alen < 0) || (clen < 0) || ((alen + clen) > maxdatasize)) {
+		total_len = generate_random_length(maxdatasize);
+		if (prandom_u32() % 4 == 0)
+			vec->alen = 0;
+		else
+			vec->alen = generate_random_length(total_len);
+		vec->plen = total_len - vec->alen;
+	} else {
+		vec->alen = alen;
+		vec->plen = clen;
+	}
 	generate_random_bytes((u8 *)vec->assoc, vec->alen);
 	generate_random_bytes((u8 *)vec->ptext, vec->plen);
 
@@ -2133,7 +2199,7 @@ static int test_aead_vs_generic_impl(const char *driver,
 	char _generic_driver[CRYPTO_MAX_ALG_NAME];
 	struct crypto_aead *generic_tfm = NULL;
 	struct aead_request *generic_req = NULL;
-	unsigned int maxkeysize;
+	unsigned int maxkeysize, maxakeysize;
 	unsigned int i;
 	struct aead_testvec vec = { 0 };
 	char vec_name[64];
@@ -2203,9 +2269,27 @@ static int test_aead_vs_generic_impl(const char *driver,
 	 */
 
 	maxkeysize = 0;
-	for (i = 0; i < test_desc->suite.aead.count; i++)
+	for (i = 0; i < test_desc->params.aead.ckeylensel.count; i++)
 		maxkeysize = max_t(unsigned int, maxkeysize,
-				   test_desc->suite.aead.vecs[i].klen);
+			test_desc->params.aead.ckeylensel.lensel[i].len_hi);
+
+	if (maxkeysize && test_desc->params.aead.akeylensel.count) {
+		/* authenc, explicit keylen ranges defined, use them */
+		maxakeysize = 0;
+		for (i = 0; i < test_desc->params.aead.akeylensel.count; i++)
+			maxakeysize = max_t(unsigned int, maxakeysize,
+			   test_desc->params.aead.akeylensel.lensel[i].len_hi);
+		maxkeysize = 8 + maxkeysize + maxakeysize;
+	} else if (!maxkeysize && test_desc->suite.aead.count) {
+		/* attempt to derive from test vectors */
+		for (i = 0; i < test_desc->suite.aead.count; i++)
+			maxkeysize = max_t(unsigned int, maxkeysize,
+					test_desc->suite.aead.vecs[i].klen);
+	} else {
+		pr_err("alg: aead: no key length templates or test vectors for %s - unable to fuzz\n",
+		       driver);
+		err = -EINVAL;
+	}
 
 	vec.key = kmalloc(maxkeysize, GFP_KERNEL);
 	vec.iv = kmalloc(ivsize, GFP_KERNEL);
@@ -2220,6 +2304,7 @@ static int test_aead_vs_generic_impl(const char *driver,
 	for (i = 0; i < fuzz_iterations * 8; i++) {
 		generate_random_aead_testvec(generic_req, &vec,
 					     maxkeysize, maxdatasize,
+					     &test_desc->params.aead,
 					     vec_name, sizeof(vec_name));
 		generate_random_testvec_config(&cfg, cfgname, sizeof(cfgname));
 
@@ -2280,11 +2365,6 @@ static int alg_test_aead(const struct alg_test_desc *desc, const char *driver,
 	struct cipher_test_sglists *tsgls = NULL;
 	int err;
 
-	if (suite->count <= 0) {
-		pr_err("alg: aead: empty test suite for %s\n", driver);
-		return -EINVAL;
-	}
-
 	tfm = crypto_alloc_aead(driver, type, mask);
 	if (IS_ERR(tfm)) {
 		pr_err("alg: aead: failed to allocate transform for %s: %ld\n",
@@ -2308,6 +2388,11 @@ static int alg_test_aead(const struct alg_test_desc *desc, const char *driver,
 		goto out;
 	}
 
+	if (suite->count <= 0) {
+		pr_err("alg: aead: empty test suite for %s\n", driver);
+		goto aead_skip_testsuite;
+	}
+
 	err = test_aead(driver, ENCRYPT, suite, req, tsgls);
 	if (err)
 		goto out;
@@ -2316,7 +2401,9 @@ static int alg_test_aead(const struct alg_test_desc *desc, const char *driver,
 	if (err)
 		goto out;
 
+aead_skip_testsuite:
 	err = test_aead_vs_generic_impl(driver, desc, req, tsgls);
+
 out:
 	free_cipher_test_sglists(tsgls);
 	aead_request_free(req);
@@ -3834,6 +3921,7 @@ static int alg_test_null(const struct alg_test_desc *desc,
 }
 
 #define __VECS(tv)	{ .vecs = tv, .count = ARRAY_SIZE(tv) }
+#define __LENS(ls)	{ .lensel = ls, .count = ARRAY_SIZE(ls) }
 
 /* Please keep this list sorted by algorithm name. */
 static const struct alg_test_desc alg_test_descs[] = {
@@ -3887,12 +3975,30 @@ static int alg_test_null(const struct alg_test_desc *desc,
 		.fips_allowed = 1,
 		.suite = {
 			.aead = __VECS(hmac_sha1_aes_cbc_tv_temp)
+		},
+		.params = {
+			.aead = {
+				.ckeylensel = __LENS(aes_klen_template),
+				.akeylensel = __LENS(sha1_klen_template),
+				.authsizesel = __LENS(sha1_alen_template),
+				.aadlensel = __LENS(aead_alen_template),
+				.ptxtlensel = __LENS(aead_plen_template),
+			}
 		}
 	}, {
 		.alg = "authenc(hmac(sha1),cbc(des))",
 		.test = alg_test_aead,
 		.suite = {
 			.aead = __VECS(hmac_sha1_des_cbc_tv_temp)
+		},
+		.params = {
+			.aead = {
+				.ckeylensel = __LENS(des_klen_template),
+				.akeylensel = __LENS(sha1_klen_template),
+				.authsizesel = __LENS(sha1_alen_template),
+				.aadlensel = __LENS(aead_alen_template),
+				.ptxtlensel = __LENS(aead_plen_template),
+			}
 		}
 	}, {
 		.alg = "authenc(hmac(sha1),cbc(des3_ede))",
@@ -3900,6 +4006,15 @@ static int alg_test_null(const struct alg_test_desc *desc,
 		.fips_allowed = 1,
 		.suite = {
 			.aead = __VECS(hmac_sha1_des3_ede_cbc_tv_temp)
+		},
+		.params = {
+			.aead = {
+				.ckeylensel = __LENS(des3_klen_template),
+				.akeylensel = __LENS(sha1_klen_template),
+				.authsizesel = __LENS(sha1_alen_template),
+				.aadlensel = __LENS(aead_alen_template),
+				.ptxtlensel = __LENS(aead_plen_template),
+			}
 		}
 	}, {
 		.alg = "authenc(hmac(sha1),ctr(aes))",
@@ -3913,8 +4028,29 @@ static int alg_test_null(const struct alg_test_desc *desc,
 		}
 	}, {
 		.alg = "authenc(hmac(sha1),rfc3686(ctr(aes)))",
-		.test = alg_test_null,
+		.test = alg_test_aead,
 		.fips_allowed = 1,
+		.params = {
+			.aead = {
+				.ckeylensel = __LENS(aes_klen_template),
+				.akeylensel = __LENS(sha1_klen_template),
+				.authsizesel = __LENS(sha1_alen_template),
+				.aadlensel = __LENS(aead_alen_template),
+				.ptxtlensel = __LENS(aead_plen_template),
+			}
+		}
+	}, {
+		.alg = "authenc(hmac(sha224),cbc(aes))",
+		.test = alg_test_aead,
+		.params = {
+			.aead = {
+				.ckeylensel = __LENS(aes_klen_template),
+				.akeylensel = __LENS(sha224_klen_template),
+				.authsizesel = __LENS(sha224_alen_template),
+				.aadlensel = __LENS(aead_alen_template),
+				.ptxtlensel = __LENS(aead_plen_template),
+			}
+		}
 	}, {
 		.alg = "authenc(hmac(sha224),cbc(des))",
 		.test = alg_test_aead,
@@ -3929,11 +4065,32 @@ static int alg_test_null(const struct alg_test_desc *desc,
 			.aead = __VECS(hmac_sha224_des3_ede_cbc_tv_temp)
 		}
 	}, {
+		.alg = "authenc(hmac(sha224),rfc3686(ctr(aes)))",
+		.test = alg_test_aead,
+		.params = {
+			.aead = {
+				.ckeylensel = __LENS(aes_klen_template),
+				.akeylensel = __LENS(sha224_klen_template),
+				.authsizesel = __LENS(sha224_alen_template),
+				.aadlensel = __LENS(aead_alen_template),
+				.ptxtlensel = __LENS(aead_plen_template),
+			}
+		}
+	}, {
 		.alg = "authenc(hmac(sha256),cbc(aes))",
 		.test = alg_test_aead,
 		.fips_allowed = 1,
 		.suite = {
 			.aead = __VECS(hmac_sha256_aes_cbc_tv_temp)
+		},
+		.params = {
+			.aead = {
+				.ckeylensel = __LENS(aes_klen_template),
+				.akeylensel = __LENS(sha256_klen_template),
+				.authsizesel = __LENS(sha256_alen_template),
+				.aadlensel = __LENS(aead_alen_template),
+				.ptxtlensel = __LENS(aead_plen_template),
+			}
 		}
 	}, {
 		.alg = "authenc(hmac(sha256),cbc(des))",
@@ -3954,8 +4111,29 @@ static int alg_test_null(const struct alg_test_desc *desc,
 		.fips_allowed = 1,
 	}, {
 		.alg = "authenc(hmac(sha256),rfc3686(ctr(aes)))",
-		.test = alg_test_null,
+		.test = alg_test_aead,
 		.fips_allowed = 1,
+		.params = {
+			.aead = {
+				.ckeylensel = __LENS(aes_klen_template),
+				.akeylensel = __LENS(sha256_klen_template),
+				.authsizesel = __LENS(sha256_alen_template),
+				.aadlensel = __LENS(aead_alen_template),
+				.ptxtlensel = __LENS(aead_plen_template),
+			}
+		}
+	}, {
+		.alg = "authenc(hmac(sha384),cbc(aes))",
+		.test = alg_test_aead,
+		.params = {
+			.aead = {
+				.ckeylensel = __LENS(aes_klen_template),
+				.akeylensel = __LENS(sha384_klen_template),
+				.authsizesel = __LENS(sha384_alen_template),
+				.aadlensel = __LENS(aead_alen_template),
+				.ptxtlensel = __LENS(aead_plen_template),
+			}
+		}
 	}, {
 		.alg = "authenc(hmac(sha384),cbc(des))",
 		.test = alg_test_aead,
@@ -3975,14 +4153,32 @@ static int alg_test_null(const struct alg_test_desc *desc,
 		.fips_allowed = 1,
 	}, {
 		.alg = "authenc(hmac(sha384),rfc3686(ctr(aes)))",
-		.test = alg_test_null,
+		.test = alg_test_aead,
 		.fips_allowed = 1,
+		.params = {
+			.aead = {
+				.ckeylensel = __LENS(aes_klen_template),
+				.akeylensel = __LENS(sha384_klen_template),
+				.authsizesel = __LENS(sha384_alen_template),
+				.aadlensel = __LENS(aead_alen_template),
+				.ptxtlensel = __LENS(aead_plen_template),
+			}
+		}
 	}, {
 		.alg = "authenc(hmac(sha512),cbc(aes))",
 		.fips_allowed = 1,
 		.test = alg_test_aead,
 		.suite = {
 			.aead = __VECS(hmac_sha512_aes_cbc_tv_temp)
+		},
+		.params = {
+			.aead = {
+				.ckeylensel = __LENS(aes_klen_template),
+				.akeylensel = __LENS(sha512_klen_template),
+				.authsizesel = __LENS(sha512_alen_template),
+				.aadlensel = __LENS(aead_alen_template),
+				.ptxtlensel = __LENS(aead_plen_template),
+			}
 		}
 	}, {
 		.alg = "authenc(hmac(sha512),cbc(des))",
@@ -4003,8 +4199,17 @@ static int alg_test_null(const struct alg_test_desc *desc,
 		.fips_allowed = 1,
 	}, {
 		.alg = "authenc(hmac(sha512),rfc3686(ctr(aes)))",
-		.test = alg_test_null,
+		.test = alg_test_aead,
 		.fips_allowed = 1,
+		.params = {
+			.aead = {
+				.ckeylensel = __LENS(aes_klen_template),
+				.akeylensel = __LENS(sha512_klen_template),
+				.authsizesel = __LENS(sha512_alen_template),
+				.aadlensel = __LENS(aead_alen_template),
+				.ptxtlensel = __LENS(aead_plen_template),
+			}
+		}
 	}, {
 		.alg = "cbc(aes)",
 		.test = alg_test_skcipher,
diff --git a/crypto/testmgr.h b/crypto/testmgr.h
index 6b459a6..105f2ce 100644
--- a/crypto/testmgr.h
+++ b/crypto/testmgr.h
@@ -28,6 +28,8 @@
 #include <linux/oid_registry.h>
 
 #define MAX_IVLEN		32
+#define TESTMGR_POISON_BYTE	0xfe
+#define TESTMGR_POISON_LEN	16
 
 /*
  * hash_testvec:	structure to describe a hash (message digest) test
@@ -176,6 +178,302 @@ struct kpp_testvec {
 static const char zeroed_string[48];
 
 /*
+ * length range declaration lo-hi plus selection threshold 0 - 1000
+ */
+struct len_range_sel {
+	unsigned int len_lo;
+	unsigned int len_hi;
+	unsigned int threshold;
+};
+
+/*
+ * List of length ranges sorted on increasing threshold
+ *
+ * 25% of each of the legal key sizes (128, 192, 256 bits)
+ * plus 25% of illegal sizes in between 0 and 1024 bits.
+ */
+static const struct len_range_sel aes_klen_template[] = {
+	{
+	.len_lo = 0,
+	.len_hi = 15,
+	.threshold = 25,
+	}, {
+	.len_lo = 16,
+	.len_hi = 16,
+	.threshold = 325,
+	}, {
+	.len_lo = 17,
+	.len_hi = 23,
+	.threshold = 350,
+	}, {
+	.len_lo = 24,
+	.len_hi = 24,
+	.threshold = 650,
+	}, {
+	.len_lo = 25,
+	.len_hi = 31,
+	.threshold = 675,
+	}, {
+	.len_lo = 32,
+	.len_hi = 32,
+	.threshold = 975,
+	}, {
+	.len_lo = 33,
+	.len_hi = 128,
+	.threshold = 1000,
+	}
+};
+
+/* 90% legal keys of size 8, rest illegal between 0 and 32 */
+static const struct len_range_sel des_klen_template[] = {
+	{
+	.len_lo = 0,
+	.len_hi = 7,
+	.threshold = 50,
+	}, {
+	.len_lo = 8,
+	.len_hi = 8,
+	.threshold = 950,
+	}, {
+	.len_lo = 9,
+	.len_hi = 32,
+	.threshold = 1000,
+	}
+};
+
+/* 90% legal keys of size 24, rest illegal between 0 and 32 */
+static const struct len_range_sel des3_klen_template[] = {
+	{
+	.len_lo = 0,
+	.len_hi = 23,
+	.threshold = 50,
+	}, {
+	.len_lo = 24,
+	.len_hi = 24,
+	.threshold = 950,
+	}, {
+	.len_lo = 25,
+	.len_hi = 32,
+	.threshold = 1000,
+	}
+};
+
+/*
+ * For HMAC's, favour the actual digest size for both key
+ * size and authenticator size, but do verify some tag
+ * truncation cases and some larger keys, including keys
+ * exceeding the block size.
+ */
+
+static const struct len_range_sel md5_klen_template[] = {
+	{
+	.len_lo = 0, /* Allow 0 here? */
+	.len_hi = 15,
+	.threshold = 50,
+	}, {
+	.len_lo = 16,
+	.len_hi = 16,
+	.threshold = 950,
+	}, {
+	.len_lo = 17,
+	.len_hi = 256,
+	.threshold = 1000,
+	}
+};
+
+static const struct len_range_sel md5_alen_template[] = {
+	{
+	.len_lo = 0, /* Allow 0 here? */
+	.len_hi = 15,
+	.threshold = 100,
+	}, {
+	.len_lo = 16,
+	.len_hi = 16,
+	.threshold = 1000,
+	}
+};
+
+static const struct len_range_sel sha1_klen_template[] = {
+	{
+	.len_lo = 0, /* Allow 0 here? */
+	.len_hi = 19,
+	.threshold = 50,
+	}, {
+	.len_lo = 20,
+	.len_hi = 20,
+	.threshold = 950,
+	}, {
+	.len_lo = 21,
+	.len_hi = 256,
+	.threshold = 1000,
+	}
+};
+
+static const struct len_range_sel sha1_alen_template[] = {
+	{
+	.len_lo = 0, /* Allow 0 here? */
+	.len_hi = 19,
+	.threshold = 100,
+	}, {
+	.len_lo = 20,
+	.len_hi = 20,
+	.threshold = 1000,
+	}
+};
+
+static const struct len_range_sel sha224_klen_template[] = {
+	{
+	.len_lo = 0, /* Allow 0 here? */
+	.len_hi = 23,
+	.threshold = 50,
+	}, {
+	.len_lo = 24,
+	.len_hi = 24,
+	.threshold = 950,
+	}, {
+	.len_lo = 25,
+	.len_hi = 256,
+	.threshold = 1000,
+	}
+};
+
+static const struct len_range_sel sha224_alen_template[] = {
+	{
+	.len_lo = 0, /* Allow 0 here? */
+	.len_hi = 23,
+	.threshold = 100,
+	}, {
+	.len_lo = 24,
+	.len_hi = 24,
+	.threshold = 1000,
+	}
+};
+
+static const struct len_range_sel sha256_klen_template[] = {
+	{
+	.len_lo = 0, /* Allow 0 here? */
+	.len_hi = 31,
+	.threshold = 50,
+	}, {
+	.len_lo = 32,
+	.len_hi = 32,
+	.threshold = 950,
+	}, {
+	.len_lo = 33,
+	.len_hi = 256,
+	.threshold = 1000,
+	}
+};
+
+static const struct len_range_sel sha256_alen_template[] = {
+	{
+	.len_lo = 0, /* Allow 0 here? */
+	.len_hi = 31,
+	.threshold = 100,
+	}, {
+	.len_lo = 32,
+	.len_hi = 32,
+	.threshold = 1000,
+	}
+};
+
+static const struct len_range_sel sha384_klen_template[] = {
+	{
+	.len_lo = 0, /* Allow 0 here? */
+	.len_hi = 47,
+	.threshold = 50,
+	}, {
+	.len_lo = 48,
+	.len_hi = 48,
+	.threshold = 950,
+	}, {
+	.len_lo = 49,
+	.len_hi = 256,
+	.threshold = 1000,
+	}
+};
+
+static const struct len_range_sel sha384_alen_template[] = {
+	{
+	.len_lo = 0, /* Allow 0 here? */
+	.len_hi = 47,
+	.threshold = 100,
+	}, {
+	.len_lo = 48,
+	.len_hi = 48,
+	.threshold = 1000,
+	}
+};
+
+static const struct len_range_sel sha512_klen_template[] = {
+	{
+	.len_lo = 0, /* Allow 0 here? */
+	.len_hi = 63,
+	.threshold = 50,
+	}, {
+	.len_lo = 64,
+	.len_hi = 64,
+	.threshold = 950,
+	}, {
+	.len_lo = 65,
+	.len_hi = 256,
+	.threshold = 1000,
+	}
+};
+
+static const struct len_range_sel sha512_alen_template[] = {
+	{
+	.len_lo = 0, /* Allow 0 here? */
+	.len_hi = 63,
+	.threshold = 100,
+	}, {
+	.len_lo = 64,
+	.len_hi = 64,
+	.threshold = 1000,
+	}
+};
+
+static const struct len_range_sel aead_alen_template[] = {
+	{
+	.len_lo = 0,
+	.len_hi = 0,
+	.threshold = 200,
+	}, {
+	.len_lo = 1,
+	.len_hi = 32,
+	.threshold = 900,
+	}, {
+	.len_lo = 33,
+	.len_hi = (2 * PAGE_SIZE) - TESTMGR_POISON_LEN,
+	.threshold = 1000,
+	}
+};
+
+static const struct len_range_sel aead_plen_template[] = {
+	{
+	.len_lo = 0,
+	.len_hi = 0,
+	.threshold = 200,
+	}, {
+	.len_lo = 1,
+	.len_hi = 63,
+	.threshold = 400,
+	}, {
+	.len_lo = 64,
+	.len_hi = 255,
+	.threshold = 600,
+	}, {
+	.len_lo = 256,
+	.len_hi = 1023,
+	.threshold = 800,
+	}, {
+	.len_lo = 1024,
+	.len_hi = (2 * PAGE_SIZE) - TESTMGR_POISON_LEN,
+	.threshold = 1000,
+	}
+};
+
+/*
  * RSA test vectors. Borrowed from openSSL.
  */
 static const struct akcipher_testvec rsa_tv_template[] = {
-- 
1.8.3.1

Re: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing

[Eric Biggers]

Hi Pascal, thanks for the patch!

On Wed, Jul 24, 2019 at 11:35:17AM +0200, Pascal van Leeuwen wrote:
> The probability of hitting specific input length corner cases relevant
> for certain hardware driver(s) (specifically: inside-secure) was found
> to be too low. Additionally, for authenc AEADs, the probability of
> generating a *legal* key blob approached zero, i.e. most vectors
> generated would only test the proper generation of a key blob error.
> 
> This patch address both of these issues by improving the random
> generation of data lengths (for the key, but also for the ICV output
> and the AAD and plaintext inputs), making the random generation
> individually tweakable on a per-ciphersuite basis.
> 
> Finally, this patch enables fuzz testing for AEAD ciphersuites that do
> not have a regular testsuite defined as it no longer depends on that
> regular testsuite for figuring out the key size.
> 
> Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>

More comments below, but first I see some test failures and warnings with this
patch applied:

alg: aead: authenc(hmac(sha1-generic),cbc-aes-aesni) decryption failed on test vector "random: alen=26 plen=594 authsize=20 klen=60"; expected_error=-22, actual_error=-74, cfg="random: may_sleep use_digest src_divs=[33.6%@+12, 18.23%@+2452, 13.7%@+3761, 35.64%@+4019] iv_offset=43"
alg: aead: empty test suite for authenc(hmac(sha1-ni),rfc3686(ctr(aes-generic)))
alg: aead: empty test suite for authenc(hmac(sha1-generic),rfc3686(ctr(aes-generic)))
alg: aead: authenc(hmac(sha256-generic),cbc-aes-aesni) decryption failed on test vector "random: alen=14 plen=6237 authsize=32 klen=72"; expected_error=-22, actual_error=-74, cfg="random: may_sleep use_digest src_divs=[93.90%@+2019, 4.74%@alignmask+4094, 1.36%@+21] dst_divs=[100.0%@+4000]"
alg: aead: empty test suite for authenc(hmac(sha256-ni),rfc3686(ctr-aes-aesni))
alg: aead: empty test suite for authenc(hmac(sha256-generic),rfc3686(ctr(aes-generic)))
alg: aead: empty test suite for authenc(hmac(sha384-avx2),rfc3686(ctr-aes-aesni))
alg: aead: empty test suite for authenc(hmac(sha384-generic),rfc3686(ctr(aes-generic)))
alg: aead: authenc(hmac(sha512-generic),cbc-aes-aesni) decryption failed on test vector "random: alen=14 plen=406 authsize=12 klen=104"; expected_error=-22, actual_error=-74, cfg="random: may_sleep use_digest src_divs=[41.41%@+852, 58.59%@+4011] dst_divs=[100.0%@alignmask+4017]"
alg: aead: empty test suite for authenc(hmac(sha512-avx2),rfc3686(ctr-aes-aesni))
alg: aead: empty test suite for authenc(hmac(sha512-generic),rfc3686(ctr(aes-generic)))


Note that the "empty test suite" message shouldn't be printed (especially not at
KERN_ERR level!) if it's working as intended.

> ---
>  crypto/testmgr.c | 269 +++++++++++++++++++++++++++++++++++++++++++------
>  crypto/testmgr.h | 298 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 535 insertions(+), 32 deletions(-)
> 
> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> index 2ba0c48..9c856d3 100644
> --- a/crypto/testmgr.c
> +++ b/crypto/testmgr.c
> @@ -84,11 +84,24 @@ int alg_test(const char *driver, const char *alg, u32 type, u32 mask)
>  #define ENCRYPT 1
>  #define DECRYPT 0
>  
> +struct len_range_set {
> +	const struct len_range_sel *lensel;
> +	unsigned int count;
> +};
> +
>  struct aead_test_suite {
>  	const struct aead_testvec *vecs;
>  	unsigned int count;
>  };
>  
> +struct aead_test_params {
> +	struct len_range_set ckeylensel;
> +	struct len_range_set akeylensel;
> +	struct len_range_set authsizesel;
> +	struct len_range_set aadlensel;
> +	struct len_range_set ptxtlensel;
> +};
> +
>  struct cipher_test_suite {
>  	const struct cipher_testvec *vecs;
>  	unsigned int count;
> @@ -143,6 +156,10 @@ struct alg_test_desc {
>  		struct akcipher_test_suite akcipher;
>  		struct kpp_test_suite kpp;
>  	} suite;
> +
> +	union {
> +		struct aead_test_params aead;
> +	} params;
>  };

Why not put these new fields in the existing 'struct aead_test_suite'?

I don't see the point of the separate 'params' struct.  It just confuses things.

>  
>  static void hexdump(unsigned char *buf, unsigned int len)
> @@ -189,9 +206,6 @@ static void testmgr_free_buf(char *buf[XBUFSIZE])
>  	__testmgr_free_buf(buf, 0);
>  }
>  
> -#define TESTMGR_POISON_BYTE	0xfe
> -#define TESTMGR_POISON_LEN	16
> -
>  static inline void testmgr_poison(void *addr, size_t len)
>  {
>  	memset(addr, TESTMGR_POISON_BYTE, len);
> @@ -2035,6 +2049,19 @@ static int test_aead_vec(const char *driver, int enc,
>  }
>  
>  #ifdef CONFIG_CRYPTO_MANAGER_EXTRA_TESTS
> +/* Select a random length value from a list of range specs */

Perhaps mention the meaning of the -1 return value in this comment?

> +int random_lensel(const struct len_range_set *lens)

static

> +{
> +	u32 i, sel = prandom_u32() % 1000;
> +
> +	for (i = 0; i < lens->count; i++)
> +		if (sel < lens->lensel[i].threshold)
> +			return (prandom_u32() % (lens->lensel[i].len_hi  -
> +						 lens->lensel[i].len_lo + 1)) +
> +				lens->lensel[i].len_lo;
> +	return -1;
> +}

This function isn't really AEAD-specific, so it seems it should be moved to near
the other common fuzz test helpers like generate_random_length(), rather than be
here where the AEAD-specific code is.

> +
>  /*
>   * Generate an AEAD test vector from the given implementation.
>   * Assumes the buffers in 'vec' were already allocated.
> @@ -2043,44 +2070,83 @@ static void generate_random_aead_testvec(struct aead_request *req,
>  					 struct aead_testvec *vec,
>  					 unsigned int maxkeysize,
>  					 unsigned int maxdatasize,
> +					 const struct aead_test_params *lengths,
>  					 char *name, size_t max_namelen)
>  {
>  	struct crypto_aead *tfm = crypto_aead_reqtfm(req);
>  	const unsigned int ivsize = crypto_aead_ivsize(tfm);
>  	unsigned int maxauthsize = crypto_aead_alg(tfm)->maxauthsize;
> -	unsigned int authsize;
> +	int authsize, clen, alen;
>  	unsigned int total_len;
>  	int i;
>  	struct scatterlist src[2], dst;
>  	u8 iv[MAX_IVLEN];
>  	DECLARE_CRYPTO_WAIT(wait);
>  
> -	/* Key: length in [0, maxkeysize], but usually choose maxkeysize */
> -	vec->klen = maxkeysize;
> -	if (prandom_u32() % 4 == 0)
> -		vec->klen = prandom_u32() % (maxkeysize + 1);
> -	generate_random_bytes((u8 *)vec->key, vec->klen);
> +	alen = random_lensel(&lengths->akeylensel);
> +	clen = random_lensel(&lengths->ckeylensel);
> +	if ((alen >= 0) && (clen >= 0)) {
> +		/* Corect blob header. TBD: Do we care about corrupting this? */
> +#ifdef __LITTLE_ENDIAN
> +		memcpy((void *)vec->key, "\x08\x00\x01\x00\x00\x00\x00\x10", 8);
> +#else
> +		memcpy((void *)vec->key, "\x00\x08\x00\x01\x00\x00\x00\x10", 8);
> +#endif

Isn't this specific to "authenc" AEADs?  There needs to be something in the test
suite that says an authenc formatted key is needed.

> +
> +		/* Generate keys based on length templates */
> +		generate_random_bytes((u8 *)(vec->key + 8), alen);
> +		generate_random_bytes((u8 *)(vec->key + 8 + alen),
> +				      clen);
> +
> +		vec->klen = 8 + alen + clen;
> +	} else {
> +		if (clen >= 0)
> +			maxkeysize = clen;
> +
> +		vec->klen = maxkeysize;
> +
> +		/*
> +		 * Key: length in [0, maxkeysize],
> +		 * but usually choose maxkeysize
> +		 */
> +		if (prandom_u32() % 4 == 0)
> +			vec->klen = prandom_u32() % (maxkeysize + 1);
> +		generate_random_bytes((u8 *)vec->key, vec->klen);
> +	}
>  	vec->setkey_error = crypto_aead_setkey(tfm, vec->key, vec->klen);

The generate_random_aead_testvec() function is getting too long and complicated.
It doesn't help that now the 'clen' variable is used for multiple different
purposes (encryption key length and ciphertext length).  Could you maybe factor
out the key generation into a separate function?

>  
>  	/* IV */
>  	generate_random_bytes((u8 *)vec->iv, ivsize);
>  
> -	/* Tag length: in [0, maxauthsize], but usually choose maxauthsize */
> -	authsize = maxauthsize;
> -	if (prandom_u32() % 4 == 0)
> -		authsize = prandom_u32() % (maxauthsize + 1);
> +	authsize = random_lensel(&lengths->authsizesel);
> +	if (authsize < 0) {
> +		/*
> +		 * Tag length: in [0, maxauthsize],
> +		 * but usually choose maxauthsize
> +		 */
> +		authsize = maxauthsize;
> +		if (prandom_u32() % 4 == 0)
> +			authsize = prandom_u32() % (maxauthsize + 1);
> +	}
>  	if (WARN_ON(authsize > maxdatasize))
>  		authsize = maxdatasize;
> -	maxdatasize -= authsize;
>  	vec->setauthsize_error = crypto_aead_setauthsize(tfm, authsize);

Updating the comments to better match the code changes would be helpful too.
E.g. here comments like the following would be helpful:

        /* Authentication tag size */
        authsize = random_lensel(&lengths->authsizesel);
        if (authsize < 0) {
                /*
                 * No length hints for this algorithm.  Fall back to a random
                 * value in [0, maxauthsize], but usually choose maxauthsize.
                 */
                authsize = maxauthsize;
                if (prandom_u32() % 4 == 0)
                        authsize = prandom_u32() % (maxauthsize + 1);
        }

>  
>  	/* Plaintext and associated data */
> -	total_len = generate_random_length(maxdatasize);
> -	if (prandom_u32() % 4 == 0)
> -		vec->alen = 0;
> -	else
> -		vec->alen = generate_random_length(total_len);
> -	vec->plen = total_len - vec->alen;
> +	alen = random_lensel(&lengths->aadlensel);
> +	clen = random_lensel(&lengths->ptxtlensel);
> +	maxdatasize -= authsize;
> +	if ((alen < 0) || (clen < 0) || ((alen + clen) > maxdatasize)) {
> +		total_len = generate_random_length(maxdatasize);
> +		if (prandom_u32() % 4 == 0)
> +			vec->alen = 0;
> +		else
> +			vec->alen = generate_random_length(total_len);
> +		vec->plen = total_len - vec->alen;
> +	} else {
> +		vec->alen = alen;
> +		vec->plen = clen;
> +	}
>  	generate_random_bytes((u8 *)vec->assoc, vec->alen);
>  	generate_random_bytes((u8 *)vec->ptext, vec->plen);
>  
> @@ -2133,7 +2199,7 @@ static int test_aead_vs_generic_impl(const char *driver,
>  	char _generic_driver[CRYPTO_MAX_ALG_NAME];
>  	struct crypto_aead *generic_tfm = NULL;
>  	struct aead_request *generic_req = NULL;
> -	unsigned int maxkeysize;
> +	unsigned int maxkeysize, maxakeysize;
>  	unsigned int i;
>  	struct aead_testvec vec = { 0 };
>  	char vec_name[64];
> @@ -2203,9 +2269,27 @@ static int test_aead_vs_generic_impl(const char *driver,
>  	 */
>  
>  	maxkeysize = 0;
> -	for (i = 0; i < test_desc->suite.aead.count; i++)
> +	for (i = 0; i < test_desc->params.aead.ckeylensel.count; i++)
>  		maxkeysize = max_t(unsigned int, maxkeysize,
> -				   test_desc->suite.aead.vecs[i].klen);
> +			test_desc->params.aead.ckeylensel.lensel[i].len_hi);
> +
> +	if (maxkeysize && test_desc->params.aead.akeylensel.count) {
> +		/* authenc, explicit keylen ranges defined, use them */
> +		maxakeysize = 0;
> +		for (i = 0; i < test_desc->params.aead.akeylensel.count; i++)
> +			maxakeysize = max_t(unsigned int, maxakeysize,
> +			   test_desc->params.aead.akeylensel.lensel[i].len_hi);
> +		maxkeysize = 8 + maxkeysize + maxakeysize;
> +	} else if (!maxkeysize && test_desc->suite.aead.count) {
> +		/* attempt to derive from test vectors */
> +		for (i = 0; i < test_desc->suite.aead.count; i++)
> +			maxkeysize = max_t(unsigned int, maxkeysize,
> +					test_desc->suite.aead.vecs[i].klen);
> +	} else {
> +		pr_err("alg: aead: no key length templates or test vectors for %s - unable to fuzz\n",
> +		       driver);
> +		err = -EINVAL;
> +	}

Can you put the "find the maxkeysize" logic in its own function, so it doesn't
make test_aead_vs_generic_impl() longer and harder to understand?

>  
>  	vec.key = kmalloc(maxkeysize, GFP_KERNEL);
>  	vec.iv = kmalloc(ivsize, GFP_KERNEL);
> @@ -2220,6 +2304,7 @@ static int test_aead_vs_generic_impl(const char *driver,
>  	for (i = 0; i < fuzz_iterations * 8; i++) {
>  		generate_random_aead_testvec(generic_req, &vec,
>  					     maxkeysize, maxdatasize,
> +					     &test_desc->params.aead,
>  					     vec_name, sizeof(vec_name));
>  		generate_random_testvec_config(&cfg, cfgname, sizeof(cfgname));
>  
> @@ -2280,11 +2365,6 @@ static int alg_test_aead(const struct alg_test_desc *desc, const char *driver,
>  	struct cipher_test_sglists *tsgls = NULL;
>  	int err;
>  
> -	if (suite->count <= 0) {
> -		pr_err("alg: aead: empty test suite for %s\n", driver);
> -		return -EINVAL;
> -	}
> -
>  	tfm = crypto_alloc_aead(driver, type, mask);
>  	if (IS_ERR(tfm)) {
>  		pr_err("alg: aead: failed to allocate transform for %s: %ld\n",
> @@ -2308,6 +2388,11 @@ static int alg_test_aead(const struct alg_test_desc *desc, const char *driver,
>  		goto out;
>  	}
>  
> +	if (suite->count <= 0) {
> +		pr_err("alg: aead: empty test suite for %s\n", driver);
> +		goto aead_skip_testsuite;
> +	}
> +

This should be at most pr_warn(), and maybe not printed at all.  We already know
that some authenc compositions don't have their own test vectors; kernel users
can't do anything about it.

> diff --git a/crypto/testmgr.h b/crypto/testmgr.h
> index 6b459a6..105f2ce 100644
> --- a/crypto/testmgr.h
> +++ b/crypto/testmgr.h
> @@ -28,6 +28,8 @@
>  #include <linux/oid_registry.h>
>  
>  #define MAX_IVLEN		32
> +#define TESTMGR_POISON_BYTE	0xfe
> +#define TESTMGR_POISON_LEN	16
>  
>  /*
>   * hash_testvec:	structure to describe a hash (message digest) test
> @@ -176,6 +178,302 @@ struct kpp_testvec {
>  static const char zeroed_string[48];
>  
>  /*
> + * length range declaration lo-hi plus selection threshold 0 - 1000
> + */
> +struct len_range_sel {
> +	unsigned int len_lo;
> +	unsigned int len_hi;
> +	unsigned int threshold;
> +};
> +
> +/*
> + * List of length ranges sorted on increasing threshold
> + *
> + * 25% of each of the legal key sizes (128, 192, 256 bits)
> + * plus 25% of illegal sizes in between 0 and 1024 bits.
> + */
> +static const struct len_range_sel aes_klen_template[] = {
> +	{
> +	.len_lo = 0,
> +	.len_hi = 15,
> +	.threshold = 25,
> +	}, {
> +	.len_lo = 16,
> +	.len_hi = 16,
> +	.threshold = 325,
> +	}, {
> +	.len_lo = 17,
> +	.len_hi = 23,
> +	.threshold = 350,
> +	}, {
> +	.len_lo = 24,
> +	.len_hi = 24,
> +	.threshold = 650,
> +	}, {
> +	.len_lo = 25,
> +	.len_hi = 31,
> +	.threshold = 675,
> +	}, {
> +	.len_lo = 32,
> +	.len_hi = 32,
> +	.threshold = 975,
> +	}, {
> +	.len_lo = 33,
> +	.len_hi = 128,
> +	.threshold = 1000,
> +	}
> +};

Can you please move these to be next to the test vectors for each algorithm, so
things are kept in one place for each algorithm?

Also, perhaps these should use the convention '.proportion_of_total', like
'struct testvec_config' already does, rather than '.threshold'?  That would be
more consistent with the existing test code, and would also make it slightly
easier to change the probabilities later.

E.g. if someone wanted to increase the probability of the first case and
decrease the probability of the last case, then with the '.threshold' convention
they'd have to change for every entry, but with the '.proportion_of_total'
convention they'd only have to change the first and last entries.

> +
> +/* 90% legal keys of size 8, rest illegal between 0 and 32 */
> +static const struct len_range_sel des_klen_template[] = {
> +	{
> +	.len_lo = 0,
> +	.len_hi = 7,
> +	.threshold = 50,
> +	}, {
> +	.len_lo = 8,
> +	.len_hi = 8,
> +	.threshold = 950,
> +	}, {
> +	.len_lo = 9,
> +	.len_hi = 32,
> +	.threshold = 1000,
> +	}
> +};
> +
> +/* 90% legal keys of size 24, rest illegal between 0 and 32 */
> +static const struct len_range_sel des3_klen_template[] = {
> +	{
> +	.len_lo = 0,
> +	.len_hi = 23,
> +	.threshold = 50,
> +	}, {
> +	.len_lo = 24,
> +	.len_hi = 24,
> +	.threshold = 950,
> +	}, {
> +	.len_lo = 25,
> +	.len_hi = 32,
> +	.threshold = 1000,
> +	}
> +};
> +
> +/*
> + * For HMAC's, favour the actual digest size for both key
> + * size and authenticator size, but do verify some tag
> + * truncation cases and some larger keys, including keys
> + * exceeding the block size.
> + */
> +
> +static const struct len_range_sel md5_klen_template[] = {

This really should be called "hmac_md5_klen", since md5 by itself is unkeyed.

Likewise for sha1, sha256, etc.

> +	{
> +	.len_lo = 0, /* Allow 0 here? */
> +	.len_hi = 15,
> +	.threshold = 50,
> +	}, {
> +	.len_lo = 16,
> +	.len_hi = 16,
> +	.threshold = 950,
> +	}, {
> +	.len_lo = 17,
> +	.len_hi = 256,
> +	.threshold = 1000,
> +	}
> +};
> +
> +static const struct len_range_sel md5_alen_template[] = {

Likewise here: it should be "hmac_md5".

Also, "alen" is used elsewhere in the test code, including in the test vectors
themselves, to mean the associated data length, *not* the authentication tag
length.  But here these lengths are for authentication tags.  So please use
"authsize".  Likewise for sha1, sha256, etc.

> +
> +static const struct len_range_sel aead_alen_template[] = {
> +	{
> +	.len_lo = 0,
> +	.len_hi = 0,
> +	.threshold = 200,
> +	}, {
> +	.len_lo = 1,
> +	.len_hi = 32,
> +	.threshold = 900,
> +	}, {
> +	.len_lo = 33,
> +	.len_hi = (2 * PAGE_SIZE) - TESTMGR_POISON_LEN,
> +	.threshold = 1000,
> +	}
> +};

Is this for authenc or for all AEADs?  Likewise for aead_plen_template[].

Again, "alen" => "authsize".

- Eric

RE: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing

[Pascal Van Leeuwen]

Hi Eric,

Thanks for your feedback!

> -----Original Message-----
> From: Eric Biggers <ebiggers@kernel.org>
> Sent: Sunday, July 28, 2019 7:31 PM
> To: Pascal van Leeuwen <pascalvanl@gmail.com>
> Cc: linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net; Pascal Van Leeuwen
> <pvanleeuwen@verimatrix.com>
> Subject: Re: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing
> 
> Hi Pascal, thanks for the patch!
> 
> On Wed, Jul 24, 2019 at 11:35:17AM +0200, Pascal van Leeuwen wrote:
> > The probability of hitting specific input length corner cases relevant
> > for certain hardware driver(s) (specifically: inside-secure) was found
> > to be too low. Additionally, for authenc AEADs, the probability of
> > generating a *legal* key blob approached zero, i.e. most vectors
> > generated would only test the proper generation of a key blob error.
> >
> > This patch address both of these issues by improving the random
> > generation of data lengths (for the key, but also for the ICV output
> > and the AAD and plaintext inputs), making the random generation
> > individually tweakable on a per-ciphersuite basis.
> >
> > Finally, this patch enables fuzz testing for AEAD ciphersuites that do
> > not have a regular testsuite defined as it no longer depends on that
> > regular testsuite for figuring out the key size.
> >
> > Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>
> 
> More comments below, but first I see some test failures and warnings with this
> patch applied:
> 
> alg: aead: authenc(hmac(sha1-generic),cbc-aes-aesni) decryption failed on test vector "random: alen=26 plen=594 authsize=20 klen=60";
> expected_error=-22, actual_error=-74, cfg="random: may_sleep use_digest src_divs=[33.6%@+12, 18.23%@+2452, 13.7%@+3761,
> 35.64%@+4019] iv_offset=43"
>
Interesting as the inside-secure driver also advertises this ciphersuite and does not generate such 
an error.  My guess is you get an error here because plen is not a multiple of 16 and this is CBC
(note to self: for block ciphers, emphasize legal lengths in the randomization ...), but the generic
implementation returns -EINVAL while this ciphersuite returns -EBADMSG.
Don't ask me what the actual correct error is in this case, I'm following generic with my driver.

> alg: aead: empty test suite for authenc(hmac(sha1-ni),rfc3686(ctr(aes-generic)))
> alg: aead: empty test suite for authenc(hmac(sha1-generic),rfc3686(ctr(aes-generic)))
> alg: aead: authenc(hmac(sha256-generic),cbc-aes-aesni) decryption failed on test vector "random: alen=14 plen=6237 authsize=32
> klen=72"; expected_error=-22, actual_error=-74, cfg="random: may_sleep use_digest src_divs=[93.90%@+2019, 4.74%@alignmask+4094,
> 1.36%@+21] dst_divs=[100.0%@+4000]"
>
Idem

> alg: aead: empty test suite for authenc(hmac(sha256-ni),rfc3686(ctr-aes-aesni))
> alg: aead: empty test suite for authenc(hmac(sha256-generic),rfc3686(ctr(aes-generic)))
> alg: aead: empty test suite for authenc(hmac(sha384-avx2),rfc3686(ctr-aes-aesni))
> alg: aead: empty test suite for authenc(hmac(sha384-generic),rfc3686(ctr(aes-generic)))

> alg: aead: authenc(hmac(sha512-generic),cbc-aes-aesni) decryption failed on test vector "random: alen=14 plen=406 authsize=12
> klen=104"; expected_error=-22, actual_error=-74, cfg="random: may_sleep use_digest src_divs=[41.41%@+852, 58.59%@+4011]
> dst_divs=[100.0%@alignmask+4017]"
>
Idem

> alg: aead: empty test suite for authenc(hmac(sha512-avx2),rfc3686(ctr-aes-aesni))
> alg: aead: empty test suite for authenc(hmac(sha512-generic),rfc3686(ctr(aes-generic)))
> 
> 
> Note that the "empty test suite" message shouldn't be printed (especially not at
> KERN_ERR level!) if it's working as intended.
> 
That's not my code, that was already there. I already got these messages before my 
modifications, for some ciphersuites. Of course if we don't want that, we can make
it a pr_warn pr_dbg?

> > ---
> >  crypto/testmgr.c | 269 +++++++++++++++++++++++++++++++++++++++++++------
> >  crypto/testmgr.h | 298 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 535 insertions(+), 32 deletions(-)
> >
> > diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> > index 2ba0c48..9c856d3 100644
> > --- a/crypto/testmgr.c
> > +++ b/crypto/testmgr.c
> > @@ -84,11 +84,24 @@ int alg_test(const char *driver, const char *alg, u32 type, u32 mask)
> >  #define ENCRYPT 1
> >  #define DECRYPT 0
> >
> > +struct len_range_set {
> > +	const struct len_range_sel *lensel;
> > +	unsigned int count;
> > +};
> > +
> >  struct aead_test_suite {
> >  	const struct aead_testvec *vecs;
> >  	unsigned int count;
> >  };
> >
> > +struct aead_test_params {
> > +	struct len_range_set ckeylensel;
> > +	struct len_range_set akeylensel;
> > +	struct len_range_set authsizesel;
> > +	struct len_range_set aadlensel;
> > +	struct len_range_set ptxtlensel;
> > +};
> > +
> >  struct cipher_test_suite {
> >  	const struct cipher_testvec *vecs;
> >  	unsigned int count;
> > @@ -143,6 +156,10 @@ struct alg_test_desc {
> >  		struct akcipher_test_suite akcipher;
> >  		struct kpp_test_suite kpp;
> >  	} suite;
> > +
> > +	union {
> > +		struct aead_test_params aead;
> > +	} params;
> >  };
> 
> Why not put these new fields in the existing 'struct aead_test_suite'?
> 
> I don't see the point of the separate 'params' struct.  It just confuses things.
> 
Mostly because I'm not that familiar with C datastructures (I'm not a programmer
and this is pretty much my first serious experience with C), so I didn't know how
to do that / didn't want to break anything else :-)

So if you can provide some example on how to do that ...

> >
> >  static void hexdump(unsigned char *buf, unsigned int len)
> > @@ -189,9 +206,6 @@ static void testmgr_free_buf(char *buf[XBUFSIZE])
> >  	__testmgr_free_buf(buf, 0);
> >  }
> >
> > -#define TESTMGR_POISON_BYTE	0xfe
> > -#define TESTMGR_POISON_LEN	16
> > -
> >  static inline void testmgr_poison(void *addr, size_t len)
> >  {
> >  	memset(addr, TESTMGR_POISON_BYTE, len);
> > @@ -2035,6 +2049,19 @@ static int test_aead_vec(const char *driver, int enc,
> >  }
> >
> >  #ifdef CONFIG_CRYPTO_MANAGER_EXTRA_TESTS
> > +/* Select a random length value from a list of range specs */
> 
> Perhaps mention the meaning of the -1 return value in this comment?
> 
Ok

> > +int random_lensel(const struct len_range_set *lens)
> 
> static
> 
Yes

> > +{
> > +	u32 i, sel = prandom_u32() % 1000;
> > +
> > +	for (i = 0; i < lens->count; i++)
> > +		if (sel < lens->lensel[i].threshold)
> > +			return (prandom_u32() % (lens->lensel[i].len_hi  -
> > +						 lens->lensel[i].len_lo + 1)) +
> > +				lens->lensel[i].len_lo;
> > +	return -1;
> > +}
> 
> This function isn't really AEAD-specific, so it seems it should be moved to near
> the other common fuzz test helpers like generate_random_length(), rather than be
> here where the AEAD-specific code is.
> 
It's currently only used for AEAD, but good point.

> > +
> >  /*
> >   * Generate an AEAD test vector from the given implementation.
> >   * Assumes the buffers in 'vec' were already allocated.
> > @@ -2043,44 +2070,83 @@ static void generate_random_aead_testvec(struct aead_request *req,
> >  					 struct aead_testvec *vec,
> >  					 unsigned int maxkeysize,
> >  					 unsigned int maxdatasize,
> > +					 const struct aead_test_params *lengths,
> >  					 char *name, size_t max_namelen)
> >  {
> >  	struct crypto_aead *tfm = crypto_aead_reqtfm(req);
> >  	const unsigned int ivsize = crypto_aead_ivsize(tfm);
> >  	unsigned int maxauthsize = crypto_aead_alg(tfm)->maxauthsize;
> > -	unsigned int authsize;
> > +	int authsize, clen, alen;
> >  	unsigned int total_len;
> >  	int i;
> >  	struct scatterlist src[2], dst;
> >  	u8 iv[MAX_IVLEN];
> >  	DECLARE_CRYPTO_WAIT(wait);
> >
> > -	/* Key: length in [0, maxkeysize], but usually choose maxkeysize */
> > -	vec->klen = maxkeysize;
> > -	if (prandom_u32() % 4 == 0)
> > -		vec->klen = prandom_u32() % (maxkeysize + 1);
> > -	generate_random_bytes((u8 *)vec->key, vec->klen);
> > +	alen = random_lensel(&lengths->akeylensel);
> > +	clen = random_lensel(&lengths->ckeylensel);
> > +	if ((alen >= 0) && (clen >= 0)) {
> > +		/* Corect blob header. TBD: Do we care about corrupting this? */
> > +#ifdef __LITTLE_ENDIAN
> > +		memcpy((void *)vec->key, "\x08\x00\x01\x00\x00\x00\x00\x10", 8);
> > +#else
> > +		memcpy((void *)vec->key, "\x00\x08\x00\x01\x00\x00\x00\x10", 8);
> > +#endif
> 
> Isn't this specific to "authenc" AEADs?  There needs to be something in the test
> suite that says an authenc formatted key is needed.
> 
It's under the condition of seperate authentication (alen) and cipher (clen) keys,
true AEAD's only have a single key. Because if they hadn't, they would also need
this kind of key blob thing (at least for consistency) and need this code.

> > +
> > +		/* Generate keys based on length templates */
> > +		generate_random_bytes((u8 *)(vec->key + 8), alen);
> > +		generate_random_bytes((u8 *)(vec->key + 8 + alen),
> > +				      clen);
> > +
> > +		vec->klen = 8 + alen + clen;
> > +	} else {
> > +		if (clen >= 0)
> > +			maxkeysize = clen;
> > +
> > +		vec->klen = maxkeysize;
> > +
> > +		/*
> > +		 * Key: length in [0, maxkeysize],
> > +		 * but usually choose maxkeysize
> > +		 */
> > +		if (prandom_u32() % 4 == 0)
> > +			vec->klen = prandom_u32() % (maxkeysize + 1);
> > +		generate_random_bytes((u8 *)vec->key, vec->klen);
> > +	}
> >  	vec->setkey_error = crypto_aead_setkey(tfm, vec->key, vec->klen);
> 
> The generate_random_aead_testvec() function is getting too long and complicated.
>
I'll ignore that comment to avoid starting some flame war ;-)

> It doesn't help that now the 'clen' variable is used for multiple different
> purposes (encryption key length and ciphertext length).  
>
Ah yeah, actually I had seperate variables for that which I at some point merged to
save some stack space. Blame it on me being oldschool ;-)

> Could you maybe factor out the key generation into a separate function?
> 
I could, if that would make people happy ...

> >
> >  	/* IV */
> >  	generate_random_bytes((u8 *)vec->iv, ivsize);
> >
> > -	/* Tag length: in [0, maxauthsize], but usually choose maxauthsize */
> > -	authsize = maxauthsize;
> > -	if (prandom_u32() % 4 == 0)
> > -		authsize = prandom_u32() % (maxauthsize + 1);
> > +	authsize = random_lensel(&lengths->authsizesel);
> > +	if (authsize < 0) {
> > +		/*
> > +		 * Tag length: in [0, maxauthsize],
> > +		 * but usually choose maxauthsize
> > +		 */
> > +		authsize = maxauthsize;
> > +		if (prandom_u32() % 4 == 0)
> > +			authsize = prandom_u32() % (maxauthsize + 1);
> > +	}
> >  	if (WARN_ON(authsize > maxdatasize))
> >  		authsize = maxdatasize;
> > -	maxdatasize -= authsize;
> >  	vec->setauthsize_error = crypto_aead_setauthsize(tfm, authsize);
> 
> Updating the comments to better match the code changes would be helpful too.
> E.g. here comments like the following would be helpful:
> 
>         /* Authentication tag size */
>         authsize = random_lensel(&lengths->authsizesel);
>         if (authsize < 0) {
>                 /*
>                  * No length hints for this algorithm.  Fall back to a random
>                  * value in [0, maxauthsize], but usually choose maxauthsize.
>                  */
>                 authsize = maxauthsize;
>                 if (prandom_u32() % 4 == 0)
>                         authsize = prandom_u32() % (maxauthsize + 1);
>         }
> 
Agree

> >
> >  	/* Plaintext and associated data */
> > -	total_len = generate_random_length(maxdatasize);
> > -	if (prandom_u32() % 4 == 0)
> > -		vec->alen = 0;
> > -	else
> > -		vec->alen = generate_random_length(total_len);
> > -	vec->plen = total_len - vec->alen;
> > +	alen = random_lensel(&lengths->aadlensel);
> > +	clen = random_lensel(&lengths->ptxtlensel);
> > +	maxdatasize -= authsize;
> > +	if ((alen < 0) || (clen < 0) || ((alen + clen) > maxdatasize)) {
> > +		total_len = generate_random_length(maxdatasize);
> > +		if (prandom_u32() % 4 == 0)
> > +			vec->alen = 0;
> > +		else
> > +			vec->alen = generate_random_length(total_len);
> > +		vec->plen = total_len - vec->alen;
> > +	} else {
> > +		vec->alen = alen;
> > +		vec->plen = clen;
> > +	}
> >  	generate_random_bytes((u8 *)vec->assoc, vec->alen);
> >  	generate_random_bytes((u8 *)vec->ptext, vec->plen);
> >
> > @@ -2133,7 +2199,7 @@ static int test_aead_vs_generic_impl(const char *driver,
> >  	char _generic_driver[CRYPTO_MAX_ALG_NAME];
> >  	struct crypto_aead *generic_tfm = NULL;
> >  	struct aead_request *generic_req = NULL;
> > -	unsigned int maxkeysize;
> > +	unsigned int maxkeysize, maxakeysize;
> >  	unsigned int i;
> >  	struct aead_testvec vec = { 0 };
> >  	char vec_name[64];
> > @@ -2203,9 +2269,27 @@ static int test_aead_vs_generic_impl(const char *driver,
> >  	 */
> >
> >  	maxkeysize = 0;
> > -	for (i = 0; i < test_desc->suite.aead.count; i++)
> > +	for (i = 0; i < test_desc->params.aead.ckeylensel.count; i++)
> >  		maxkeysize = max_t(unsigned int, maxkeysize,
> > -				   test_desc->suite.aead.vecs[i].klen);
> > +			test_desc->params.aead.ckeylensel.lensel[i].len_hi);
> > +
> > +	if (maxkeysize && test_desc->params.aead.akeylensel.count) {
> > +		/* authenc, explicit keylen ranges defined, use them */
> > +		maxakeysize = 0;
> > +		for (i = 0; i < test_desc->params.aead.akeylensel.count; i++)
> > +			maxakeysize = max_t(unsigned int, maxakeysize,
> > +			   test_desc->params.aead.akeylensel.lensel[i].len_hi);
> > +		maxkeysize = 8 + maxkeysize + maxakeysize;
> > +	} else if (!maxkeysize && test_desc->suite.aead.count) {
> > +		/* attempt to derive from test vectors */
> > +		for (i = 0; i < test_desc->suite.aead.count; i++)
> > +			maxkeysize = max_t(unsigned int, maxkeysize,
> > +					test_desc->suite.aead.vecs[i].klen);
> > +	} else {
> > +		pr_err("alg: aead: no key length templates or test vectors for %s - unable to fuzz\n",
> > +		       driver);
> > +		err = -EINVAL;
> > +	}
> 
> Can you put the "find the maxkeysize" logic in its own function, so it doesn't
> make test_aead_vs_generic_impl() longer and harder to understand?
> 
I could ... :-)

> >
> >  	vec.key = kmalloc(maxkeysize, GFP_KERNEL);
> >  	vec.iv = kmalloc(ivsize, GFP_KERNEL);
> > @@ -2220,6 +2304,7 @@ static int test_aead_vs_generic_impl(const char *driver,
> >  	for (i = 0; i < fuzz_iterations * 8; i++) {
> >  		generate_random_aead_testvec(generic_req, &vec,
> >  					     maxkeysize, maxdatasize,
> > +					     &test_desc->params.aead,
> >  					     vec_name, sizeof(vec_name));
> >  		generate_random_testvec_config(&cfg, cfgname, sizeof(cfgname));
> >
> > @@ -2280,11 +2365,6 @@ static int alg_test_aead(const struct alg_test_desc *desc, const char *driver,
> >  	struct cipher_test_sglists *tsgls = NULL;
> >  	int err;
> >
> > -	if (suite->count <= 0) {
> > -		pr_err("alg: aead: empty test suite for %s\n", driver);
> > -		return -EINVAL;
> > -	}
> > -
> >  	tfm = crypto_alloc_aead(driver, type, mask);
> >  	if (IS_ERR(tfm)) {
> >  		pr_err("alg: aead: failed to allocate transform for %s: %ld\n",
> > @@ -2308,6 +2388,11 @@ static int alg_test_aead(const struct alg_test_desc *desc, const char *driver,
> >  		goto out;
> >  	}
> >
> > +	if (suite->count <= 0) {
> > +		pr_err("alg: aead: empty test suite for %s\n", driver);
> > +		goto aead_skip_testsuite;
> > +	}
> > +
> 
> This should be at most pr_warn(), and maybe not printed at all.  We already know
> that some authenc compositions don't have their own test vectors; kernel users
> can't do anything about it.
> 
See comment above, not my code but if everyone agrees I'm fine with the change.

> > diff --git a/crypto/testmgr.h b/crypto/testmgr.h
> > index 6b459a6..105f2ce 100644
> > --- a/crypto/testmgr.h
> > +++ b/crypto/testmgr.h
> > @@ -28,6 +28,8 @@
> >  #include <linux/oid_registry.h>
> >
> >  #define MAX_IVLEN		32
> > +#define TESTMGR_POISON_BYTE	0xfe
> > +#define TESTMGR_POISON_LEN	16
> >
> >  /*
> >   * hash_testvec:	structure to describe a hash (message digest) test
> > @@ -176,6 +178,302 @@ struct kpp_testvec {
> >  static const char zeroed_string[48];
> >
> >  /*
> > + * length range declaration lo-hi plus selection threshold 0 - 1000
> > + */
> > +struct len_range_sel {
> > +	unsigned int len_lo;
> > +	unsigned int len_hi;
> > +	unsigned int threshold;
> > +};
> > +
> > +/*
> > + * List of length ranges sorted on increasing threshold
> > + *
> > + * 25% of each of the legal key sizes (128, 192, 256 bits)
> > + * plus 25% of illegal sizes in between 0 and 1024 bits.
> > + */
> > +static const struct len_range_sel aes_klen_template[] = {
> > +	{
> > +	.len_lo = 0,
> > +	.len_hi = 15,
> > +	.threshold = 25,
> > +	}, {
> > +	.len_lo = 16,
> > +	.len_hi = 16,
> > +	.threshold = 325,
> > +	}, {
> > +	.len_lo = 17,
> > +	.len_hi = 23,
> > +	.threshold = 350,
> > +	}, {
> > +	.len_lo = 24,
> > +	.len_hi = 24,
> > +	.threshold = 650,
> > +	}, {
> > +	.len_lo = 25,
> > +	.len_hi = 31,
> > +	.threshold = 675,
> > +	}, {
> > +	.len_lo = 32,
> > +	.len_hi = 32,
> > +	.threshold = 975,
> > +	}, {
> > +	.len_lo = 33,
> > +	.len_hi = 128,
> > +	.threshold = 1000,
> > +	}
> > +};
> 
> Can you please move these to be next to the test vectors for each algorithm, so
> things are kept in one place for each algorithm?
> 
Actually, these are supposed to be generic, to be shared across multiple
test vectors. So what do you think would be the best place for them?

> Also, perhaps these should use the convention '.proportion_of_total', like
> 'struct testvec_config' already does, rather than '.threshold'?  That would be
> more consistent with the existing test code, and would also make it slightly
> easier to change the probabilities later.
> 
I'm not quite sure if its the same thing. If someone can acknowledge that it is,
I could give it the same name. But otherwise, that would just be confusing ...

> E.g. if someone wanted to increase the probability of the first case and
> decrease the probability of the last case, then with the '.threshold' convention
> they'd have to change for every entry, but with the '.proportion_of_total'
> convention they'd only have to change the first and last entries.
> 
Oh, you are suggesting to change the whole mechanism, not just the name.
Honestly, I didn't like the proportion_of_total mechanism because it 
requires you to parse the data twice. Again, I'm oldschool so I try to provide
my data in such a form that it requires the least amount of actual processing.

> > +
> > +/* 90% legal keys of size 8, rest illegal between 0 and 32 */
> > +static const struct len_range_sel des_klen_template[] = {
> > +	{
> > +	.len_lo = 0,
> > +	.len_hi = 7,
> > +	.threshold = 50,
> > +	}, {
> > +	.len_lo = 8,
> > +	.len_hi = 8,
> > +	.threshold = 950,
> > +	}, {
> > +	.len_lo = 9,
> > +	.len_hi = 32,
> > +	.threshold = 1000,
> > +	}
> > +};
> > +
> > +/* 90% legal keys of size 24, rest illegal between 0 and 32 */
> > +static const struct len_range_sel des3_klen_template[] = {
> > +	{
> > +	.len_lo = 0,
> > +	.len_hi = 23,
> > +	.threshold = 50,
> > +	}, {
> > +	.len_lo = 24,
> > +	.len_hi = 24,
> > +	.threshold = 950,
> > +	}, {
> > +	.len_lo = 25,
> > +	.len_hi = 32,
> > +	.threshold = 1000,
> > +	}
> > +};
> > +
> > +/*
> > + * For HMAC's, favour the actual digest size for both key
> > + * size and authenticator size, but do verify some tag
> > + * truncation cases and some larger keys, including keys
> > + * exceeding the block size.
> > + */
> > +
> > +static const struct len_range_sel md5_klen_template[] = {
> 
> This really should be called "hmac_md5_klen", since md5 by itself is unkeyed.
> 
> Likewise for sha1, sha256, etc.
> 
Good point

> > +	{
> > +	.len_lo = 0, /* Allow 0 here? */
> > +	.len_hi = 15,
> > +	.threshold = 50,
> > +	}, {
> > +	.len_lo = 16,
> > +	.len_hi = 16,
> > +	.threshold = 950,
> > +	}, {
> > +	.len_lo = 17,
> > +	.len_hi = 256,
> > +	.threshold = 1000,
> > +	}
> > +};
> > +
> > +static const struct len_range_sel md5_alen_template[] = {
> 
> Likewise here: it should be "hmac_md5".
> 
Idem

> Also, "alen" is used elsewhere in the test code, including in the test vectors
> themselves, to mean the associated data length, *not* the authentication tag
> length.  But here these lengths are for authentication tags.  So please use
> "authsize".  Likewise for sha1, sha256, etc.
> 
Fair enough

> > +
> > +static const struct len_range_sel aead_alen_template[] = {
> > +	{
> > +	.len_lo = 0,
> > +	.len_hi = 0,
> > +	.threshold = 200,
> > +	}, {
> > +	.len_lo = 1,
> > +	.len_hi = 32,
> > +	.threshold = 900,
> > +	}, {
> > +	.len_lo = 33,
> > +	.len_hi = (2 * PAGE_SIZE) - TESTMGR_POISON_LEN,
> > +	.threshold = 1000,
> > +	}
> > +};
> 
> Is this for authenc or for all AEADs?  Likewise for aead_plen_template[].
> 
I believe these *could* be used for all AEAD's (that don't have specific limits on
AAD length, which some rfcxxxx AEAD's seem to have as I just learned recently).

> Again, "alen" => "authsize".
> 
Ok

> - Eric

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

RE: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing

[Pascal Van Leeuwen]

Hi Eric,

> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of
> Pascal Van Leeuwen
> Sent: Monday, July 29, 2019 11:11 AM
> To: Eric Biggers <ebiggers@kernel.org>; Pascal van Leeuwen <pascalvanl@gmail.com>
> Cc: linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net
> Subject: RE: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz
> testing
> 
> Hi Eric,
> 
> Thanks for your feedback!
> 
> > -----Original Message-----
> > From: Eric Biggers <ebiggers@kernel.org>
> > Sent: Sunday, July 28, 2019 7:31 PM
> > To: Pascal van Leeuwen <pascalvanl@gmail.com>
> > Cc: linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net;
> Pascal Van Leeuwen
> > <pvanleeuwen@verimatrix.com>
> > Subject: Re: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz
> testing
> > >
> > > +struct len_range_set {
> > > +	const struct len_range_sel *lensel;
> > > +	unsigned int count;
> > > +};
> > > +
> > >  struct aead_test_suite {
> > >  	const struct aead_testvec *vecs;
> > >  	unsigned int count;
> > >  };
> > >
> > > +struct aead_test_params {
> > > +	struct len_range_set ckeylensel;
> > > +	struct len_range_set akeylensel;
> > > +	struct len_range_set authsizesel;
> > > +	struct len_range_set aadlensel;
> > > +	struct len_range_set ptxtlensel;
> > > +};
> > > +
> > >  struct cipher_test_suite {
> > >  	const struct cipher_testvec *vecs;
> > >  	unsigned int count;
> > > @@ -143,6 +156,10 @@ struct alg_test_desc {
> > >  		struct akcipher_test_suite akcipher;
> > >  		struct kpp_test_suite kpp;
> > >  	} suite;
> > > +
> > > +	union {
> > > +		struct aead_test_params aead;
> > > +	} params;
> > >  };
> >
> > Why not put these new fields in the existing 'struct aead_test_suite'?
> >
> > I don't see the point of the separate 'params' struct.  It just confuses things.
> >
> Mostly because I'm not that familiar with C datastructures (I'm not a programmer
> and this is pretty much my first serious experience with C), so I didn't know how
> to do that / didn't want to break anything else :-)
> 
> So if you can provide some example on how to do that ...
> 
Actually, while looking into some way to combine these fields into 
'struct aead_test_suite':  I really can't think of a way to do that that
would be as convenient as the current approach which allows me to:

- NOT have these params for the other types (cipher, comp, hash etc.), at
  least for now
- NOT have to touch any declarations in the alg_test_desc assignment that
  do not need this
- conveniently use a macro line __LENS (idea shamelessly borrowed from
  __VECS) to assign the struct ptr / list length fields pairs

If you know of a better way to achieve all that, then feel free to teach
me. But, frankly I do not see why having 1 entry defining the testsuite 
and  a seperate entry defining the fuzz test parameters would necessarily
be confusing? Apart from 'params' perhaps not being a really good name, 
being too generic and all, 'fuzz_params' would probably be better?

> > - Eric
> 
> Regards,
> Pascal van Leeuwen
> Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
> www.insidesecure.com

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

Re: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing

[Eric Biggers]

On Mon, Jul 29, 2019 at 04:10:06PM +0000, Pascal Van Leeuwen wrote:
> Hi Eric,
> 
> > -----Original Message-----
> > From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of
> > Pascal Van Leeuwen
> > Sent: Monday, July 29, 2019 11:11 AM
> > To: Eric Biggers <ebiggers@kernel.org>; Pascal van Leeuwen <pascalvanl@gmail.com>
> > Cc: linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net
> > Subject: RE: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz
> > testing
> > 
> > Hi Eric,
> > 
> > Thanks for your feedback!
> > 
> > > -----Original Message-----
> > > From: Eric Biggers <ebiggers@kernel.org>
> > > Sent: Sunday, July 28, 2019 7:31 PM
> > > To: Pascal van Leeuwen <pascalvanl@gmail.com>
> > > Cc: linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net;
> > Pascal Van Leeuwen
> > > <pvanleeuwen@verimatrix.com>
> > > Subject: Re: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz
> > testing
> > > >
> > > > +struct len_range_set {
> > > > +	const struct len_range_sel *lensel;
> > > > +	unsigned int count;
> > > > +};
> > > > +
> > > >  struct aead_test_suite {
> > > >  	const struct aead_testvec *vecs;
> > > >  	unsigned int count;
> > > >  };
> > > >
> > > > +struct aead_test_params {
> > > > +	struct len_range_set ckeylensel;
> > > > +	struct len_range_set akeylensel;
> > > > +	struct len_range_set authsizesel;
> > > > +	struct len_range_set aadlensel;
> > > > +	struct len_range_set ptxtlensel;
> > > > +};
> > > > +
> > > >  struct cipher_test_suite {
> > > >  	const struct cipher_testvec *vecs;
> > > >  	unsigned int count;
> > > > @@ -143,6 +156,10 @@ struct alg_test_desc {
> > > >  		struct akcipher_test_suite akcipher;
> > > >  		struct kpp_test_suite kpp;
> > > >  	} suite;
> > > > +
> > > > +	union {
> > > > +		struct aead_test_params aead;
> > > > +	} params;
> > > >  };
> > >
> > > Why not put these new fields in the existing 'struct aead_test_suite'?
> > >
> > > I don't see the point of the separate 'params' struct.  It just confuses things.
> > >
> > Mostly because I'm not that familiar with C datastructures (I'm not a programmer
> > and this is pretty much my first serious experience with C), so I didn't know how
> > to do that / didn't want to break anything else :-)
> > 
> > So if you can provide some example on how to do that ...
> > 
> Actually, while looking into some way to combine these fields into 
> 'struct aead_test_suite':  I really can't think of a way to do that that
> would be as convenient as the current approach which allows me to:
> 
> - NOT have these params for the other types (cipher, comp, hash etc.), at
>   least for now

> - NOT have to touch any declarations in the alg_test_desc assignment that
>   do not need this
> - conveniently use a macro line __LENS (idea shamelessly borrowed from
>   __VECS) to assign the struct ptr / list length fields pairs
> 
> If you know of a better way to achieve all that, then feel free to teach
> me. But, frankly I do not see why having 1 entry defining the testsuite 
> and  a seperate entry defining the fuzz test parameters would necessarily
> be confusing? Apart from 'params' perhaps not being a really good name, 
> being too generic and all, 'fuzz_params' would probably be better?
> 

Doesn't simply putting the fields in 'struct aead_test_suite' work?

The reason the current approach confuses me is that it's unclear what should go
in the aead_test_suite and what should go in the aead_test_params, both now and
in the future as people add new stuff.  They seem like the same thing to me.

- Eric

RE: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing

[Pascal Van Leeuwen]

> -----Original Message-----
> From: Eric Biggers <ebiggers@kernel.org>
> Sent: Monday, July 29, 2019 8:23 PM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Cc: Pascal van Leeuwen <pascalvanl@gmail.com>; linux-crypto@vger.kernel.org;
> herbert@gondor.apana.org.au; davem@davemloft.net
> Subject: Re: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing
> 
> On Mon, Jul 29, 2019 at 04:10:06PM +0000, Pascal Van Leeuwen wrote:
> > Hi Eric,
> >
> > > -----Original Message-----
> > > From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf
> Of
> > > Pascal Van Leeuwen
> > > Sent: Monday, July 29, 2019 11:11 AM
> > > To: Eric Biggers <ebiggers@kernel.org>; Pascal van Leeuwen <pascalvanl@gmail.com>
> > > Cc: linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net
> > > Subject: RE: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz
> > > testing
> > >
> > > Hi Eric,
> > >
> > > Thanks for your feedback!
> > >
> > > > -----Original Message-----
> > > > From: Eric Biggers <ebiggers@kernel.org>
> > > > Sent: Sunday, July 28, 2019 7:31 PM
> > > > To: Pascal van Leeuwen <pascalvanl@gmail.com>
> > > > Cc: linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net;
> > > Pascal Van Leeuwen
> > > > <pvanleeuwen@verimatrix.com>
> > > > Subject: Re: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz
> > > testing
> > > > >
> > > > > +struct len_range_set {
> > > > > +	const struct len_range_sel *lensel;
> > > > > +	unsigned int count;
> > > > > +};
> > > > > +
> > > > >  struct aead_test_suite {
> > > > >  	const struct aead_testvec *vecs;
> > > > >  	unsigned int count;
> > > > >  };
> > > > >
> > > > > +struct aead_test_params {
> > > > > +	struct len_range_set ckeylensel;
> > > > > +	struct len_range_set akeylensel;
> > > > > +	struct len_range_set authsizesel;
> > > > > +	struct len_range_set aadlensel;
> > > > > +	struct len_range_set ptxtlensel;
> > > > > +};
> > > > > +
> > > > >  struct cipher_test_suite {
> > > > >  	const struct cipher_testvec *vecs;
> > > > >  	unsigned int count;
> > > > > @@ -143,6 +156,10 @@ struct alg_test_desc {
> > > > >  		struct akcipher_test_suite akcipher;
> > > > >  		struct kpp_test_suite kpp;
> > > > >  	} suite;
> > > > > +
> > > > > +	union {
> > > > > +		struct aead_test_params aead;
> > > > > +	} params;
> > > > >  };
> > > >
> > > > Why not put these new fields in the existing 'struct aead_test_suite'?
> > > >
> > > > I don't see the point of the separate 'params' struct.  It just confuses things.
> > > >
> > > Mostly because I'm not that familiar with C datastructures (I'm not a programmer
> > > and this is pretty much my first serious experience with C), so I didn't know how
> > > to do that / didn't want to break anything else :-)
> > >
> > > So if you can provide some example on how to do that ...
> > >
> > Actually, while looking into some way to combine these fields into
> > 'struct aead_test_suite':  I really can't think of a way to do that that
> > would be as convenient as the current approach which allows me to:
> >
> > - NOT have these params for the other types (cipher, comp, hash etc.), at
> >   least for now
> 
> > - NOT have to touch any declarations in the alg_test_desc assignment that
> >   do not need this
> > - conveniently use a macro line __LENS (idea shamelessly borrowed from
> >   __VECS) to assign the struct ptr / list length fields pairs
> >
> > If you know of a better way to achieve all that, then feel free to teach
> > me. But, frankly I do not see why having 1 entry defining the testsuite
> > and  a seperate entry defining the fuzz test parameters would necessarily
> > be confusing? Apart from 'params' perhaps not being a really good name,
> > being too generic and all, 'fuzz_params' would probably be better?
> >
> 
> Doesn't simply putting the fields in 'struct aead_test_suite' work?
> 
Depends on your definition of "work". Obviously, you could define the data
structure like that and eventually make it work. But it would make the 
initialization of it a lot less convenient. Or at least, with my limited C
coding skills, I don't see how to make it as convenient as it is now.
So it would require a lot of work on rewriting that part of the code, for
zero actual benefit (IMHO).

But actual *implementation* suggestions are welcome ...

> The reason the current approach confuses me is that it's unclear what should go
> in the aead_test_suite and what should go in the aead_test_params, both now and
> in the future as people add new stuff.  They seem like the same thing to me.
> 
aead_test_suite is the actual fixed test suite
fuzz_params are the parameters for fuzz testing

Frankly, I fail to see what would be so confusing about that, compared to
most other stuff I've seen that certainly wasn't immediately obvious to me.
Of course, some comments to further clarify could be added.

> - Eric

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

Re: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing

[Eric Biggers]

On Mon, Jul 29, 2019 at 09:10:38AM +0000, Pascal Van Leeuwen wrote:
> Hi Eric,
> 
> Thanks for your feedback!
> 
> > -----Original Message-----
> > From: Eric Biggers <ebiggers@kernel.org>
> > Sent: Sunday, July 28, 2019 7:31 PM
> > To: Pascal van Leeuwen <pascalvanl@gmail.com>
> > Cc: linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net; Pascal Van Leeuwen
> > <pvanleeuwen@verimatrix.com>
> > Subject: Re: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing
> > 
> > Hi Pascal, thanks for the patch!
> > 
> > On Wed, Jul 24, 2019 at 11:35:17AM +0200, Pascal van Leeuwen wrote:
> > > The probability of hitting specific input length corner cases relevant
> > > for certain hardware driver(s) (specifically: inside-secure) was found
> > > to be too low. Additionally, for authenc AEADs, the probability of
> > > generating a *legal* key blob approached zero, i.e. most vectors
> > > generated would only test the proper generation of a key blob error.
> > >
> > > This patch address both of these issues by improving the random
> > > generation of data lengths (for the key, but also for the ICV output
> > > and the AAD and plaintext inputs), making the random generation
> > > individually tweakable on a per-ciphersuite basis.
> > >
> > > Finally, this patch enables fuzz testing for AEAD ciphersuites that do
> > > not have a regular testsuite defined as it no longer depends on that
> > > regular testsuite for figuring out the key size.
> > >
> > > Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>
> > 
> > More comments below, but first I see some test failures and warnings with this
> > patch applied:
> > 
> > alg: aead: authenc(hmac(sha1-generic),cbc-aes-aesni) decryption failed on test vector "random: alen=26 plen=594 authsize=20 klen=60";
> > expected_error=-22, actual_error=-74, cfg="random: may_sleep use_digest src_divs=[33.6%@+12, 18.23%@+2452, 13.7%@+3761,
> > 35.64%@+4019] iv_offset=43"
> >
> Interesting as the inside-secure driver also advertises this ciphersuite and does not generate such 
> an error.  My guess is you get an error here because plen is not a multiple of 16 and this is CBC
> (note to self: for block ciphers, emphasize legal lengths in the randomization ...), but the generic
> implementation returns -EINVAL while this ciphersuite returns -EBADMSG.
> Don't ask me what the actual correct error is in this case, I'm following generic with my driver.

EINVAL is for invalid lengths while EBADMSG is for inauthentic inputs.
Inauthentic test vectors aren't yet automatically generated (even after this
patch), so I don't think EBADMSG should be seen here.  Are you sure there isn't
a bug in your patch that's causing this?

Regardless, something needs to be done about this test failure.  Generally, when
improving the tests I've sent out any needed fixes for the generic, x86, arm,
and arm64 software crypto algorithms first, since those are the most commonly
used and the easiest for most people to test.

> 
> > alg: aead: empty test suite for authenc(hmac(sha1-ni),rfc3686(ctr(aes-generic)))
> > alg: aead: empty test suite for authenc(hmac(sha1-generic),rfc3686(ctr(aes-generic)))
> > alg: aead: authenc(hmac(sha256-generic),cbc-aes-aesni) decryption failed on test vector "random: alen=14 plen=6237 authsize=32
> > klen=72"; expected_error=-22, actual_error=-74, cfg="random: may_sleep use_digest src_divs=[93.90%@+2019, 4.74%@alignmask+4094,
> > 1.36%@+21] dst_divs=[100.0%@+4000]"
> >
> Idem
> 
> > alg: aead: empty test suite for authenc(hmac(sha256-ni),rfc3686(ctr-aes-aesni))
> > alg: aead: empty test suite for authenc(hmac(sha256-generic),rfc3686(ctr(aes-generic)))
> > alg: aead: empty test suite for authenc(hmac(sha384-avx2),rfc3686(ctr-aes-aesni))
> > alg: aead: empty test suite for authenc(hmac(sha384-generic),rfc3686(ctr(aes-generic)))
> 
> > alg: aead: authenc(hmac(sha512-generic),cbc-aes-aesni) decryption failed on test vector "random: alen=14 plen=406 authsize=12
> > klen=104"; expected_error=-22, actual_error=-74, cfg="random: may_sleep use_digest src_divs=[41.41%@+852, 58.59%@+4011]
> > dst_divs=[100.0%@alignmask+4017]"
> >
> Idem
> 
> > alg: aead: empty test suite for authenc(hmac(sha512-avx2),rfc3686(ctr-aes-aesni))
> > alg: aead: empty test suite for authenc(hmac(sha512-generic),rfc3686(ctr(aes-generic)))
> > 
> > 
> > Note that the "empty test suite" message shouldn't be printed (especially not at
> > KERN_ERR level!) if it's working as intended.
> > 
> That's not my code, that was already there. I already got these messages before my 
> modifications, for some ciphersuites. Of course if we don't want that, we can make
> it a pr_warn pr_dbg?

I didn't get these error messages before this patch.  They start showing up
because this patch changes alg_test_null to alg_test_aead for algorithms with no
test vectors.

> 
> > >  struct aead_test_suite {
> > >  	const struct aead_testvec *vecs;
> > >  	unsigned int count;
> > >  };
> > >
> > > +struct aead_test_params {
> > > +	struct len_range_set ckeylensel;
> > > +	struct len_range_set akeylensel;
> > > +	struct len_range_set authsizesel;
> > > +	struct len_range_set aadlensel;
> > > +	struct len_range_set ptxtlensel;
> > > +};
> > > +
> > >  struct cipher_test_suite {
> > >  	const struct cipher_testvec *vecs;
> > >  	unsigned int count;
> > > @@ -143,6 +156,10 @@ struct alg_test_desc {
> > >  		struct akcipher_test_suite akcipher;
> > >  		struct kpp_test_suite kpp;
> > >  	} suite;
> > > +
> > > +	union {
> > > +		struct aead_test_params aead;
> > > +	} params;
> > >  };
> > 
> > Why not put these new fields in the existing 'struct aead_test_suite'?
> > 
> > I don't see the point of the separate 'params' struct.  It just confuses things.
> > 
> Mostly because I'm not that familiar with C datastructures (I'm not a programmer
> and this is pretty much my first serious experience with C), so I didn't know how
> to do that / didn't want to break anything else :-)
> 
> So if you can provide some example on how to do that ...

I'm simply suggesting adding the fields of 'struct aead_test_params' to
'struct aead_test_suite'.

> > > +	alen = random_lensel(&lengths->akeylensel);
> > > +	clen = random_lensel(&lengths->ckeylensel);
> > > +	if ((alen >= 0) && (clen >= 0)) {
> > > +		/* Corect blob header. TBD: Do we care about corrupting this? */
> > > +#ifdef __LITTLE_ENDIAN
> > > +		memcpy((void *)vec->key, "\x08\x00\x01\x00\x00\x00\x00\x10", 8);
> > > +#else
> > > +		memcpy((void *)vec->key, "\x00\x08\x00\x01\x00\x00\x00\x10", 8);
> > > +#endif
> > 
> > Isn't this specific to "authenc" AEADs?  There needs to be something in the test
> > suite that says an authenc formatted key is needed.
> > 
> It's under the condition of seperate authentication (alen) and cipher (clen) keys,
> true AEAD's only have a single key. Because if they hadn't, they would also need
> this kind of key blob thing (at least for consistency) and need this code.
> 
> > > +
> > > +		/* Generate keys based on length templates */
> > > +		generate_random_bytes((u8 *)(vec->key + 8), alen);
> > > +		generate_random_bytes((u8 *)(vec->key + 8 + alen),
> > > +				      clen);
> > > +
> > > +		vec->klen = 8 + alen + clen;
> > > +	} else {
> > > +		if (clen >= 0)
> > > +			maxkeysize = clen;
> > > +
> > > +		vec->klen = maxkeysize;
> > > +
> > > +		/*
> > > +		 * Key: length in [0, maxkeysize],
> > > +		 * but usually choose maxkeysize
> > > +		 */
> > > +		if (prandom_u32() % 4 == 0)
> > > +			vec->klen = prandom_u32() % (maxkeysize + 1);
> > > +		generate_random_bytes((u8 *)vec->key, vec->klen);
> > > +	}

Sure, but why is this patch making the length selectors specific to AEADs that
use separate authentication and encryption keys?  It should work for both.

> > >  	vec->setkey_error = crypto_aead_setkey(tfm, vec->key, vec->klen);
> > 
> > The generate_random_aead_testvec() function is getting too long and complicated.
> >
> I'll ignore that comment to avoid starting some flame war ;-)
> 
> > It doesn't help that now the 'clen' variable is used for multiple different
> > purposes (encryption key length and ciphertext length).  
> >
> Ah yeah, actually I had seperate variables for that which I at some point merged to
> save some stack space. Blame it on me being oldschool ;-)
> 
> > Could you maybe factor out the key generation into a separate function?
> > 
> I could, if that would make people happy ...

Not sure why anyone would start a flame war.  This is just usual software
development: functions often get too long and complicated as people keep
patching in more stuff, so periodically there needs to be some refactoring to
keep the code understandable/maintainable/reviewable.  And I strongly believe
that contributors to the problem need to be responsible for doing their part,
and not assume that Someone Else will do it :-)

> > > +
> > > +/*
> > > + * List of length ranges sorted on increasing threshold
> > > + *
> > > + * 25% of each of the legal key sizes (128, 192, 256 bits)
> > > + * plus 25% of illegal sizes in between 0 and 1024 bits.
> > > + */
> > > +static const struct len_range_sel aes_klen_template[] = {
> > > +	{
> > > +	.len_lo = 0,
> > > +	.len_hi = 15,
> > > +	.threshold = 25,
> > > +	}, {
> > > +	.len_lo = 16,
> > > +	.len_hi = 16,
> > > +	.threshold = 325,
> > > +	}, {
> > > +	.len_lo = 17,
> > > +	.len_hi = 23,
> > > +	.threshold = 350,
> > > +	}, {
> > > +	.len_lo = 24,
> > > +	.len_hi = 24,
> > > +	.threshold = 650,
> > > +	}, {
> > > +	.len_lo = 25,
> > > +	.len_hi = 31,
> > > +	.threshold = 675,
> > > +	}, {
> > > +	.len_lo = 32,
> > > +	.len_hi = 32,
> > > +	.threshold = 975,
> > > +	}, {
> > > +	.len_lo = 33,
> > > +	.len_hi = 128,
> > > +	.threshold = 1000,
> > > +	}
> > > +};
> > 
> > Can you please move these to be next to the test vectors for each algorithm, so
> > things are kept in one place for each algorithm?
> > 
> Actually, these are supposed to be generic, to be shared across multiple
> test vectors. So what do you think would be the best place for them?

These are specific to AES, though.  So I'd expect to find them next to the plain
AES test vectors (aes_tv_template[]). 

> 
> > Also, perhaps these should use the convention '.proportion_of_total', like
> > 'struct testvec_config' already does, rather than '.threshold'?  That would be
> > more consistent with the existing test code, and would also make it slightly
> > easier to change the probabilities later.
> > 
> I'm not quite sure if its the same thing. If someone can acknowledge that it is,
> I could give it the same name. But otherwise, that would just be confusing ...
> 
> > E.g. if someone wanted to increase the probability of the first case and
> > decrease the probability of the last case, then with the '.threshold' convention
> > they'd have to change for every entry, but with the '.proportion_of_total'
> > convention they'd only have to change the first and last entries.
> > 
> Oh, you are suggesting to change the whole mechanism, not just the name.
> Honestly, I didn't like the proportion_of_total mechanism because it 
> requires you to parse the data twice. Again, I'm oldschool so I try to provide
> my data in such a form that it requires the least amount of actual processing.
> 

It would be the same number of passes.  One for each the actual generation, and
one in testmgr_onetime_init() to verify the numbers add up to exactly 100%.
(A verification pass would still be needed in the '.threshold' convention too,
to verify that the numbers are all < 100% and in increasing order.)

I'd rather optimize for making it easy to write and change the test vectors,
since those are much more lines of code than the .c code that runs the tests.

Thanks!

- Eric

RE: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing

[Pascal Van Leeuwen]

> > Interesting as the inside-secure driver also advertises this ciphersuite and does not
> generate such
> > an error.  My guess is you get an error here because plen is not a multiple of 16 and this
> is CBC
> > (note to self: for block ciphers, emphasize legal lengths in the randomization ...), but
> the generic
> > implementation returns -EINVAL while this ciphersuite returns -EBADMSG.
> > Don't ask me what the actual correct error is in this case, I'm following generic with my
> driver.
> 
> EINVAL is for invalid lengths while EBADMSG is for inauthentic inputs.
> Inauthentic test vectors aren't yet automatically generated (even after this
> patch), so I don't think EBADMSG should be seen here.  Are you sure there isn't
> a bug in your patch that's causing this?
> 
As far as I understand it, the output of the encryption is fed back in to
decrypt. However, if the encryption didn't work due to blocksize mismatch, 
there would not be any valid encrypted and authenticated data written out. 
So, if the (generic) driver checks that for decryption, it would result in
-EINVAL. If it wouldn't check that, it would try to decrypt and authentica
te the data, which would almost certainly result in a tag mismatch and
thus an -EBADMSG error being reported.

So actually, the witnessed issue can be perfectly explained from a missing
block size check in aesni.

> Regardless, something needs to be done about this test failure.  Generally, when
> improving the tests I've sent out any needed fixes for the generic, x86, arm,
> and arm64 software crypto algorithms first, since those are the most commonly
> used and the easiest for most people to test.
> 
Well, first off, I'm running with the inside-secure driver installed, so I
actually don't see any fuzz results for any other implementations. I 
really don't have the time, tools, hardware, setup or skills to run all
kinds of different kernel configs and debug any failures that may occur
with any configuration and/or driver.  I'm not a  SW engineer and this 
is not my dayjob. It is not even a hobby as don't enjoy it very much.
So I will depend on the rest of the community to help out there.

> > > Note that the "empty test suite" message shouldn't be printed (especially not at
> > > KERN_ERR level!) if it's working as intended.
> > >
> > That's not my code, that was already there. I already got these messages before my
> > modifications, for some ciphersuites. Of course if we don't want that, we can make
> > it a pr_warn pr_dbg?
> 
> I didn't get these error messages before this patch.  They start showing up
> because this patch changes alg_test_null to alg_test_aead for algorithms with no
> test vectors.
>
Ok, I guess I caused it for some additional ciphersuites by forcing them
to be at least fuzz tested. But there were some ciphersuites without test
vectors already reporting this in my situation because they did not point
to alg_test_null in the first place. So it wasn't entirely clear what the
whole intention was in the first place, as it wasn't consistent.
If we all agree on the logging level we want for this message, then I can
make that change.

> > > Why not put these new fields in the existing 'struct aead_test_suite'?
> > >
> > > I don't see the point of the separate 'params' struct.  It just confuses things.
> > >
> > Mostly because I'm not that familiar with C datastructures (I'm not a programmer
> > and this is pretty much my first serious experience with C), so I didn't know how
> > to do that / didn't want to break anything else :-)
> >
> > So if you can provide some example on how to do that ...
> 
> I'm simply suggesting adding the fields of 'struct aead_test_params' to
> 'struct aead_test_suite'.
> 
My next mail tried to explain why that's not so simple ...

> > > > +	alen = random_lensel(&lengths->akeylensel);
> > > > +	clen = random_lensel(&lengths->ckeylensel);
> > > > +	if ((alen >= 0) && (clen >= 0)) {
> > > > +		/* Corect blob header. TBD: Do we care about corrupting this? */
> > > > +#ifdef __LITTLE_ENDIAN
> > > > +		memcpy((void *)vec->key, "\x08\x00\x01\x00\x00\x00\x00\x10", 8);
> > > > +#else
> > > > +		memcpy((void *)vec->key, "\x00\x08\x00\x01\x00\x00\x00\x10", 8);
> > > > +#endif
> > >
> > > Isn't this specific to "authenc" AEADs?  There needs to be something in the test
> > > suite that says an authenc formatted key is needed.
> > >
> > It's under the condition of seperate authentication (alen) and cipher (clen) keys,
> > true AEAD's only have a single key. Because if they hadn't, they would also need
> > this kind of key blob thing (at least for consistency) and need this code.
> >
> > > > +
> > > > +		/* Generate keys based on length templates */
> > > > +		generate_random_bytes((u8 *)(vec->key + 8), alen);
> > > > +		generate_random_bytes((u8 *)(vec->key + 8 + alen),
> > > > +				      clen);
> > > > +
> > > > +		vec->klen = 8 + alen + clen;
> > > > +	} else {
> > > > +		if (clen >= 0)
> > > > +			maxkeysize = clen;
> > > > +
> > > > +		vec->klen = maxkeysize;
> > > > +
> > > > +		/*
> > > > +		 * Key: length in [0, maxkeysize],
> > > > +		 * but usually choose maxkeysize
> > > > +		 */
> > > > +		if (prandom_u32() % 4 == 0)
> > > > +			vec->klen = prandom_u32() % (maxkeysize + 1);
> > > > +		generate_random_bytes((u8 *)vec->key, vec->klen);
> > > > +	}
> 
> Sure, but why is this patch making the length selectors specific to AEADs that
> use separate authentication and encryption keys?  It should work for both.
> 
Actually, the patch *should* (didn't try yet) make it work for both: if both
alen and clen are valid (>=0) then it creates a key blob from those ranges. 
If only clen is valid (>=0) but a alen is not (i.e., -1), then it will just
generate a random key the "normal" way with length clen.
So, for authenc you define both ranges, for other AEAD you define only a
cipher key length range with the auth key range count at 0.

> > > >  	vec->setkey_error = crypto_aead_setkey(tfm, vec->key, vec->klen);
> > >
> > > The generate_random_aead_testvec() function is getting too long and complicated.
> > >
> > I'll ignore that comment to avoid starting some flame war ;-)
> >
> > > It doesn't help that now the 'clen' variable is used for multiple different
> > > purposes (encryption key length and ciphertext length).
> > >
> > Ah yeah, actually I had seperate variables for that which I at some point merged to
> > save some stack space. Blame it on me being oldschool ;-)
> >
> > > Could you maybe factor out the key generation into a separate function?
> > >
> > I could, if that would make people happy ...
> 
> Not sure why anyone would start a flame war.  This is just usual software
> development: functions often get too long and complicated as people keep
> patching in more stuff, so periodically there needs to be some refactoring to
> keep the code understandable/maintainable/reviewable.  
>
That's an opinion I don't share. And I was trying not to go there :-)
So to avoid any useless discussion, I will just do it.

> And I strongly believe that contributors to the problem need to
> be responsible for doing their part, and not assume that Someone
> Else will do it :-)
> 
Fair enough, although this large community should provide opportunity
for others to help out a bit here and there ... especially when it
comes to trivial & non-essential improvements that anyone could do.
(I don't really have a whole lot of time I can spend on this, so I'd
rather focus on the concept rather than the implementation details)

> > > > +
> > > > +/*
> > > > + * List of length ranges sorted on increasing threshold
> > > > + *
> > > > + * 25% of each of the legal key sizes (128, 192, 256 bits)
> > > > + * plus 25% of illegal sizes in between 0 and 1024 bits.
> > > > + */
> > > > +static const struct len_range_sel aes_klen_template[] = {
> > > > +	{
> > > > +	.len_lo = 0,
> > > > +	.len_hi = 15,
> > > > +	.threshold = 25,
> > > > +	}, {
> > > > +	.len_lo = 16,
> > > > +	.len_hi = 16,
> > > > +	.threshold = 325,
> > > > +	}, {
> > > > +	.len_lo = 17,
> > > > +	.len_hi = 23,
> > > > +	.threshold = 350,
> > > > +	}, {
> > > > +	.len_lo = 24,
> > > > +	.len_hi = 24,
> > > > +	.threshold = 650,
> > > > +	}, {
> > > > +	.len_lo = 25,
> > > > +	.len_hi = 31,
> > > > +	.threshold = 675,
> > > > +	}, {
> > > > +	.len_lo = 32,
> > > > +	.len_hi = 32,
> > > > +	.threshold = 975,
> > > > +	}, {
> > > > +	.len_lo = 33,
> > > > +	.len_hi = 128,
> > > > +	.threshold = 1000,
> > > > +	}
> > > > +};
> > >
> > > Can you please move these to be next to the test vectors for each algorithm, so
> > > things are kept in one place for each algorithm?
> > >
> > Actually, these are supposed to be generic, to be shared across multiple
> > test vectors. So what do you think would be the best place for them?
> 
> These are specific to AES, though.  So I'd expect to find them next to the plain
> AES test vectors (aes_tv_template[]).
>
Ah yes, that would indeed be a better place, thanks!
 
> >
> > > Also, perhaps these should use the convention '.proportion_of_total', like
> > > 'struct testvec_config' already does, rather than '.threshold'?  That would be
> > > more consistent with the existing test code, and would also make it slightly
> > > easier to change the probabilities later.
> > >
> > I'm not quite sure if its the same thing. If someone can acknowledge that it is,
> > I could give it the same name. But otherwise, that would just be confusing ...
> >
> > > E.g. if someone wanted to increase the probability of the first case and
> > > decrease the probability of the last case, then with the '.threshold' convention
> > > they'd have to change for every entry, but with the '.proportion_of_total'
> > > convention they'd only have to change the first and last entries.
> > >
> > Oh, you are suggesting to change the whole mechanism, not just the name.
> > Honestly, I didn't like the proportion_of_total mechanism because it
> > requires you to parse the data twice. Again, I'm oldschool so I try to provide
> > my data in such a form that it requires the least amount of actual processing.
> >
> 
> It would be the same number of passes.  One for each the actual generation, and
> one in testmgr_onetime_init() to verify the numbers add up to exactly 100%.
>
Never mind, I think I had the wrong idea on how proportion_of_total worked,
they would indeed need the same number of passes. I guess the threshold 
approach is due to my HW background, reducing computation at all cost, while
the proportion of total approach may be easier for maintaining the tables.

I can change to that mechanism instead.

> (A verification pass would still be needed in the '.threshold' convention too,
> to verify that the numbers are all < 100% and in increasing order.)
> 

> I'd rather optimize for making it easy to write and change the test vectors,
> since those are much more lines of code than the .c code that runs the tests.
> 
Yeah, I figured as much. It is not natural for me to worry about these things.


> Thanks!
> 
> - Eric

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

Re: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing

[Herbert Xu]

On Mon, Jul 29, 2019 at 10:16:48PM +0000, Pascal Van Leeuwen wrote:
>
> > EINVAL is for invalid lengths while EBADMSG is for inauthentic inputs.
> > Inauthentic test vectors aren't yet automatically generated (even after this
> > patch), so I don't think EBADMSG should be seen here.  Are you sure there isn't
> > a bug in your patch that's causing this?
> > 
> As far as I understand it, the output of the encryption is fed back in to
> decrypt. However, if the encryption didn't work due to blocksize mismatch, 
> there would not be any valid encrypted and authenticated data written out. 
> So, if the (generic) driver checks that for decryption, it would result in
> -EINVAL. If it wouldn't check that, it would try to decrypt and authentica
> te the data, which would almost certainly result in a tag mismatch and
> thus an -EBADMSG error being reported.
> 
> So actually, the witnessed issue can be perfectly explained from a missing
> block size check in aesni.

The same input can indeed fail for multiple reasons.  I think in
such cases it is unreasonable to expect all implementations to
return the same error value.

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

RE: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing

[Pascal Van Leeuwen]

Herbert,

> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: Tuesday, July 30, 2019 12:31 AM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Cc: Eric Biggers <ebiggers@kernel.org>; Pascal van Leeuwen <pascalvanl@gmail.com>; linux-
> crypto@vger.kernel.org; davem@davemloft.net
> Subject: Re: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing
> 
> On Mon, Jul 29, 2019 at 10:16:48PM +0000, Pascal Van Leeuwen wrote:
> >
> > > EINVAL is for invalid lengths while EBADMSG is for inauthentic inputs.
> > > Inauthentic test vectors aren't yet automatically generated (even after this
> > > patch), so I don't think EBADMSG should be seen here.  Are you sure there isn't
> > > a bug in your patch that's causing this?
> > >
> > As far as I understand it, the output of the encryption is fed back in to
> > decrypt. However, if the encryption didn't work due to blocksize mismatch,
> > there would not be any valid encrypted and authenticated data written out.
> > So, if the (generic) driver checks that for decryption, it would result in
> > -EINVAL. If it wouldn't check that, it would try to decrypt and authentica
> > te the data, which would almost certainly result in a tag mismatch and
> > thus an -EBADMSG error being reported.
> >
> > So actually, the witnessed issue can be perfectly explained from a missing
> > block size check in aesni.
> 
> The same input can indeed fail for multiple reasons.  I think in
> such cases it is unreasonable to expect all implementations to
> return the same error value.
> 
Hmmm ... first off, testmgr expects error codes to match exactly. So if
you're saying that's not always the case, it would need to be changed.
(but then, what difference would still be acceptable?)
But so far it seems to have worked pretty well, except for this now.

You're the expert, but shouldn't there be some priority to the checks
being performed? To me, it seems reasonable to do things like length
checks prior to even *starting* decryption and authentication.
Therefore, it makes more sense to get -EINVAL than -EBADMSG in this 
case. IMHO you should only get -EBADMSG if the message was properly
formatted, but the tags eventually mismatched. From a security point
of view it can be very important to have a very clear distinction
between those 2 cases.

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


Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

Re: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing

[Eric Biggers]

On Mon, Jul 29, 2019 at 10:49:38PM +0000, Pascal Van Leeuwen wrote:
> Herbert,
> 
> > -----Original Message-----
> > From: Herbert Xu <herbert@gondor.apana.org.au>
> > Sent: Tuesday, July 30, 2019 12:31 AM
> > To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> > Cc: Eric Biggers <ebiggers@kernel.org>; Pascal van Leeuwen <pascalvanl@gmail.com>; linux-
> > crypto@vger.kernel.org; davem@davemloft.net
> > Subject: Re: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing
> > 
> > On Mon, Jul 29, 2019 at 10:16:48PM +0000, Pascal Van Leeuwen wrote:
> > >
> > > > EINVAL is for invalid lengths while EBADMSG is for inauthentic inputs.
> > > > Inauthentic test vectors aren't yet automatically generated (even after this
> > > > patch), so I don't think EBADMSG should be seen here.  Are you sure there isn't
> > > > a bug in your patch that's causing this?
> > > >
> > > As far as I understand it, the output of the encryption is fed back in to
> > > decrypt. However, if the encryption didn't work due to blocksize mismatch,
> > > there would not be any valid encrypted and authenticated data written out.
> > > So, if the (generic) driver checks that for decryption, it would result in
> > > -EINVAL. If it wouldn't check that, it would try to decrypt and authentica
> > > te the data, which would almost certainly result in a tag mismatch and
> > > thus an -EBADMSG error being reported.
> > >
> > > So actually, the witnessed issue can be perfectly explained from a missing
> > > block size check in aesni.
> > 
> > The same input can indeed fail for multiple reasons.  I think in
> > such cases it is unreasonable to expect all implementations to
> > return the same error value.
> > 
> Hmmm ... first off, testmgr expects error codes to match exactly. So if
> you're saying that's not always the case, it would need to be changed.
> (but then, what difference would still be acceptable?)
> But so far it seems to have worked pretty well, except for this now.
> 
> You're the expert, but shouldn't there be some priority to the checks
> being performed? To me, it seems reasonable to do things like length
> checks prior to even *starting* decryption and authentication.
> Therefore, it makes more sense to get -EINVAL than -EBADMSG in this 
> case. IMHO you should only get -EBADMSG if the message was properly
> formatted, but the tags eventually mismatched. From a security point
> of view it can be very important to have a very clear distinction
> between those 2 cases.
> 

Oh, I see.  Currently the fuzz tests assume that if encryption fails with an
error (such as EINVAL), then decryption fails with that same error.

Regardless of what we think the correct decryption error is, running the
decryption test at all in this case is sort of broken, since the ciphertext
buffer was never initialized.  So for now we probably should just sidestep this
issue by skipping the decryption test if encryption failed, like this:

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 96e5923889b9c1..0413bdad9f6974 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -2330,10 +2330,12 @@ static int test_aead_vs_generic_impl(const char *driver,
 					req, tsgls);
 		if (err)
 			goto out;
-		err = test_aead_vec_cfg(driver, DECRYPT, &vec, vec_name, cfg,
-					req, tsgls);
-		if (err)
-			goto out;
+		if (vec.crypt_error != 0) {
+			err = test_aead_vec_cfg(driver, DECRYPT, &vec, vec_name,
+						cfg, req, tsgls);
+			if (err)
+				goto out;
+		}
 		cond_resched();
 	}
 	err = 0;

I'm planning to (at some point) update the AEAD tests to intentionally generate
some inauthentic inputs, but that will take some more work.

- Eric

RE: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing

[Pascal Van Leeuwen]

> > You're the expert, but shouldn't there be some priority to the checks
> > being performed? To me, it seems reasonable to do things like length
> > checks prior to even *starting* decryption and authentication.
> > Therefore, it makes more sense to get -EINVAL than -EBADMSG in this
> > case. IMHO you should only get -EBADMSG if the message was properly
> > formatted, but the tags eventually mismatched. From a security point
> > of view it can be very important to have a very clear distinction
> > between those 2 cases.
> >
> 
> Oh, I see.  Currently the fuzz tests assume that if encryption fails with an
> error (such as EINVAL), then decryption fails with that same error.
> 
Ah ok, oops. It should really log the error that was returned by the
generic decryption instead. Which should just be a matter of annotating
it back to vec.crypt_error?

> Regardless of what we think the correct decryption error is, running the
> decryption test at all in this case is sort of broken, since the ciphertext
> buffer was never initialized.
>
You could consider it broken or just some convenient way of getting
vectors that don't authenticate without needing to spend any effort ...

> So for now we probably should just sidestep this
> issue by skipping the decryption test if encryption failed, like this:
> 
> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> index 96e5923889b9c1..0413bdad9f6974 100644
> --- a/crypto/testmgr.c
> +++ b/crypto/testmgr.c
> @@ -2330,10 +2330,12 @@ static int test_aead_vs_generic_impl(const char *driver,
>  					req, tsgls);
>  		if (err)
>  			goto out;
> -		err = test_aead_vec_cfg(driver, DECRYPT, &vec, vec_name, cfg,
> -					req, tsgls);
> -		if (err)
> -			goto out;
> +		if (vec.crypt_error != 0) {
> +			err = test_aead_vec_cfg(driver, DECRYPT, &vec, vec_name,
> +						cfg, req, tsgls);
> +			if (err)
> +				goto out;
> +		}
>  		cond_resched();
>  	}
>  	err = 0;
> 
> I'm planning to (at some point) update the AEAD tests to intentionally generate
> some inauthentic inputs, but that will take some more work.
> 
> - Eric
>
I believe that's a rather essential part of verifying AEAD decryption(!)


Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

Re: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing

[Eric Biggers]

On Tue, Jul 30, 2019 at 12:37:06AM +0000, Pascal Van Leeuwen wrote:
> > > You're the expert, but shouldn't there be some priority to the checks
> > > being performed? To me, it seems reasonable to do things like length
> > > checks prior to even *starting* decryption and authentication.
> > > Therefore, it makes more sense to get -EINVAL than -EBADMSG in this
> > > case. IMHO you should only get -EBADMSG if the message was properly
> > > formatted, but the tags eventually mismatched. From a security point
> > > of view it can be very important to have a very clear distinction
> > > between those 2 cases.
> > >
> > 
> > Oh, I see.  Currently the fuzz tests assume that if encryption fails with an
> > error (such as EINVAL), then decryption fails with that same error.
> > 
> Ah ok, oops. It should really log the error that was returned by the
> generic decryption instead. Which should just be a matter of annotating
> it back to vec.crypt_error?
> 

It doesn't do the generic decryption yet though, only the generic encryption.

> > Regardless of what we think the correct decryption error is, running the
> > decryption test at all in this case is sort of broken, since the ciphertext
> > buffer was never initialized.
> >
> You could consider it broken or just some convenient way of getting
> vectors that don't authenticate without needing to spend any effort ...
> 

It's not okay for it to be potentially using uninitialized memory though, even
if just in the fuzz tests.

> > So for now we probably should just sidestep this
> > issue by skipping the decryption test if encryption failed, like this:
> > 
> > diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> > index 96e5923889b9c1..0413bdad9f6974 100644
> > --- a/crypto/testmgr.c
> > +++ b/crypto/testmgr.c
> > @@ -2330,10 +2330,12 @@ static int test_aead_vs_generic_impl(const char *driver,
> >  					req, tsgls);
> >  		if (err)
> >  			goto out;
> > -		err = test_aead_vec_cfg(driver, DECRYPT, &vec, vec_name, cfg,
> > -					req, tsgls);
> > -		if (err)
> > -			goto out;
> > +		if (vec.crypt_error != 0) {
> > +			err = test_aead_vec_cfg(driver, DECRYPT, &vec, vec_name,
> > +						cfg, req, tsgls);
> > +			if (err)
> > +				goto out;
> > +		}
> >  		cond_resched();
> >  	}
> >  	err = 0;
> > 
> > I'm planning to (at some point) update the AEAD tests to intentionally generate
> > some inauthentic inputs, but that will take some more work.
> > 
> > - Eric
> >
> I believe that's a rather essential part of verifying AEAD decryption(!)
> 

Agreed, which is why I am planning to work on it :-).  Actually a while ago I
started a patch for it, but there are some issues I haven't had time to address
quite yet:
https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git/commit/?h=wip-crypto&id=687f4198ba09032c60143e0477b48f94c5714263

- Eric

RE: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing

[Pascal Van Leeuwen]

> > > Oh, I see.  Currently the fuzz tests assume that if encryption fails with an
> > > error (such as EINVAL), then decryption fails with that same error.
> > >
> > Ah ok, oops. It should really log the error that was returned by the
> > generic decryption instead. Which should just be a matter of annotating
> > it back to vec.crypt_error?
> >
> 
> It doesn't do the generic decryption yet though, only the generic encryption.
> 
I didn't look at the code in enough detail to pick that up, I was expecting
it do do generic decryption and compare that to decryption with the algorithm
being fuzzed. So what does it do then? Compare to the original input to the
encryption? Ok, I guess that would save a generic decryption pass but, as we
see here, it would not be able to capture all the details of the API.

> > > Regardless of what we think the correct decryption error is, running the
> > > decryption test at all in this case is sort of broken, since the ciphertext
> > > buffer was never initialized.
> > >
> > You could consider it broken or just some convenient way of getting
> > vectors that don't authenticate without needing to spend any effort ...
> >
> 
> It's not okay for it to be potentially using uninitialized memory though, even
> if just in the fuzz tests.
> 
Well, in this particular case things should fail before you even hit the
actual processing, so memory contents should be irrelevant really.
(by that same reasoning you would not actually hit vectors that don't
authenticate, by the way, there was an error in my thinking there)

And even if the implementation would read the contents, would it really
hurt that it's actually unitialized? They should still not be able to
influence the outcome here, so is there any other way that could cause
real-life problems? (assuming it's a legal to access buffer)

> > > So for now we probably should just sidestep this
> > > issue by skipping the decryption test if encryption failed, like this:
> > >
> > > diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> > > index 96e5923889b9c1..0413bdad9f6974 100644
> > > --- a/crypto/testmgr.c
> > > +++ b/crypto/testmgr.c
> > > @@ -2330,10 +2330,12 @@ static int test_aead_vs_generic_impl(const char *driver,
> > >  					req, tsgls);
> > >  		if (err)
> > >  			goto out;
> > > -		err = test_aead_vec_cfg(driver, DECRYPT, &vec, vec_name, cfg,
> > > -					req, tsgls);
> > > -		if (err)
> > > -			goto out;
> > > +		if (vec.crypt_error != 0) {
> > > +			err = test_aead_vec_cfg(driver, DECRYPT, &vec, vec_name,
> > > +						cfg, req, tsgls);
> > > +			if (err)
> > > +				goto out;
> > > +		}
> > >  		cond_resched();
> > >  	}
> > >  	err = 0;
> > >
> > > I'm planning to (at some point) update the AEAD tests to intentionally generate
> > > some inauthentic inputs, but that will take some more work.
> > >
> > > - Eric
> > >
> > I believe that's a rather essential part of verifying AEAD decryption(!)
> >
> 
> Agreed, which is why I am planning to work on it :-).  Actually a while ago I
> started a patch for it, but there are some issues I haven't had time to address
> quite yet:
> https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git/commit/?h=wip-
> crypto&id=687f4198ba09032c60143e0477b48f94c5714263
> 
Ok

> - Eric

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

Re: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing

[Eric Biggers]

On Tue, Jul 30, 2019 at 01:26:17AM +0000, Pascal Van Leeuwen wrote:
> > > > Oh, I see.  Currently the fuzz tests assume that if encryption fails with an
> > > > error (such as EINVAL), then decryption fails with that same error.
> > > >
> > > Ah ok, oops. It should really log the error that was returned by the
> > > generic decryption instead. Which should just be a matter of annotating
> > > it back to vec.crypt_error?
> > >
> > 
> > It doesn't do the generic decryption yet though, only the generic encryption.
> > 
> I didn't look at the code in enough detail to pick that up, I was expecting
> it do do generic decryption and compare that to decryption with the algorithm
> being fuzzed. So what does it do then? Compare to the original input to the
> encryption? Ok, I guess that would save a generic decryption pass but, as we
> see here, it would not be able to capture all the details of the API.

Currently to generate an AEAD test vector the code just generates a "random"
plaintext and encrypts it with the generic implementation.

My plan is to extend the tests to also sometimes generate a "random" ciphertext
and try to decrypt it; and also sometimes try to decrypt a corrupted ciphertext.

> 
> > > > Regardless of what we think the correct decryption error is, running the
> > > > decryption test at all in this case is sort of broken, since the ciphertext
> > > > buffer was never initialized.
> > > >
> > > You could consider it broken or just some convenient way of getting
> > > vectors that don't authenticate without needing to spend any effort ...
> > >
> > 
> > It's not okay for it to be potentially using uninitialized memory though, even
> > if just in the fuzz tests.
> > 
> Well, in this particular case things should fail before you even hit the
> actual processing, so memory contents should be irrelevant really.
> (by that same reasoning you would not actually hit vectors that don't
> authenticate, by the way, there was an error in my thinking there)

But the problem is that that's not what's actually happening, right?  "authenc"
actually does the authentication (of uninitialized memory, in this case) before
it gets around to failing due to the cbc length restriction.

Anyway, I suggest sending the patch I suggested as 1 of 2 to avoid this case (so
your patch does not cause test failures), then this patch as 2 of 2.

- Eric

RE: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing

[Pascal Van Leeuwen]

> -----Original Message-----
> From: Eric Biggers <ebiggers@kernel.org>
> Sent: Tuesday, July 30, 2019 6:26 AM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>; Pascal van Leeuwen <pascalvanl@gmail.com>;
> linux-crypto@vger.kernel.org; davem@davemloft.net
> Subject: Re: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz
> testing
> 
> On Tue, Jul 30, 2019 at 01:26:17AM +0000, Pascal Van Leeuwen wrote:
> > > > > Oh, I see.  Currently the fuzz tests assume that if encryption fails with an
> > > > > error (such as EINVAL), then decryption fails with that same error.
> > > > >
> > > > Ah ok, oops. It should really log the error that was returned by the
> > > > generic decryption instead. Which should just be a matter of annotating
> > > > it back to vec.crypt_error?
> > > >
> > >
> > > It doesn't do the generic decryption yet though, only the generic encryption.
> > >
> > I didn't look at the code in enough detail to pick that up, I was expecting
> > it do do generic decryption and compare that to decryption with the algorithm
> > being fuzzed. So what does it do then? Compare to the original input to the
> > encryption? Ok, I guess that would save a generic decryption pass but, as we
> > see here, it would not be able to capture all the details of the API.
> 
> Currently to generate an AEAD test vector the code just generates a "random"
> plaintext and encrypts it with the generic implementation.
> 
> My plan is to extend the tests to also sometimes generate a "random" ciphertext
> and try to decrypt it; and also sometimes try to decrypt a corrupted ciphertext.
> 
My guess is trying that first part will give you the second part for 
free :-) (i.e. if it's random, it almost certainly won't authenticate)

> >
> > > > > Regardless of what we think the correct decryption error is, running the
> > > > > decryption test at all in this case is sort of broken, since the ciphertext
> > > > > buffer was never initialized.
> > > > >
> > > > You could consider it broken or just some convenient way of getting
> > > > vectors that don't authenticate without needing to spend any effort ...
> > > >
> > >
> > > It's not okay for it to be potentially using uninitialized memory though, even
> > > if just in the fuzz tests.
> > >
> > Well, in this particular case things should fail before you even hit the
> > actual processing, so memory contents should be irrelevant really.
> > (by that same reasoning you would not actually hit vectors that don't
> > authenticate, by the way, there was an error in my thinking there)
> 
> But the problem is that that's not what's actually happening, right?  "authenc"
> actually does the authentication (of uninitialized memory, in this case) before
> it gets around to failing due to the cbc length restriction.
> 
> Anyway, I suggest sending the patch I suggested as 1 of 2 to avoid this case (so
> your patch does not cause test failures), then this patch as 2 of 2.
> 
Ok, fine

> - Eric


Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

Re: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing

[Eric Biggers]

On Mon, Jul 29, 2019 at 10:16:48PM +0000, Pascal Van Leeuwen wrote:
> > > > Note that the "empty test suite" message shouldn't be printed (especially not at
> > > > KERN_ERR level!) if it's working as intended.
> > > >
> > > That's not my code, that was already there. I already got these messages before my
> > > modifications, for some ciphersuites. Of course if we don't want that, we can make
> > > it a pr_warn pr_dbg?
> > 
> > I didn't get these error messages before this patch.  They start showing up
> > because this patch changes alg_test_null to alg_test_aead for algorithms with no
> > test vectors.
> >
> Ok, I guess I caused it for some additional ciphersuites by forcing them
> to be at least fuzz tested. But there were some ciphersuites without test
> vectors already reporting this in my situation because they did not point
> to alg_test_null in the first place. 

Are you sure?  I don't see anything that had no test vectors but didn't use
alg_test_null.

> So it wasn't entirely clear what the
> whole intention was in the first place, as it wasn't consistent.
> If we all agree on the logging level we want for this message, then I can
> make that change.

I suggest at least downgrading it to KERN_INFO, since that's the level used for
logging that there wasn't any test description found at all:

	printk(KERN_INFO "alg: No test for %s (%s)\n", alg, driver);

> 
> > > > Why not put these new fields in the existing 'struct aead_test_suite'?
> > > >
> > > > I don't see the point of the separate 'params' struct.  It just confuses things.
> > > >
> > > Mostly because I'm not that familiar with C datastructures (I'm not a programmer
> > > and this is pretty much my first serious experience with C), so I didn't know how
> > > to do that / didn't want to break anything else :-)
> > >
> > > So if you can provide some example on how to do that ...
> > 
> > I'm simply suggesting adding the fields of 'struct aead_test_params' to
> > 'struct aead_test_suite'.
> > 
> My next mail tried to explain why that's not so simple ...

The only actual issue is that you can't reuse the __VECS() macro because it adds
an extra level of braces, right?

> Actually, the patch *should* (didn't try yet) make it work for both: if both
> alen and clen are valid (>=0) then it creates a key blob from those ranges. 
> If only clen is valid (>=0) but a alen is not (i.e., -1), then it will just
> generate a random key the "normal" way with length clen.
> So, for authenc you define both ranges, for other AEAD you define only a
> cipher key length range with the auth key range count at 0.
> 

Okay, I guess that makes sense.  It wasn't obvious to me though.

- Eric

RE: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing

[Pascal Van Leeuwen]

> -----Original Message-----
> From: Eric Biggers <ebiggers@kernel.org>
> Sent: Tuesday, July 30, 2019 2:17 AM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Cc: Pascal van Leeuwen <pascalvanl@gmail.com>; linux-crypto@vger.kernel.org;
> herbert@gondor.apana.org.au; davem@davemloft.net
> Subject: Re: [PATCH] crypto: testmgr - Improve randomization of params for AEAD fuzz testing
> 
> On Mon, Jul 29, 2019 at 10:16:48PM +0000, Pascal Van Leeuwen wrote:
> > > > > Note that the "empty test suite" message shouldn't be printed (especially not at
> > > > > KERN_ERR level!) if it's working as intended.
> > > > >
> > > > That's not my code, that was already there. I already got these messages before my
> > > > modifications, for some ciphersuites. Of course if we don't want that, we can make
> > > > it a pr_warn pr_dbg?
> > >
> > > I didn't get these error messages before this patch.  They start showing up
> > > because this patch changes alg_test_null to alg_test_aead for algorithms with no
> > > test vectors.
> > >
> > Ok, I guess I caused it for some additional ciphersuites by forcing them
> > to be at least fuzz tested. But there were some ciphersuites without test
> > vectors already reporting this in my situation because they did not point
> > to alg_test_null in the first place.
> 
> Are you sure?  I don't see anything that had no test vectors but didn't use
> alg_test_null.
> 
> > So it wasn't entirely clear what the
> > whole intention was in the first place, as it wasn't consistent.
> > If we all agree on the logging level we want for this message, then I can
> > make that change.
> 
> I suggest at least downgrading it to KERN_INFO, since that's the level used for
> logging that there wasn't any test description found at all:
> 
> 	printk(KERN_INFO "alg: No test for %s (%s)\n", alg, driver);
> 
Ah ... I think I may have confused those 2 error messages that mean pretty
much the same thing to me ... no test ... empty testsuite. If the testsuite
is empty, there is no test. So KERN_INFO appears to be what we want here.

> >
> > > > > Why not put these new fields in the existing 'struct aead_test_suite'?
> > > > >
> > > > > I don't see the point of the separate 'params' struct.  It just confuses things.
> > > > >
> > > > Mostly because I'm not that familiar with C datastructures (I'm not a programmer
> > > > and this is pretty much my first serious experience with C), so I didn't know how
> > > > to do that / didn't want to break anything else :-)
> > > >
> > > > So if you can provide some example on how to do that ...
> > >
> > > I'm simply suggesting adding the fields of 'struct aead_test_params' to
> > > 'struct aead_test_suite'.
> > >
> > My next mail tried to explain why that's not so simple ...
> 
> The only actual issue is that you can't reuse the __VECS() macro because it adds
> an extra level of braces, right?
> 
Yes, not being able to use __LENS within __VECS would be the main problem.
And I'm not familiar enough with the C preprocessor to solve that myself.

> > Actually, the patch *should* (didn't try yet) make it work for both: if both
> > alen and clen are valid (>=0) then it creates a key blob from those ranges.
> > If only clen is valid (>=0) but a alen is not (i.e., -1), then it will just
> > generate a random key the "normal" way with length clen.
> > So, for authenc you define both ranges, for other AEAD you define only a
> > cipher key length range with the auth key range count at 0.
> >
> 
> Okay, I guess that makes sense.  It wasn't obvious to me though.
> 
> - Eric

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com