| Message ID | 20180622143722.9406-4-jglauber@cavium.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
Hi Jan, On Fri, Jun 22, 2018 at 04:37:20PM +0200, Jan Glauber wrote: > While commit 336073840a87 ("crypto: testmgr - Allow different compression results") > allowed to test non-generic compression algorithms there are some corner > cases that would not be detected in test_comp(). > > For example if input -> compression -> decompression would all yield > the same bytes the test would still pass. > > Improve the compression test by using the generic variant (if available) > to decompress the compressed test vector from the non-generic > algorithm. > > Suggested-by: Herbert Xu <herbert@gondor.apana.org.au> > Signed-off-by: Jan Glauber <jglauber@cavium.com> > --- > crypto/testmgr.c | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > diff --git a/crypto/testmgr.c b/crypto/testmgr.c > index d1d99843cce4..cfb5fe4c5ccf 100644 > --- a/crypto/testmgr.c > +++ b/crypto/testmgr.c > @@ -1346,6 +1346,7 @@ static int test_comp(struct crypto_comp *tfm, > int ctcount, int dtcount) > { Any particular reason for not updating test_acomp() too? > const char *algo = crypto_tfm_alg_driver_name(crypto_comp_tfm(tfm)); > + const char *name = crypto_tfm_alg_name(crypto_comp_tfm(tfm)); > char *output, *decomp_output; > unsigned int i; > int ret; > @@ -1363,6 +1364,8 @@ static int test_comp(struct crypto_comp *tfm, > for (i = 0; i < ctcount; i++) { > int ilen; > unsigned int dlen = COMP_BUF_SIZE; > + struct crypto_comp *tfm_decomp = NULL; > + char *gname; > > memset(output, 0, sizeof(COMP_BUF_SIZE)); > memset(decomp_output, 0, sizeof(COMP_BUF_SIZE)); > @@ -1377,9 +1380,27 @@ static int test_comp(struct crypto_comp *tfm, > goto out; > } > > + /* > + * If compression of a non-generic algorithm was tested try to > + * decompress using the generic variant. > + */ > + if (!strstr(algo, "generic")) { That's a pretty sloppy string comparison. It matches "generic" anywhere in the string, like "foogenericbar". It should just be "-generic" at the end, right? Like: static bool is_generic_driver(const char *driver_name) { size_t len = strlen(driver_name); return len >= 8 && !strcmp(&driver_name[len - 8], "-generic"); } > + /* Construct name from cra_name + "-generic" */ > + gname = kmalloc(strlen(name) + 9, GFP_KERNEL); > + strncpy(gname, name, strlen(name)); > + strncpy(gname + strlen(name), "-generic", 9); > + > + tfm_decomp = crypto_alloc_comp(gname, 0, 0); > + kfree(gname); If you're going to allocate memory here you need to check for error (note: kasprintf() would make building the string a bit cleaner). But algorithm names are limited anyway, so a better way may be: char generic_name[CRYPTO_MAX_ALG_NAME]; if (snprintf(generic_name, "%s-generic", name) < sizeof(generic_name)) tfm_decomp = crypto_alloc_comp(gname, 0, 0); > + } > + > + /* If there is no generic variant use the same tfm as before. */ > + if (!tfm_decomp || IS_ERR(tfm_decomp)) > + tfm_decomp = tfm; > + if (!IS_ERR_OR_NULL(tfm_decomp)) > ilen = dlen; > dlen = COMP_BUF_SIZE; > - ret = crypto_comp_decompress(tfm, output, > + ret = crypto_comp_decompress(tfm_decomp, output, > ilen, decomp_output, &dlen); Shouldn't you decompress with both tfms, not just the generic one? It's also weird that each 'struct comp_testvec' in 'ctemplate[]' has an 'output', but it's never used. The issue seems to be that there are separate test vectors for compression and decompression, but you really only need one set. It would have the '.uncompressed' and '.compressed' data. From that, you could compress the '.uncompressed' data with the tfm under test, and decompress that result with both the tfm under test and the generic tfm. Then, you could decompress the '.compressed' data with the tfm under test and verify it matches the '.uncompressed' data. (I did something similar for symmetric ciphers in commit 92a4c9fef34c.) Thanks, - Eric
Hi Eric, sorry for my late response. On Fri, Jun 22, 2018 at 08:12:26PM -0700, Eric Biggers wrote: > Hi Jan, > > On Fri, Jun 22, 2018 at 04:37:20PM +0200, Jan Glauber wrote: > > While commit 336073840a87 ("crypto: testmgr - Allow different compression results") > > allowed to test non-generic compression algorithms there are some corner > > cases that would not be detected in test_comp(). > > > > For example if input -> compression -> decompression would all yield > > the same bytes the test would still pass. > > > > Improve the compression test by using the generic variant (if available) > > to decompress the compressed test vector from the non-generic > > algorithm. > > > > Suggested-by: Herbert Xu <herbert@gondor.apana.org.au> > > Signed-off-by: Jan Glauber <jglauber@cavium.com> > > --- > > crypto/testmgr.c | 23 ++++++++++++++++++++++- > > 1 file changed, 22 insertions(+), 1 deletion(-) > > > > diff --git a/crypto/testmgr.c b/crypto/testmgr.c > > index d1d99843cce4..cfb5fe4c5ccf 100644 > > --- a/crypto/testmgr.c > > +++ b/crypto/testmgr.c > > @@ -1346,6 +1346,7 @@ static int test_comp(struct crypto_comp *tfm, > > int ctcount, int dtcount) > > { > > Any particular reason for not updating test_acomp() too? No, the same logic could be applied there too, I'll try that. > > const char *algo = crypto_tfm_alg_driver_name(crypto_comp_tfm(tfm)); > > + const char *name = crypto_tfm_alg_name(crypto_comp_tfm(tfm)); > > char *output, *decomp_output; > > unsigned int i; > > int ret; > > @@ -1363,6 +1364,8 @@ static int test_comp(struct crypto_comp *tfm, > > for (i = 0; i < ctcount; i++) { > > int ilen; > > unsigned int dlen = COMP_BUF_SIZE; > > + struct crypto_comp *tfm_decomp = NULL; > > + char *gname; > > > > memset(output, 0, sizeof(COMP_BUF_SIZE)); > > memset(decomp_output, 0, sizeof(COMP_BUF_SIZE)); > > @@ -1377,9 +1380,27 @@ static int test_comp(struct crypto_comp *tfm, > > goto out; > > } > > > > + /* > > + * If compression of a non-generic algorithm was tested try to > > + * decompress using the generic variant. > > + */ > > + if (!strstr(algo, "generic")) { > > That's a pretty sloppy string comparison. It matches "generic" anywhere in the > string, like "foogenericbar". It should just be "-generic" at the end, right? I think the maintainers should scream if someone misuses generic... > Like: > > static bool is_generic_driver(const char *driver_name) > { > size_t len = strlen(driver_name); > > return len >= 8 && !strcmp(&driver_name[len - 8], "-generic"); > } ... but I agree that this is cleaner. > > + /* Construct name from cra_name + "-generic" */ > > + gname = kmalloc(strlen(name) + 9, GFP_KERNEL); > > + strncpy(gname, name, strlen(name)); > > + strncpy(gname + strlen(name), "-generic", 9); > > + > > + tfm_decomp = crypto_alloc_comp(gname, 0, 0); > > + kfree(gname); > > If you're going to allocate memory here you need to check for error (note: > kasprintf() would make building the string a bit cleaner). But algorithm names > are limited anyway, so a better way may be: > > char generic_name[CRYPTO_MAX_ALG_NAME]; > > if (snprintf(generic_name, "%s-generic", name) < > sizeof(generic_name)) > tfm_decomp = crypto_alloc_comp(gname, 0, 0); > OK (for the tests the stack usage can probably be ignored). > > + } > > + > > + /* If there is no generic variant use the same tfm as before. */ > > + if (!tfm_decomp || IS_ERR(tfm_decomp)) > > + tfm_decomp = tfm; > > + > > if (!IS_ERR_OR_NULL(tfm_decomp)) OK. > > ilen = dlen; > > dlen = COMP_BUF_SIZE; > > - ret = crypto_comp_decompress(tfm, output, > > + ret = crypto_comp_decompress(tfm_decomp, output, > > ilen, decomp_output, &dlen); > > Shouldn't you decompress with both tfms, not just the generic one? Good idea. > It's also weird that each 'struct comp_testvec' in 'ctemplate[]' has an > 'output', but it's never used. The issue seems to be that there are separate > test vectors for compression and decompression, but you really only need one > set. It would have the '.uncompressed' and '.compressed' data. From that, you > could compress the '.uncompressed' data with the tfm under test, and decompress > that result with both the tfm under test and the generic tfm. Then, you could > decompress the '.compressed' data with the tfm under test and verify it matches > the '.uncompressed' data. (I did something similar for symmetric ciphers in > commit 92a4c9fef34c.) I did the patches before your change. I'll see if the same can be applied here. thanks, Jan
diff --git a/crypto/testmgr.c b/crypto/testmgr.c index d1d99843cce4..cfb5fe4c5ccf 100644 --- a/crypto/testmgr.c +++ b/crypto/testmgr.c @@ -1346,6 +1346,7 @@ static int test_comp(struct crypto_comp *tfm, int ctcount, int dtcount) { const char *algo = crypto_tfm_alg_driver_name(crypto_comp_tfm(tfm)); + const char *name = crypto_tfm_alg_name(crypto_comp_tfm(tfm)); char *output, *decomp_output; unsigned int i; int ret; @@ -1363,6 +1364,8 @@ static int test_comp(struct crypto_comp *tfm, for (i = 0; i < ctcount; i++) { int ilen; unsigned int dlen = COMP_BUF_SIZE; + struct crypto_comp *tfm_decomp = NULL; + char *gname; memset(output, 0, sizeof(COMP_BUF_SIZE)); memset(decomp_output, 0, sizeof(COMP_BUF_SIZE)); @@ -1377,9 +1380,27 @@ static int test_comp(struct crypto_comp *tfm, goto out; } + /* + * If compression of a non-generic algorithm was tested try to + * decompress using the generic variant. + */ + if (!strstr(algo, "generic")) { + /* Construct name from cra_name + "-generic" */ + gname = kmalloc(strlen(name) + 9, GFP_KERNEL); + strncpy(gname, name, strlen(name)); + strncpy(gname + strlen(name), "-generic", 9); + + tfm_decomp = crypto_alloc_comp(gname, 0, 0); + kfree(gname); + } + + /* If there is no generic variant use the same tfm as before. */ + if (!tfm_decomp || IS_ERR(tfm_decomp)) + tfm_decomp = tfm; + ilen = dlen; dlen = COMP_BUF_SIZE; - ret = crypto_comp_decompress(tfm, output, + ret = crypto_comp_decompress(tfm_decomp, output, ilen, decomp_output, &dlen); if (ret) { pr_err("alg: comp: compression failed: decompress: on test %d for %s failed: ret=%d\n",
While commit 336073840a87 ("crypto: testmgr - Allow different compression results") allowed to test non-generic compression algorithms there are some corner cases that would not be detected in test_comp(). For example if input -> compression -> decompression would all yield the same bytes the test would still pass. Improve the compression test by using the generic variant (if available) to decompress the compressed test vector from the non-generic algorithm. Suggested-by: Herbert Xu <herbert@gondor.apana.org.au> Signed-off-by: Jan Glauber <jglauber@cavium.com> --- crypto/testmgr.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)