Re: [PATCH v2 0/5] crypto: Speck support

[Theodore Y. Ts'o]

On Tue, Apr 24, 2018 at 10:58:35PM +0200, Jason A. Donenfeld wrote:
> > Note that Linux
> > doesn't bow down to any particular standards organization, and it offers
> > algorithms that were specified in various places, even some with no more 
> > than a
> > publication by the author.  In fact, support for SM4 was just added too, 
> > which
> > is a Chinese government standard.  Are you going to send a patch to remove 
> > that
> > too, or is it just NSA designed algorithms that are not okay?
> 
> No need to be belittling; I have much less tinfoil strapped around my
> head than perhaps you think. I'm not blindly opposed to
> government-designed algorithms. Take SHA2, for example -- built by the
> NSA.
> 
> But I do care quite a bit about using ciphers that have acceptance of
> the academic community and a large body of literature documenting its
> design decisions and analyzing it.

So where is the large body of literature documenting the design
decisions of SM2?  Has it received as much analysis as Speck?  And if
not, why aren't you gunning after it?

- Ted

RE: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support

[Dey, Megha]

>-Original Message-
>From: Herbert Xu [mailto:herb...@gondor.apana.org.au]
>Sent: Wednesday, April 18, 2018 8:25 PM
>To: Dey, Megha 
>Cc: linux-ker...@vger.kernel.org; linux-crypto@vger.kernel.org;
>da...@davemloft.net
>Subject: Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure
>support
>
>On Thu, Apr 19, 2018 at 12:54:16AM +, Dey, Megha wrote:
>>
>> Yeah I think I misunderstood. I think what you mean is to remove mcryptd.c
>completely and avoid the extra layer of indirection to call the underlying
>algorithm, instead call it directly, correct?
>>
>> So currently we have 3 algorithms registered for every multibuffer algorithm:
>> name : __sha1-mb
>> driver   : mcryptd(__intel_sha1-mb)
>>
>> name : sha1
>> driver   : sha1_mb
>>
>> name : __sha1-mb
>> driver   : __intel_sha1-mb
>>
>> If we remove mcryptd, then we will have just the 2?
>
>It should be down to just one, i.e., the current inner algorithm.
>It's doing all the scheduling work already so I don't really see why it needs 
>the
>wrappers around it.

Hi Herbert,

Is there any existing implementation of async crypto algorithm that uses the 
above approach? The ones I could find are either sync, have an outer and inner 
algorithm or use cryptd.

I tried removing the mcryptd layer and the outer algorithm and some plumbing to 
pass the correct structures, but see crashes.(obviously some errors in the 
plumbing)

I am not sure if we remove mcryptd, how would we queue work, flush partially 
completed jobs or call completions (currently done by mcryptd) if we simply 
call the inner algorithm.

Are you suggesting these are not required at all? I am not sure how to move 
forward.

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

Re: [PATCH v2 0/5] crypto: Speck support

[Eric Biggers]

Hi Jason,

On Tue, Apr 24, 2018 at 10:58:35PM +0200, Jason A. Donenfeld wrote:
> Hi Eric,
> 
> On Tue, Apr 24, 2018 at 8:16 PM, Eric Biggers  wrote:
> > So, what do you propose replacing it with?
> 
> Something more cryptographically justifiable.
> 

It's easy to say that, but do you have an actual suggestion?  As I mentioned,
for disk encryption without AES instructions the main alternatives we've
considered are ChaCha20 with reused nonces, an unpublished wide-block mode based
on ChaCha20 and Poly1305 (with no external cryptanalysis yet, and probably
actually using ChaCha8 or ChaCha12 to meet performance requirements), or the
status quo of no encryption at all.

It *might* be possible to add per-block metadata support to f2fs, in which it
could be used with ChaCha20 in fscrypt.  But if feasible at all it would be
quite difficult (requiring some significant filesystem surgery, and disabling
conflicting filesystem features that allow data to be updated in-place) and
would not cover dm-crypt, nor ext4.

Note also that many other lightweight block ciphers are designed for hardware
and perform poorly in software, e.g. PRESENT is even slower than AES.  Thus
there really weren't many options.

Any concrete suggestions are greatly appreciated!

> > outside crypto review, vs. the many cryptanalysis papers on Speck.  (In that
> > respect the controversy about Speck has actually become an advantage, as it 
> > has
> > received much more cryptanalysis than other lightweight block ciphers.)
> 
> That's the thing that worries me, actually. Many of the design
> decisions behind Speck haven't been justified.
> 

Originally that was true, but later there were significant clarifications
released, e.g. the paper "Notes on the design and analysis of Simon and Speck"
(https://eprint.iacr.org/2017/560.pdf).  In fact, from what I can see, many
competing lightweight block ciphers don't have as much design justification
available as Speck.  Daniel Bernstein's papers are excellent, but unfortunately
he has only designed a stream cipher, not a block cipher or another algorithm
that is applicable for disk encryption.

> > The reason we chose Speck had nothing to do with the proposed ISO standard 
> > or
> > any sociopolitical factors, but rather because it was the only algorithm we
> > could find that met the performance and security requirements.
> 
> > Note that Linux
> > doesn't bow down to any particular standards organization, and it offers
> > algorithms that were specified in various places, even some with no more 
> > than a
> > publication by the author.  In fact, support for SM4 was just added too, 
> > which
> > is a Chinese government standard.  Are you going to send a patch to remove 
> > that
> > too, or is it just NSA designed algorithms that are not okay?
> 
> No need to be belittling; I have much less tinfoil strapped around my
> head than perhaps you think. I'm not blindly opposed to
> government-designed algorithms. Take SHA2, for example -- built by the
> NSA.
> 
> But I do care quite a bit about using ciphers that have acceptance of
> the academic community and a large body of literature documenting its
> design decisions and analyzing it. Some of the best symmetric
> cryptographers in academia have expressed reservations about it, and
> it was just rejected from a major standard's body. Linux, of course,
> is free to disagree -- or "bow down" as you oddly put it -- but I'd
> make sure you've got a pretty large bucket of justifications for that
> disagreement.
> 

There have actually been many papers analyzing Speck.  As with other ciphers,
reduced-round variants have been successfully attacked, while the full variants
have held up.  This is expected.  It's true that some other ciphers such as
ChaCha20 have a higher security margin, which has resulted in some criticism of
Speck.  But the correct security margin is always debatable, and in a
performance-oriented cipher it's desirable to not have an excessive number of
rounds.  In fact it was even looking like ChaCha20 was not going to be fast
enough on some CPUs, so if we went the ChaCha route we may have actually have
had to use ChaCha12 or ChaCha8 instead.

Also, some papers present results for just the weakest variants of Speck
(Speck32 and Speck48) while omitting the strongest (Speck128, the one that's
planned to be offered for Android), presumably because the authors weren't able
to attack it as successfully.  I think that's causing some confusion.

I don't see how the ISO standarization process means much for crypto algorithms.
It seems very political, and actually it seems that people involved were pretty
clear that Speck was rejected primarily for political reasons.  Interestingly,
ChaCha20 is not an ISO standard either.  Does that mean ChaCha20 shouldn't be
used?

In any case, given that the status quo on low-end Android devices is no
encryption, it would be great to start having them be encrypted.  It would be a
shame if pushback 

Re: [PATCH v2 0/5] crypto: Speck support

[Jeffrey Walton]

On Tue, Apr 24, 2018 at 12:11 PM, Jason A. Donenfeld  wrote:
> Can we please not Speck?
>
> It was just rejected by the ISO/IEC.
>
> https://twitter.com/TomerAshur/status/988659711091228673

Yeah, but here was the reason given
(https://www.wikitribune.com/story/2018/04/20/internet/67004/67004/):

A source at an International Organization for Standardization (ISO)
meeting of expert delegations in Wuhan, China, told WikiTribune
that the U.S. delegation, including NSA officials, refused to provide
the standard level of technical information to proceed.

Jeff

Re: [PATCH v2 0/5] crypto: Speck support

[Paul Crowley]

On Tue, 24 Apr 2018 at 13:58, Jason A. Donenfeld  wrote:
> On Tue, Apr 24, 2018 at 8:16 PM, Eric Biggers  wrote:
> > So, what do you propose replacing it with?

> Something more cryptographically justifiable.

I'm keen to hear recommendations here, if there are options we should be
considering I'd like to know about them.

> That's the thing that worries me, actually. Many of the design
> decisions behind Speck haven't been justified.

It seems to me justified about as well as one would hope for a new cipher -
  "Notes on the design and analysis of Simon and Speck" seems to me to give
more detail on the reasoning than went into eg Salsa20, which I think also
hit a perfectly acceptable bar and was a good choice for adding to the
Linux kernel. Of course it's building on the fairly detailed understanding
we now have of how to build a secure ARX cipher. Given what a prize
cryptanalysis of an NSA-designed block cipher would be for anyone in the
field, the sheer simplicity and straightforwardness of the design, taken
with the very large gap between the full cipher and the best cryptanalysis,
and drawing on my own experience attacking Salsa20, I feel pretty good
about fielding this design. But if you have a specific alternative in mind
- a 128-bit block cipher (so we can use it in XTS mode) which is fast and
side-channel-free on ARM processors with NEON but without ARM CE - I'm very
keen to hear about it.

Could you say a little more about what it is that separates Speck from SM4
for you?

Thanks!

Re: [PATCH v2 0/5] crypto: Speck support

[Jason A. Donenfeld]

Hi Eric,

On Tue, Apr 24, 2018 at 8:16 PM, Eric Biggers  wrote:
> So, what do you propose replacing it with?

Something more cryptographically justifiable.

> outside crypto review, vs. the many cryptanalysis papers on Speck.  (In that
> respect the controversy about Speck has actually become an advantage, as it 
> has
> received much more cryptanalysis than other lightweight block ciphers.)

That's the thing that worries me, actually. Many of the design
decisions behind Speck haven't been justified.

> The reason we chose Speck had nothing to do with the proposed ISO standard or
> any sociopolitical factors, but rather because it was the only algorithm we
> could find that met the performance and security requirements.

> Note that Linux
> doesn't bow down to any particular standards organization, and it offers
> algorithms that were specified in various places, even some with no more than 
> a
> publication by the author.  In fact, support for SM4 was just added too, which
> is a Chinese government standard.  Are you going to send a patch to remove 
> that
> too, or is it just NSA designed algorithms that are not okay?

No need to be belittling; I have much less tinfoil strapped around my
head than perhaps you think. I'm not blindly opposed to
government-designed algorithms. Take SHA2, for example -- built by the
NSA.

But I do care quite a bit about using ciphers that have acceptance of
the academic community and a large body of literature documenting its
design decisions and analyzing it. Some of the best symmetric
cryptographers in academia have expressed reservations about it, and
it was just rejected from a major standard's body. Linux, of course,
is free to disagree -- or "bow down" as you oddly put it -- but I'd
make sure you've got a pretty large bucket of justifications for that
disagreement.

> (in fact, you'd
> probably have a different opinion of it if the authors had simply worked
> somewhere else and published the exact same algorithm);

Again, no need to patronize. I don't actually have a bias like that.

> But I hope you can understand that all *technical* indicators are that Speck 
> is
> secure enough

That's the thing I'm worried about.

Jason

Re: [PATCH] crypto: remove Speck

[Eric Biggers]

Hi Jason,

On Tue, Apr 24, 2018 at 06:18:26PM +0200, Jason A. Donenfeld wrote:
> This NSA-designed cipher was rejected for inclusion in international
> standards by ISO/IEC. Before anyone actually starts using it by
> accident, let's just not ship it at all.
> 
> Signed-off-by: Jason A. Donenfeld 
> ---
>  arch/arm/crypto/Kconfig |6 -
>  arch/arm/crypto/Makefile|2 -
>  arch/arm/crypto/speck-neon-core.S   |  432 
>  arch/arm/crypto/speck-neon-glue.c   |  288 --
>  arch/arm64/crypto/Kconfig   |6 -
>  arch/arm64/crypto/Makefile  |3 -
>  arch/arm64/crypto/speck-neon-core.S |  352 ---
>  arch/arm64/crypto/speck-neon-glue.c |  282 -
>  arch/s390/defconfig |1 -
>  crypto/Kconfig  |   14 -
>  crypto/Makefile |1 -
>  crypto/speck.c  |  307 --
>  crypto/testmgr.c|   36 -
>  crypto/testmgr.h| 1486 ---
>  include/crypto/speck.h  |   62 --
>  15 files changed, 3278 deletions(-)
>  delete mode 100644 arch/arm/crypto/speck-neon-core.S
>  delete mode 100644 arch/arm/crypto/speck-neon-glue.c
>  delete mode 100644 arch/arm64/crypto/speck-neon-core.S
>  delete mode 100644 arch/arm64/crypto/speck-neon-glue.c
>  delete mode 100644 crypto/speck.c
>  delete mode 100644 include/crypto/speck.h
> 

Please see my response on the other thread ("[PATCH v2 0/5] crypto: Speck 
support"):

https://marc.info/?l=linux-crypto-vger=152459378914647

Thanks,

- Eric

Re: [PATCH v2 0/5] crypto: Speck support

[Eric Biggers]

Hi Jason,

On Tue, Apr 24, 2018 at 06:11:26PM +0200, Jason A. Donenfeld wrote:
> Can we please not Speck?
> 
> It was just rejected by the ISO/IEC.
> 
> https://twitter.com/TomerAshur/status/988659711091228673

So, what do you propose replacing it with?

As I explained in the patch, the purpose of adding Speck is to allow low-end
Android devices -- ones that have CPUs without the ARMv8 Cryptography Extensions
-- to start using dm-crypt or fscrypt.  Currently such devices are unencrypted.
So, Speck is replacing *no encryption*, not another encryption algorithm.  By
removing Speck, you are removing encryption.  It's great that people are
enthusiastic about debating choices of crypto algorithms.  But it's unfortunate
that "no crypto" tends to pass by without comment from the same people.

We really wanted to use ChaCha20 instead.  But it would have been used in a
context where IVs are reused (f2fs encryption on flash storage), which
catastrophically breaks stream ciphers, but is less bad for a block cipher
operating in XTS mode.  Thus, we had to use either a block cipher, or a
wide-block encryption mode (pseudorandom permutation over the whole input).  Of
course, we would have liked to store nonces instead, but that is not currently
feasible with either dm-crypt or fscrypt.  It can be done with dm-crypt on top
of dm-integrity, but that performs very poorly and would be especially
inappropriate for low end mobile devices.

Paul Crowley actually designed a very neat wide-block encryption mode based on
ChaCha20 and Poly1305, which we considered too.  But it would have been harder
to implement, and we'd have had to be pushing it with zero or very little
outside crypto review, vs. the many cryptanalysis papers on Speck.  (In that
respect the controversy about Speck has actually become an advantage, as it has
received much more cryptanalysis than other lightweight block ciphers.)

The reason we chose Speck had nothing to do with the proposed ISO standard or
any sociopolitical factors, but rather because it was the only algorithm we
could find that met the performance and security requirements.  Note that Linux
doesn't bow down to any particular standards organization, and it offers
algorithms that were specified in various places, even some with no more than a
publication by the author.  In fact, support for SM4 was just added too, which
is a Chinese government standard.  Are you going to send a patch to remove that
too, or is it just NSA designed algorithms that are not okay?

It is unfortunate that we had to choose an algorithm that has some
emotional/political baggage attached, and of course we did expect some pushback.
But I hope you can understand that all *technical* indicators are that Speck is
secure enough, and not really backdoor-able other than a new cryptoanalytical
technique that would likely apply to other ARX ciphers as well (in fact, you'd
probably have a different opinion of it if the authors had simply worked
somewhere else and published the exact same algorithm); and also, the trend
towards stream ciphers such as ChaCha20 has mostly ignored the disk encryption
use case, where a block cipher is still needed.

Thanks,

Eric

Re: [PATCH v4 2/2] crypto: caam - allow retrieving 'era' from register

[Fabio Estevam]

Hi Herbert,

On Tue, Apr 24, 2018 at 1:39 PM, Herbert Xu  wrote:

> As this is a new device support issue I'd prefer to delay this
> until the next merge window.

Understood.

I will send a patch that passes ''fsl,sec-era' property in the dts, so
that we can have CAAM working on 4.17 fon i.MX7.

Thanks

Re: [PATCH v4 2/2] crypto: caam - allow retrieving 'era' from register

[Herbert Xu]

Hi Fabio:

On Fri, Apr 20, 2018 at 03:21:47PM -0300, Fabio Estevam wrote:
> 
> It is not a regression.
> 
> We haven't seen this problem before because dtsi files passed the
> 'fsl,sec-era' property.
> 
> Since 4.17-rc1, imx7 supports CAAM:
> 0eeabcad7da5  ("ARM: dts: imx7s: add CAAM device node")
> 
> ,but it does not pass the 'fsl,sec-era' property leading to the following 
> error:
> caam 3090.caam: device ID = 0x0a160300 (Era -524)
> 
> Documentation/devicetree/bindings/crypto/fsl-sec4.txt states that
> 'fsl,sec-era' property is optional, so 0eeabcad7da5 is not incorrect
> by not passing it.
> 
> As we can retrieve the era information by reading the CAAM registers
> we can fix the problem on imx7 running 4.17-rc with this patch.

As this is a new device support issue I'd prefer to delay this
until the next merge window.

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

Re: Question on random.c add_interrupt_randomness function

[Theodore Y. Ts'o]

On Tue, Apr 24, 2018 at 03:24:55PM +0200, Harald Freudenberger wrote:
> The condition is true and terminates the function
> when the count value of the cpu fast pool is below 64
> AND the time since last mix of the pool is lower than
> HZ (so lower than 1s).
> This means the code following this condition is runu
> when the count value is > 64 or the last mix is more
> than 1s old.
> As the fast_mix() function does a fast_pool->count++
> effectively every 64 invocations this condition is
> false and the rest of the function is executed.
> 
> Is this the intention?

Yes, that's the intention.  Originally we required sampling the IP
and/or architecture's cycle counter (if available) and mixing it using
fast_mix before we transfer it to the input_pool and credit it with a
bit of entropy.

The problem is that on some architectures interrupts happen are much
less frequently.  So what we did is to assume that one second's worth
of interrupts will be sufficient to derive a single bit of entropy.

   - Ted

Re: [PATCH v2 0/5] crypto: Speck support

[Jason A. Donenfeld]

Can we please not Speck?

It was just rejected by the ISO/IEC.

https://twitter.com/TomerAshur/status/988659711091228673

[PATCH] crypto: remove Speck

[Jason A. Donenfeld]

This NSA-designed cipher was rejected for inclusion in international
standards by ISO/IEC. Before anyone actually starts using it by
accident, let's just not ship it at all.

Signed-off-by: Jason A. Donenfeld 
---
 arch/arm/crypto/Kconfig |6 -
 arch/arm/crypto/Makefile|2 -
 arch/arm/crypto/speck-neon-core.S   |  432 
 arch/arm/crypto/speck-neon-glue.c   |  288 --
 arch/arm64/crypto/Kconfig   |6 -
 arch/arm64/crypto/Makefile  |3 -
 arch/arm64/crypto/speck-neon-core.S |  352 ---
 arch/arm64/crypto/speck-neon-glue.c |  282 -
 arch/s390/defconfig |1 -
 crypto/Kconfig  |   14 -
 crypto/Makefile |1 -
 crypto/speck.c  |  307 --
 crypto/testmgr.c|   36 -
 crypto/testmgr.h| 1486 ---
 include/crypto/speck.h  |   62 --
 15 files changed, 3278 deletions(-)
 delete mode 100644 arch/arm/crypto/speck-neon-core.S
 delete mode 100644 arch/arm/crypto/speck-neon-glue.c
 delete mode 100644 arch/arm64/crypto/speck-neon-core.S
 delete mode 100644 arch/arm64/crypto/speck-neon-glue.c
 delete mode 100644 crypto/speck.c
 delete mode 100644 include/crypto/speck.h

diff --git a/arch/arm/crypto/Kconfig b/arch/arm/crypto/Kconfig
index 925d1364727a..b8e69fe282b8 100644
--- a/arch/arm/crypto/Kconfig
+++ b/arch/arm/crypto/Kconfig
@@ -121,10 +121,4 @@ config CRYPTO_CHACHA20_NEON
select CRYPTO_BLKCIPHER
select CRYPTO_CHACHA20
 
-config CRYPTO_SPECK_NEON
-   tristate "NEON accelerated Speck cipher algorithms"
-   depends on KERNEL_MODE_NEON
-   select CRYPTO_BLKCIPHER
-   select CRYPTO_SPECK
-
 endif
diff --git a/arch/arm/crypto/Makefile b/arch/arm/crypto/Makefile
index 8de542c48ade..bd5bceef0605 100644
--- a/arch/arm/crypto/Makefile
+++ b/arch/arm/crypto/Makefile
@@ -10,7 +10,6 @@ obj-$(CONFIG_CRYPTO_SHA1_ARM_NEON) += sha1-arm-neon.o
 obj-$(CONFIG_CRYPTO_SHA256_ARM) += sha256-arm.o
 obj-$(CONFIG_CRYPTO_SHA512_ARM) += sha512-arm.o
 obj-$(CONFIG_CRYPTO_CHACHA20_NEON) += chacha20-neon.o
-obj-$(CONFIG_CRYPTO_SPECK_NEON) += speck-neon.o
 
 ce-obj-$(CONFIG_CRYPTO_AES_ARM_CE) += aes-arm-ce.o
 ce-obj-$(CONFIG_CRYPTO_SHA1_ARM_CE) += sha1-arm-ce.o
@@ -54,7 +53,6 @@ ghash-arm-ce-y:= ghash-ce-core.o ghash-ce-glue.o
 crct10dif-arm-ce-y := crct10dif-ce-core.o crct10dif-ce-glue.o
 crc32-arm-ce-y:= crc32-ce-core.o crc32-ce-glue.o
 chacha20-neon-y := chacha20-neon-core.o chacha20-neon-glue.o
-speck-neon-y := speck-neon-core.o speck-neon-glue.o
 
 ifdef REGENERATE_ARM_CRYPTO
 quiet_cmd_perl = PERL$@
diff --git a/arch/arm/crypto/speck-neon-core.S 
b/arch/arm/crypto/speck-neon-core.S
deleted file mode 100644
index 3c1e203e53b9..
--- a/arch/arm/crypto/speck-neon-core.S
+++ /dev/null
@@ -1,432 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * NEON-accelerated implementation of Speck128-XTS and Speck64-XTS
- *
- * Copyright (c) 2018 Google, Inc
- *
- * Author: Eric Biggers 
- */
-
-#include 
-
-   .text
-   .fpuneon
-
-   // arguments
-   ROUND_KEYS  .reqr0  // const {u64,u32} *round_keys
-   NROUNDS .reqr1  // int nrounds
-   DST .reqr2  // void *dst
-   SRC .reqr3  // const void *src
-   NBYTES  .reqr4  // unsigned int nbytes
-   TWEAK   .reqr5  // void *tweak
-
-   // registers which hold the data being encrypted/decrypted
-   X0  .reqq0
-   X0_L.reqd0
-   X0_H.reqd1
-   Y0  .reqq1
-   Y0_H.reqd3
-   X1  .reqq2
-   X1_L.reqd4
-   X1_H.reqd5
-   Y1  .reqq3
-   Y1_H.reqd7
-   X2  .reqq4
-   X2_L.reqd8
-   X2_H.reqd9
-   Y2  .reqq5
-   Y2_H.reqd11
-   X3  .reqq6
-   X3_L.reqd12
-   X3_H.reqd13
-   Y3  .reqq7
-   Y3_H.reqd15
-
-   // the round key, duplicated in all lanes
-   ROUND_KEY   .reqq8
-   ROUND_KEY_L .reqd16
-   ROUND_KEY_H .reqd17
-
-   // index vector for vtbl-based 8-bit rotates
-   ROTATE_TABLE.reqd18
-
-   // multiplication table for updating XTS tweaks
-   GF128MUL_TABLE  .reqd19
-   GF64MUL_TABLE   .reqd19
-
-   // current XTS tweak value(s)
-   TWEAKV  .reqq10
-   TWEAKV_L.reqd20
-   TWEAKV_H.reqd21
-
-   TMP0.reqq12
-   TMP0_L  .reqd24
-   TMP0_H  .reqd25
-   TMP1.reqq13
- 

Re: [PATCH] crypto: ccree: limit build to plausible archs

[Gilad Ben-Yossef]

On Tue, Apr 24, 2018 at 11:52 AM, Geert Uytterhoeven
 wrote:

>
> My underlying idea is not to cut down build time for test code (that's what
> we have COMPILE_TEST for), but to enhance usability for users and distros,
> who need to know if it makes sense to enable an option.

OK, considering the driver Kconfig depends on OF I think we're OK here
with regard to usability and distros.

>
> IMHO the extensive list of possible architectures is not really an
> improvement. So either a dependency on COMPILE_TEST, or no dependency
> at all makes the most sense to me.

Fair enough. Herbert, please drop the  patch than.

Thanks!
Gilad
-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Question on random.c add_interrupt_randomness function

[Harald Freudenberger]

Hello Theodore,
I am currently investigating a better implementation of
the arch_get_random_long_seed() implementation for s390.
And so I stumbled over the function add_interrupt_randomness()
in random.c and have one question regarding this code:

void add_interrupt_randomness(int irq, int irq_flags)
{
    ...
    fast_mix(fast_pool);
    ...
    if ((fast_pool->count < 64) &&
        !time_after(now, fast_pool->last + HZ))
        return;
    ...
}

The condition is true and terminates the function
when the count value of the cpu fast pool is below 64
AND the time since last mix of the pool is lower than
HZ (so lower than 1s).
This means the code following this condition is run
when the count value is > 64 or the last mix is more
than 1s old.
As the fast_mix() function does a fast_pool->count++
effectively every 64 invocations this condition is
false and the rest of the function is executed.

Is this the intention? Shouldn't the condition
terminate the function either when there are fewer
than 64 mixes in the pool OR time since last
invocation is < 1 s ?

regards
Harald Freudenberger

Re: [PATCH] crypto: ccree: limit build to plausible archs

[Geert Uytterhoeven]

Hi Gilad,

On Tue, Apr 24, 2018 at 10:31 AM, Gilad Ben-Yossef  wrote:
> On Mon, Apr 23, 2018 at 8:53 PM, Geert Uytterhoeven
>  wrote:
>> On Mon, Apr 23, 2018 at 3:22 PM, Gilad Ben-Yossef  
>> wrote:
>>> On Mon, Apr 23, 2018 at 3:13 PM, Geert Uytterhoeven
>>>  wrote:
 On Mon, Apr 23, 2018 at 1:48 PM, Gilad Ben-Yossef  
 wrote:
> Limit option to compile ccree to plausible architectures.

 Thanks for your patch!

> Suggested-by: Geert Uytterhoeven 
> Signed-off-by: Gilad Ben-Yossef 
> ---
>  drivers/crypto/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index d1ea1a0..7302785 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -726,6 +726,7 @@ config CRYPTO_DEV_ARTPEC6
>  config CRYPTO_DEV_CCREE
> tristate "Support for ARM TrustZone CryptoCell family of security 
> processors"
> depends on CRYPTO && CRYPTO_HW && OF && HAS_DMA
> +   depends on (XTENSA || X86 || UNICORE32 || SUPERH || RISCV || 
> PPC32 || OPENRISC || NIOS2 || NDS32 || MIPS || MICROBLAZE || HEXAGON || 
> H8300 || ARM || ARM64 || ARC || COMPILE_TEST)

 That list looks a bit excessive to me...
>>>
>>> I'm sorry, but as an Arm employee I'm not in liberty to identify which
>>> customer licensed or might license CryptoCell for which platform, now
>>> or in the future.
>>>
>>> I'm sure you understand.

>> What about using "depends on  || COMPILE_TEST", with  the
>> platforms for which the DTS (incl. "arm,cryptocell-*") will be submitted
>> for v4.18? The list can easily be extended when needed.

> Seriously though, I think I was not efficient in communicating my
> point through. Let me try again:

Perhaps I also wasn't clear. With "platform dependency", I meant not just
architectures (e.g. ARM64), but also platform families (e.g. ARCH_EXYNOS).

> The boards that came out last year (which I plan to send DT entries
> for)  are using CryptoCell 630p, which we shipped as a design IP a few
> years ago. It takes years for an IP design to ship, be designed into
> SoCs, for these SoCs to ship and be designed into boards and until
> these ship... and when they do, they almost always don't use the
> cutting edge latest kernel.

OK, so the list can be extended when you send the DTS for that...

> That is to say - I don't know which SoC and using which architecture
> the CryptoCell IP was and will be designed into but I want the driver
> and the kernel to be ready when they do - this IS after all the whole
> point o doing stuff Upstream First(tm)!

... and IMHO an initial empty list is fine, as it will still be upstream, and
compile-tested by the bots.

(note: I do love Upstream First(tm)!)

> This is the same of having random PCI devices depend on PCI and not a
> specific architecture even if no one shipped a system with that device
> yet and again the same with I2C devices depending on I2C and not a
> specific architecture even if we don't have a board out with that I2C
> device designed it. It's a generic none architecture dependent
> component.

At least those depend on PCI or I2C, and thus won't show up on machines
without PCI or I2C.

> This is why I initially did not mark the device driver as depending on
> any specific architecture.
>
> In light of your request, and what I believe is the underlying idea to
> cut down as much as possible build time for test code, to which I
> agree, I've decided to cut out architecture that there is very little
> chance to even go into a SoC with CryptoCell, such as s390 or um.

My underlying idea is not to cut down build time for test code (that's what
we have COMPILE_TEST for), but to enhance usability for users and distros,
who need to know if it makes sense to enable an option.

> So, any pointer to an architecture that will not likely to go into a
> SoC in the coming years with some off the shelf HW IP so I can cut off
> more is very welcome, but using currently shipping boards to indicate
> which architecture to support is not appropriate.
>
> I hope this makes things a bit clearer.

IMHO the extensive list of possible architectures is not really an
improvement. So either a dependency on COMPILE_TEST, or no dependency
at all makes the most sense to me.

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH] crypto: ccree: limit build to plausible archs

[Gilad Ben-Yossef]

On Mon, Apr 23, 2018 at 8:53 PM, Geert Uytterhoeven
 wrote:
> Hi Gilad,
>
> On Mon, Apr 23, 2018 at 3:22 PM, Gilad Ben-Yossef  wrote:
>> On Mon, Apr 23, 2018 at 3:13 PM, Geert Uytterhoeven
>>  wrote:
>>> On Mon, Apr 23, 2018 at 1:48 PM, Gilad Ben-Yossef  
>>> wrote:
 Limit option to compile ccree to plausible architectures.
>>>
>>> Thanks for your patch!
>>>
 Suggested-by: Geert Uytterhoeven 
 Signed-off-by: Gilad Ben-Yossef 
 ---
  drivers/crypto/Kconfig | 1 +
  1 file changed, 1 insertion(+)

 diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
 index d1ea1a0..7302785 100644
 --- a/drivers/crypto/Kconfig
 +++ b/drivers/crypto/Kconfig
 @@ -726,6 +726,7 @@ config CRYPTO_DEV_ARTPEC6
  config CRYPTO_DEV_CCREE
 tristate "Support for ARM TrustZone CryptoCell family of security 
 processors"
 depends on CRYPTO && CRYPTO_HW && OF && HAS_DMA
 +   depends on (XTENSA || X86 || UNICORE32 || SUPERH || RISCV || PPC32 
 || OPENRISC || NIOS2 || NDS32 || MIPS || MICROBLAZE || HEXAGON || H8300 || 
 ARM || ARM64 || ARC || COMPILE_TEST)
>>>
>>> That list looks a bit excessive to me...
>>
>> I'm sorry, but as an Arm employee I'm not in liberty to identify which
>> customer licensed or might license CryptoCell for which platform, now
>> or in the future.
>>
>> I'm sure you understand.
>
> IC, a clever marketing scheme to make everyone think that everybody else
> is already a licensee ;-)

I think you are placing too much importance on Kconfig dependencies
marketing power, otherwise I would have long ago rented Kconfig space
to Saatchi & Saatchi - e.g. "This Kconfig entry is brought to you
by...
"  :-)

>
> What about using "depends on  || COMPILE_TEST", with  the
> platforms for which the DTS (incl. "arm,cryptocell-*") will be submitted
> for v4.18? The list can easily be extended when needed.
>

Seriously though, I think I was not efficient in communicating my
point through. Let me try again:

The CryptoCell 712 doesn't exists as silicon anywhere. It's just been
released to our design partners not too long ago.
I am guessing there are a few SoCs which include CryptoCell 710 which
taped out, but probably no publicly available boards yet.
By the standard you set above, we shouldn't be enabling the driver
build on ANY platform...

The boards that came out last year (which I plan to send DT entries
for)  are using CryptoCell 630p, which we shipped as a design IP a few
years ago. It takes years for an IP design to ship, be designed into
SoCs, for these SoCs to ship and be designed into boards and until
these ship... and when they do, they almost always don't use the
cutting edge latest kernel.

That is to say - I don't know which SoC and using which architecture
the CryptoCell IP was and will be designed into but I want the driver
and the kernel to be ready when they do - this IS after all the whole
point o doing stuff Upstream First(tm)!

This is the same of having random PCI devices depend on PCI and not a
specific architecture even if no one shipped a system with that device
yet and again the same with I2C devices depending on I2C and not a
specific architecture even if we don't have a board out with that I2C
device designed it. It's a generic none architecture dependent
component.

This is why I initially did not mark the device driver as depending on
any specific architecture.
In light of your request, and what I believe is the underlying idea to
cut down as much as possible build time for test code, to which I
agree, I've decided to cut out architecture that there is very little
chance to even go into a SoC with CryptoCell, such as s390 or um.

So, any pointer to an architecture that will not likely to go into a
SoC in the coming years with some off the shelf HW IP so I can cut off
more is very welcome, but using currently shipping boards to indicate
which architecture to support is not appropriate.

I hope this makes things a bit clearer.

Thanks,
Gilad




-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru