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@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Jan Glauber <jglauber@xxxxxxxxxx>
> ---
> 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
