> -----Original Message-----
> From: Eric Biggers <ebiggers@xxxxxxxxxx>
> Sent: Monday, July 29, 2019 8:23 PM
> To: Pascal Van Leeuwen <pvanleeuwen@xxxxxxxxxxxxxx>
> Cc: Pascal van Leeuwen <pascalvanl@xxxxxxxxx>; linux-crypto@xxxxxxxxxxxxxxx;
> herbert@xxxxxxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx <linux-crypto-owner@xxxxxxxxxxxxxxx> On Behalf
> Of
> > > Pascal Van Leeuwen
> > > Sent: Monday, July 29, 2019 11:11 AM
> > > To: Eric Biggers <ebiggers@xxxxxxxxxx>; Pascal van Leeuwen <pascalvanl@xxxxxxxxx>
> > > Cc: linux-crypto@xxxxxxxxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx
> > > 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@xxxxxxxxxx>
> > > > Sent: Sunday, July 28, 2019 7:31 PM
> > > > To: Pascal van Leeuwen <pascalvanl@xxxxxxxxx>
> > > > Cc: linux-crypto@xxxxxxxxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx;
> > > Pascal Van Leeuwen
> > > > <pvanleeuwen@xxxxxxxxxxxxxx>
> > > > 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
