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

[Eric Biggers]

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

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

[Ard Biesheuvel]

On 28 March 2017 at 06:43, Eric Biggers  wrote:
> Hi Ard,
>
> On Sun, Mar 26, 2017 at 07:49:01PM +0100, Ard Biesheuvel wrote:
>> The generic AES driver uses 16 lookup tables of 1 KB each, and has
>> encryption and decryption routines that are fully unrolled. Given how
>> the dependencies between this code and other drivers are declared in
>> Kconfig files, this code is always pulled into the core kernel, even
>> if it is usually superseded at runtime by accelerated drivers that
>> exist for many architectures.
>>
>> This leaves us with 25 KB of dead code in the kernel, which is negligible
>> in typical environments, but which is actually a big deal for the IoT
>> domain, where every kilobyte counts.
>>
>> For this reason, this series refactors the way the various AES
>> implementations are wired up, to allow the generic version in
>> crypto/aes_generic.c to be omitted from the build entirely.
>>
>> Patch #1 removes some bogus 'select CRYPTO_AES' statement.
>>
>> Patch #2 introduces CRYPTO_NEED_AES which can be selected by driver that
>> require an AES cipher to be available, but don't care how it is implemented.
>>
>> Patches #3 and #4 make some preparatory changes that allow dependencies on
>> crypto_aes_expand_key to be fulfilled by the new (and much smaller) fixed
>> time AES driver. (#5)
>>
>> Patch #6 splits the generic AES driver into a core containing the precomputed
>> sub/shift/mix tables and the key expansion routines on the one hand, and the
>> encryption/decryption routines and the crypto API registration on the other.
>>
>> Patch #7 introduces the CRYPTO_HAVE_AES Kconfig symbol, and adds statements 
>> to
>> various AES implementations that can fulfil the CRYPTO_NEED_AES dependencies
>> added in patch #2. The introduced Kconfig logic allows CRYPTO_AES to be
>> deselected even if AES dependencies exist, as long as one of these 
>> alternatives
>> is selected.
>
> 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.

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

[Eric Biggers]

Hi Ard,

On Sun, Mar 26, 2017 at 07:49:01PM +0100, Ard Biesheuvel wrote:
> The generic AES driver uses 16 lookup tables of 1 KB each, and has
> encryption and decryption routines that are fully unrolled. Given how
> the dependencies between this code and other drivers are declared in
> Kconfig files, this code is always pulled into the core kernel, even
> if it is usually superseded at runtime by accelerated drivers that
> exist for many architectures.
> 
> This leaves us with 25 KB of dead code in the kernel, which is negligible
> in typical environments, but which is actually a big deal for the IoT
> domain, where every kilobyte counts.
> 
> For this reason, this series refactors the way the various AES
> implementations are wired up, to allow the generic version in
> crypto/aes_generic.c to be omitted from the build entirely.
> 
> Patch #1 removes some bogus 'select CRYPTO_AES' statement.
> 
> Patch #2 introduces CRYPTO_NEED_AES which can be selected by driver that
> require an AES cipher to be available, but don't care how it is implemented.
> 
> Patches #3 and #4 make some preparatory changes that allow dependencies on
> crypto_aes_expand_key to be fulfilled by the new (and much smaller) fixed
> time AES driver. (#5)
> 
> Patch #6 splits the generic AES driver into a core containing the precomputed
> sub/shift/mix tables and the key expansion routines on the one hand, and the
> encryption/decryption routines and the crypto API registration on the other.
> 
> Patch #7 introduces the CRYPTO_HAVE_AES Kconfig symbol, and adds statements to
> various AES implementations that can fulfil the CRYPTO_NEED_AES dependencies
> added in patch #2. The introduced Kconfig logic allows CRYPTO_AES to be
> deselected even if AES dependencies exist, as long as one of these 
> alternatives
> is selected.

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.

- Eric

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

[Nicolas Pitre]

On Sun, 26 Mar 2017, Ard Biesheuvel wrote:

> The generic AES driver uses 16 lookup tables of 1 KB each, and has
> encryption and decryption routines that are fully unrolled. Given how
> the dependencies between this code and other drivers are declared in
> Kconfig files, this code is always pulled into the core kernel, even
> if it is usually superseded at runtime by accelerated drivers that
> exist for many architectures.
> 
> This leaves us with 25 KB of dead code in the kernel, which is negligible
> in typical environments, but which is actually a big deal for the IoT
> domain, where every kilobyte counts.
> 
> For this reason, this series refactors the way the various AES
> implementations are wired up, to allow the generic version in
> crypto/aes_generic.c to be omitted from the build entirely.

I'm not a cryptographer but I do agree with the above goal.

Acked_by: Nicolas Pitre 

> 
> Patch #1 removes some bogus 'select CRYPTO_AES' statement.
> 
> Patch #2 introduces CRYPTO_NEED_AES which can be selected by driver that
> require an AES cipher to be available, but don't care how it is implemented.
> 
> Patches #3 and #4 make some preparatory changes that allow dependencies on
> crypto_aes_expand_key to be fulfilled by the new (and much smaller) fixed
> time AES driver. (#5)
> 
> Patch #6 splits the generic AES driver into a core containing the precomputed
> sub/shift/mix tables and the key expansion routines on the one hand, and the
> encryption/decryption routines and the crypto API registration on the other.
> 
> Patch #7 introduces the CRYPTO_HAVE_AES Kconfig symbol, and adds statements to
> various AES implementations that can fulfil the CRYPTO_NEED_AES dependencies
> added in patch #2. The introduced Kconfig logic allows CRYPTO_AES to be
> deselected even if AES dependencies exist, as long as one of these 
> alternatives
> is selected.
> 
> Ard Biesheuvel (7):
>   drivers/crypto/Kconfig: drop bogus CRYPTO_AES dependencies
>   crypto: aes - add new Kconfig symbol for soft dependency on AES
>   crypto: aes/x86 - eliminate set_key() handling for IRQ context
>   crypto: aes/arm64 - eliminate dependency on crypto_aes_set_key()
>   crypto: aes - move crypto_aes_expand_key() to fixed-time AES driver
>   crypto: aes - split off shared AES tables and key expansion routines
>   crypto: aes - allow alternative AES drivers to fulfil AES dependency
> 
>  arch/arm/crypto/Kconfig  |5 +-
>  arch/arm64/crypto/Kconfig|5 +-
>  arch/arm64/crypto/aes-glue.c |   12 +-
>  arch/x86/crypto/aesni-intel_glue.c   |   14 +-
>  crypto/Kconfig   |   25 +-
>  crypto/Makefile  |1 +
>  crypto/aes_core.c| 1302 
>  crypto/aes_generic.c | 1239 ---
>  crypto/aes_ti.c  |7 +-
>  drivers/block/Kconfig|2 +-
>  drivers/crypto/Kconfig   |   21 +-
>  drivers/net/Kconfig  |2 +-
>  drivers/net/wireless/cisco/Kconfig   |2 +-
>  drivers/net/wireless/intel/ipw2x00/Kconfig   |2 +-
>  drivers/net/wireless/intersil/hostap/Kconfig |2 +-
>  drivers/staging/rtl8192e/Kconfig |2 +-
>  drivers/usb/wusbcore/Kconfig |2 +-
>  fs/ceph/Kconfig  |2 +-
>  fs/cifs/Kconfig  |2 +-
>  fs/crypto/Kconfig|2 +-
>  net/Kconfig  |2 +-
>  net/bluetooth/Kconfig|2 +-
>  net/ceph/Kconfig |2 +-
>  net/mac80211/Kconfig |2 +-
>  net/mac802154/Kconfig|2 +-
>  net/sunrpc/Kconfig   |3 +-
>  security/keys/Kconfig|4 +-
>  27 files changed, 1377 insertions(+), 1291 deletions(-)
>  create mode 100644 crypto/aes_core.c
> 
> -- 
> 2.7.4
> 
>