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@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?
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
