Re: [PATCH 0/7] crypto: aes - allow generic AES to be omitted

[Eric Biggers <ebiggers3@gmail.com>]

On Tue, Mar 28, 2017 at 09:51:54AM +0100, Ard Biesheuvel wrote:
> On 28 March 2017 at 06:43, Eric Biggers <ebiggers3@gmail.com> wrote:
> >
> > Just a thought: how about renaming CRYPTO_AES to CRYPTO_AES_GENERIC, then
> > renaming what you called CRYPTO_NEED_AES to CRYPTO_AES?  Then all the 'select
> > CRYPTO_AES' can remain as-is, instead of replacing them with the (in my opinion
> > uglier) 'select CRYPTO_NEED_AES'.  And it should still work for people who have
> > CRYPTO_AES=y or CRYPTO_AES=m in their kernel config, since they'll still get at
> > least one AES implementation (though they may stop getting the generic one).
> >
> > Also, in general I think we need better Kconfig help text.  As proposed you can
> > now toggle simply "AES cipher algorithms", and nowhere in the help text is it
> > mentioned that that is only the generic implementation, which you don't need if
> > you have enabled some other implementation.  Similarly for "Fixed time AES
> > cipher"; it perhaps should be mentioned that it's only useful if a fixed-time
> > implementation using special CPU instructions like AES-NI or ARMv8-CE isn't
> > usable.
> >
> 
> Thanks for the feedback. I take it you are on board with the general idea then?
> 
> Re name change, those are good points. I will experiment with that.
> 
> I was a bit on the fence about modifying the x86 code more than
> required, but actually, I think it makes sense for the AES-NI code to
> use fixed-time AES as a fallback rather than the table-based x86 code,
> given that the fallback is rarely used (only when executed in the
> context of an interrupt taken from kernel code that is already using
> the FPU) and falling back to a non-fixed time implementation loses
> some guarantees that the AES-NI code gives.

Definitely, I just feel it needs to be cleaned up a little so that the different
AES config options and modules aren't quite as confusing to those not as
familiar with them.

Did you also consider having 

	crypto_aes_set_key_generic()
and
	crypto_aes_expand_key_ti()
	crypto_aes_set_key_ti()

instead of crypto_aes_set_key() and crypto_aes_expand_key()?  As-is, it isn't
immediately clear which function is part of which module.

- Eric