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 1/1] crypto: If two strings are exact match, they must have same length.

[Ming Ma (mingma)]

Please ignore this patch, we have seen some issues in older verison of linux 
kernel. But it doesn't seem to be an issue in the latest kernel.

thanks

-Original Message-
From: linux-crypto-ow...@vger.kernel.org 
[mailto:linux-crypto-ow...@vger.kernel.org] On Behalf Of Herbert Xu
Sent: Tuesday, March 21, 2017 8:01 PM
To: Ming Ma (mingma) 
Cc: da...@davemloft.net; linux-crypto@vger.kernel.org; 
linux-ker...@vger.kernel.org
Subject: Re: [PATCH 1/1] crypto: If two strings are exact match, they must have 
same length.

On Tue, Mar 21, 2017 at 04:40:40PM -0500, Ming Ma wrote:
> When both "crct10dif-pclmul" algorithm and "crct10dif-generic" 
> algorithm exist in crypto_alg_list, "crct10dif-pclmul" should be 
> selected, since it has higher priority than "crct10dif-generic". 
> However, both algorithms have the same cra_name "crct10dif". If we use 
> "crct10dif" to find a matched algorithm in crypto_alg_list, it's 
> possible "crct10dif-generic" is selected, because the code calls 
> strcmp to decide if two string are exact match, but doesn't check if two 
> strings have the same length.
> 
> exact = !strcmp(q->cra_driver_name, name);
> 
> So ,if "crct10dif-generic" is in front of "crct10dif-pclmul" in 
> crypto_alg_list, it will be picked as the matched algorithm, even if 
> it has lower priority than "crct10dif-pclmul".
> Signed-off-by: Ming Ma 
> ---
>  crypto/api.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/crypto/api.c b/crypto/api.c index b16ce16..5b3d45a 100644
> --- a/crypto/api.c
> +++ b/crypto/api.c
> @@ -76,7 +76,8 @@ static struct crypto_alg *__crypto_alg_lookup(const char 
> *name, u32 type,
>   ((struct crypto_larval *)q)->mask != mask)
>   continue;
>  
> - exact = !strcmp(q->cra_driver_name, name);
> + exact = (strlen(name) == strlen(q->cra_driver_name)) &&
> + !strcmp(q->cra_driver_name, name);
>   fuzzy = !strcmp(q->cra_name, name);
>   if (!exact && !(fuzzy && q->cra_priority > best))
>   continue;

This is bogus.  Please describe how you reproduced the problem.

The priority matching should work.

Cheers,
--
Email: Herbert Xu  Home Page: 
http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC PATCH v2 15/32] x86: Add support for changing memory encryption attribute in early boot

[Brijesh Singh]

Hi Boris,

On 03/24/2017 12:12 PM, Borislav Petkov wrote:

 }

+static inline int __init early_set_memory_decrypted(void *addr,
+   unsigned long size)
+{
+   return 1;



return 1 when !CONFIG_AMD_MEM_ENCRYPT ?

The non-early variants return 0.



I will fix it and use the same return value.


+}
+
+static inline int __init early_set_memory_encrypted(void *addr,
+   unsigned long size)
+{
+   return 1;
+}
+
 #define __sme_pa   __pa



+   unsigned long pfn, npages;
+   unsigned long addr = (unsigned long)vaddr & PAGE_MASK;
+
+   /* We are going to change the physical page attribute from C=1 to C=0.
+* Flush the caches to ensure that all the data with C=1 is flushed to
+* memory. Any caching of the vaddr after function returns will
+* use C=0.
+*/


Kernel comments style is:

/*
 * A sentence ending with a full-stop.
 * Another sentence. ...
 * More sentences. ...
 */



I will update to use kernel comment style.



+   clflush_cache_range(vaddr, size);
+
+   npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
+   pfn = slow_virt_to_phys((void *)addr) >> PAGE_SHIFT;
+
+   return kernel_map_pages_in_pgd(init_mm.pgd, pfn, addr, npages,
+   flags & ~sme_me_mask);
+
+}
+
+int __init early_set_memory_decrypted(void *vaddr, unsigned long size)
+{
+   unsigned long flags = get_pte_flags((unsigned long)vaddr);


So this does lookup_address()...


+   return early_set_memory_enc_dec(vaddr, size, flags & ~sme_me_mask);


... and this does it too in slow_virt_to_phys(). So you do it twice per
vaddr.

So why don't you define a __slow_virt_to_phys() helper - notice
the "__" - which returns flags in its second parameter and which
slow_virt_to_phys() calls with a NULL second parameter in the other
cases?



I will look into creating a helper function. thanks

-Brijesh

Re: [PATCH v3 1/3] crypto: hw_random - Add new Exynos RNG driver

[Stephan Müller]

Am Montag, 27. März 2017, 06:23:11 CEST schrieb PrasannaKumar Muralidharan:

Hi PrasannaKumar,

> > Oh my, if you are right with your first guess, this is a bad DRNG design.
> > 
> > Just out of curiousity: what happens if a caller invokes the seed function
> > twice or more times (each time with the sufficient amount of bits)? What
> > is
> > your guess here?
> 
> Should the second seed use the random data generated by the device?

A DRNG should be capable of processing an arbitrary amount of seed data. It 
may be the case that the seed data must be processed in chunks though.

That said, it may be the case that after injecting one chunk of seed the 
currently discussed RNG simply needs to generate a random number to process 
the input data before another seed can be added. But that is pure speculation.

But I guess that can be easily tested: inject a known seed into the DRNG, 
generate a random number, inject the same seed again and again generate a 
random number. If both are identical (which I do not hope), then the internal 
state is simply overwritten (strange DRNG design).

A similar test can be made to see whether a larger set of seed simply 
overwrites the state or is really processed.

1. seed
2. generate random data
3. reset
4. seed with anther seed
5. generate random data
6. reset
7. seed with same data from 1
8. seed with same data from 2
9. generate random data

If data from 9 is identical to 2, then additional seed data is discarded -> 
bad design. If data from 9 is identical to 5, then the additional data 
overwrites the initial data -> bad DRNG design. If data from 9 neither matches 
2 or 5, then all seed is taken -> good design.

Ciao
Stephan