> > Interesting as the inside-secure driver also advertises this ciphersuite and does not
> generate such
> > an error. My guess is you get an error here because plen is not a multiple of 16 and this
> is CBC
> > (note to self: for block ciphers, emphasize legal lengths in the randomization ...), but
> the generic
> > implementation returns -EINVAL while this ciphersuite returns -EBADMSG.
> > Don't ask me what the actual correct error is in this case, I'm following generic with my
> driver.
>
> EINVAL is for invalid lengths while EBADMSG is for inauthentic inputs.
> Inauthentic test vectors aren't yet automatically generated (even after this
> patch), so I don't think EBADMSG should be seen here. Are you sure there isn't
> a bug in your patch that's causing this?
>
As far as I understand it, the output of the encryption is fed back in to
decrypt. However, if the encryption didn't work due to blocksize mismatch,
there would not be any valid encrypted and authenticated data written out.
So, if the (generic) driver checks that for decryption, it would result in
-EINVAL. If it wouldn't check that, it would try to decrypt and authentica
te the data, which would almost certainly result in a tag mismatch and
thus an -EBADMSG error being reported.
So actually, the witnessed issue can be perfectly explained from a missing
block size check in aesni.
> Regardless, something needs to be done about this test failure. Generally, when
> improving the tests I've sent out any needed fixes for the generic, x86, arm,
> and arm64 software crypto algorithms first, since those are the most commonly
> used and the easiest for most people to test.
>
Well, first off, I'm running with the inside-secure driver installed, so I
actually don't see any fuzz results for any other implementations. I
really don't have the time, tools, hardware, setup or skills to run all
kinds of different kernel configs and debug any failures that may occur
with any configuration and/or driver. I'm not a SW engineer and this
is not my dayjob. It is not even a hobby as don't enjoy it very much.
So I will depend on the rest of the community to help out there.
> > > 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. 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.
> > > 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 ...
> > > > + alen = random_lensel(&lengths->akeylensel);
> > > > + clen = random_lensel(&lengths->ckeylensel);
> > > > + if ((alen >= 0) && (clen >= 0)) {
> > > > + /* Corect blob header. TBD: Do we care about corrupting this? */
> > > > +#ifdef __LITTLE_ENDIAN
> > > > + memcpy((void *)vec->key, "\x08\x00\x01\x00\x00\x00\x00\x10", 8);
> > > > +#else
> > > > + memcpy((void *)vec->key, "\x00\x08\x00\x01\x00\x00\x00\x10", 8);
> > > > +#endif
> > >
> > > Isn't this specific to "authenc" AEADs? There needs to be something in the test
> > > suite that says an authenc formatted key is needed.
> > >
> > It's under the condition of seperate authentication (alen) and cipher (clen) keys,
> > true AEAD's only have a single key. Because if they hadn't, they would also need
> > this kind of key blob thing (at least for consistency) and need this code.
> >
> > > > +
> > > > + /* Generate keys based on length templates */
> > > > + generate_random_bytes((u8 *)(vec->key + 8), alen);
> > > > + generate_random_bytes((u8 *)(vec->key + 8 + alen),
> > > > + clen);
> > > > +
> > > > + vec->klen = 8 + alen + clen;
> > > > + } else {
> > > > + if (clen >= 0)
> > > > + maxkeysize = clen;
> > > > +
> > > > + vec->klen = maxkeysize;
> > > > +
> > > > + /*
> > > > + * Key: length in [0, maxkeysize],
> > > > + * but usually choose maxkeysize
> > > > + */
> > > > + if (prandom_u32() % 4 == 0)
> > > > + vec->klen = prandom_u32() % (maxkeysize + 1);
> > > > + generate_random_bytes((u8 *)vec->key, vec->klen);
> > > > + }
>
> Sure, but why is this patch making the length selectors specific to AEADs that
> use separate authentication and encryption keys? It should work for both.
>
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.
> > > > vec->setkey_error = crypto_aead_setkey(tfm, vec->key, vec->klen);
> > >
> > > The generate_random_aead_testvec() function is getting too long and complicated.
> > >
> > I'll ignore that comment to avoid starting some flame war ;-)
> >
> > > It doesn't help that now the 'clen' variable is used for multiple different
> > > purposes (encryption key length and ciphertext length).
> > >
> > Ah yeah, actually I had seperate variables for that which I at some point merged to
> > save some stack space. Blame it on me being oldschool ;-)
> >
> > > Could you maybe factor out the key generation into a separate function?
> > >
> > I could, if that would make people happy ...
>
> Not sure why anyone would start a flame war. This is just usual software
> development: functions often get too long and complicated as people keep
> patching in more stuff, so periodically there needs to be some refactoring to
> keep the code understandable/maintainable/reviewable.
>
That's an opinion I don't share. And I was trying not to go there :-)
So to avoid any useless discussion, I will just do it.
> And I strongly believe that contributors to the problem need to
> be responsible for doing their part, and not assume that Someone
> Else will do it :-)
>
Fair enough, although this large community should provide opportunity
for others to help out a bit here and there ... especially when it
comes to trivial & non-essential improvements that anyone could do.
(I don't really have a whole lot of time I can spend on this, so I'd
rather focus on the concept rather than the implementation details)
> > > > +
> > > > +/*
> > > > + * List of length ranges sorted on increasing threshold
> > > > + *
> > > > + * 25% of each of the legal key sizes (128, 192, 256 bits)
> > > > + * plus 25% of illegal sizes in between 0 and 1024 bits.
> > > > + */
> > > > +static const struct len_range_sel aes_klen_template[] = {
> > > > + {
> > > > + .len_lo = 0,
> > > > + .len_hi = 15,
> > > > + .threshold = 25,
> > > > + }, {
> > > > + .len_lo = 16,
> > > > + .len_hi = 16,
> > > > + .threshold = 325,
> > > > + }, {
> > > > + .len_lo = 17,
> > > > + .len_hi = 23,
> > > > + .threshold = 350,
> > > > + }, {
> > > > + .len_lo = 24,
> > > > + .len_hi = 24,
> > > > + .threshold = 650,
> > > > + }, {
> > > > + .len_lo = 25,
> > > > + .len_hi = 31,
> > > > + .threshold = 675,
> > > > + }, {
> > > > + .len_lo = 32,
> > > > + .len_hi = 32,
> > > > + .threshold = 975,
> > > > + }, {
> > > > + .len_lo = 33,
> > > > + .len_hi = 128,
> > > > + .threshold = 1000,
> > > > + }
> > > > +};
> > >
> > > Can you please move these to be next to the test vectors for each algorithm, so
> > > things are kept in one place for each algorithm?
> > >
> > Actually, these are supposed to be generic, to be shared across multiple
> > test vectors. So what do you think would be the best place for them?
>
> These are specific to AES, though. So I'd expect to find them next to the plain
> AES test vectors (aes_tv_template[]).
>
Ah yes, that would indeed be a better place, thanks!
> >
> > > Also, perhaps these should use the convention '.proportion_of_total', like
> > > 'struct testvec_config' already does, rather than '.threshold'? That would be
> > > more consistent with the existing test code, and would also make it slightly
> > > easier to change the probabilities later.
> > >
> > I'm not quite sure if its the same thing. If someone can acknowledge that it is,
> > I could give it the same name. But otherwise, that would just be confusing ...
> >
> > > E.g. if someone wanted to increase the probability of the first case and
> > > decrease the probability of the last case, then with the '.threshold' convention
> > > they'd have to change for every entry, but with the '.proportion_of_total'
> > > convention they'd only have to change the first and last entries.
> > >
> > Oh, you are suggesting to change the whole mechanism, not just the name.
> > Honestly, I didn't like the proportion_of_total mechanism because it
> > requires you to parse the data twice. Again, I'm oldschool so I try to provide
> > my data in such a form that it requires the least amount of actual processing.
> >
>
> It would be the same number of passes. One for each the actual generation, and
> one in testmgr_onetime_init() to verify the numbers add up to exactly 100%.
>
Never mind, I think I had the wrong idea on how proportion_of_total worked,
they would indeed need the same number of passes. I guess the threshold
approach is due to my HW background, reducing computation at all cost, while
the proportion of total approach may be easier for maintaining the tables.
I can change to that mechanism instead.
> (A verification pass would still be needed in the '.threshold' convention too,
> to verify that the numbers are all < 100% and in increasing order.)
>
> I'd rather optimize for making it easy to write and change the test vectors,
> since those are much more lines of code than the .c code that runs the tests.
>
Yeah, I figured as much. It is not natural for me to worry about these things.
> Thanks!
>
> - Eric
Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com
