On Tue, Mar 28, 2017 at 09:51:54AM +0100, Ard Biesheuvel wrote:
> On 28 March 2017 at 06:43, Eric Biggers <ebigge...@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

Reply via email to