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

[Eric Biggers <ebiggers@kernel.org>]

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