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

2017-03-25 Thread Stephan Müller 
Am Samstag, 25. März 2017, 08:36:48 CET schrieb Krzysztof Kozlowski:

Hi Krzysztof,

> On Fri, Mar 24, 2017 at 09:41:59PM +0100, Stephan Müller wrote:
> > Am Freitag, 24. März 2017, 19:26:04 CET schrieb Krzysztof Kozlowski:
> > 
> > Hi Krzysztof,
> > 
> > > +static unsigned int exynos_rng_copy_random(struct exynos_rng_dev *rng,
> > > +u8 *dst, unsigned int dlen)
> > > +{
> > > + unsigned int cnt = 0;
> > > + int i, j;
> > > + u32 val;
> > > +
> > > + for (j = 0; j < EXYNOS_RNG_SEED_REGS; j++) {
> > > + val = exynos_rng_readl(rng, EXYNOS_RNG_OUT(j));
> > > +
> > > + for (i = 0; i < 4; i++) {
> > > + dst[cnt] = val & 0xff;
> > > + val >>= 8;
> > > + if (++cnt >= dlen)
> > > + return cnt;
> > > + }
> > > + rng->seed_save[j] = val;
> > 
> > Just to clarify: is this call right? Shouldn't that be removed? Any RNG
> > that is given to a caller is tainted and should not serve as seed.
> 
> In that case I could either re-use RNGs not passed to the caller (like
> in the block quoted below) or generate another round of them just for
> purpose of next seeding.
> 
> With the first approach the problem is that I might wait for such unused
> numbers pretty long. If user is requesting large amount of data, then I
> will always give him all five output numbers. I will not have unused
> numbers.
> 
> The second approach seems safe, but requires additional engine run which
> will slow down some of the generate() calls.

Random numbers should never be used twice.
> 
> > > + }
> > > +
> > > + /*
> > > +  * Engine filled all output registers, so read the remaining registers
> > > +  * for storing data as future seed.
> > > +  */
> > > + for (; j < EXYNOS_RNG_SEED_REGS; j++)
> > > + rng->seed_save[j] = exynos_rng_readl(rng, EXYNOS_RNG_OUT(j));
> > 
> > With this call, I guess the questioned line above could go away, right?
> 
> This is used in combination with the previous line so I will get five
> seeds (for five registers).
> 
> Best regards,
> Krzysztof


Ciao
Stephan

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

2017-03-25 Thread Krzysztof Kozlowski 
On Fri, Mar 24, 2017 at 09:41:59PM +0100, Stephan Müller wrote:
> Am Freitag, 24. März 2017, 19:26:04 CET schrieb Krzysztof Kozlowski:
> 
> Hi Krzysztof,
> 
> > +static unsigned int exynos_rng_copy_random(struct exynos_rng_dev *rng,
> > +  u8 *dst, unsigned int dlen)
> > +{
> > +   unsigned int cnt = 0;
> > +   int i, j;
> > +   u32 val;
> > +
> > +   for (j = 0; j < EXYNOS_RNG_SEED_REGS; j++) {
> > +   val = exynos_rng_readl(rng, EXYNOS_RNG_OUT(j));
> > +
> > +   for (i = 0; i < 4; i++) {
> > +   dst[cnt] = val & 0xff;
> > +   val >>= 8;
> > +   if (++cnt >= dlen)
> > +   return cnt;
> > +   }
> > +   rng->seed_save[j] = val;
> 
> Just to clarify: is this call right? Shouldn't that be removed? Any RNG that 
> is given to a caller is tainted and should not serve as seed.

In that case I could either re-use RNGs not passed to the caller (like
in the block quoted below) or generate another round of them just for
purpose of next seeding.

With the first approach the problem is that I might wait for such unused
numbers pretty long. If user is requesting large amount of data, then I
will always give him all five output numbers. I will not have unused
numbers.

The second approach seems safe, but requires additional engine run which
will slow down some of the generate() calls.

> > +   }
> > +
> > +   /*
> > +* Engine filled all output registers, so read the remaining registers
> > +* for storing data as future seed.
> > +*/
> > +   for (; j < EXYNOS_RNG_SEED_REGS; j++)
> > +   rng->seed_save[j] = exynos_rng_readl(rng, EXYNOS_RNG_OUT(j));
> 
> With this call, I guess the questioned line above could go away, right?

This is used in combination with the previous line so I will get five
seeds (for five registers).

Best regards,
Krzysztof

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

2017-03-24 Thread Stephan Müller 
Am Freitag, 24. März 2017, 19:26:04 CET schrieb Krzysztof Kozlowski:

Hi Krzysztof,

> +static unsigned int exynos_rng_copy_random(struct exynos_rng_dev *rng,
> +u8 *dst, unsigned int dlen)
> +{
> + unsigned int cnt = 0;
> + int i, j;
> + u32 val;
> +
> + for (j = 0; j < EXYNOS_RNG_SEED_REGS; j++) {
> + val = exynos_rng_readl(rng, EXYNOS_RNG_OUT(j));
> +
> + for (i = 0; i < 4; i++) {
> + dst[cnt] = val & 0xff;
> + val >>= 8;
> + if (++cnt >= dlen)
> + return cnt;
> + }
> + rng->seed_save[j] = val;

Just to clarify: is this call right? Shouldn't that be removed? Any RNG that 
is given to a caller is tainted and should not serve as seed.
> + }
> +
> + /*
> +  * Engine filled all output registers, so read the remaining registers
> +  * for storing data as future seed.
> +  */
> + for (; j < EXYNOS_RNG_SEED_REGS; j++)
> + rng->seed_save[j] = exynos_rng_readl(rng, EXYNOS_RNG_OUT(j));

With this call, I guess the questioned line above could go away, right?


Ciao
Stephan

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

2017-03-24 Thread Krzysztof Kozlowski 
Replace existing hw_ranndom/exynos-rng driver with a new, reworked one.
This is a driver for pseudo random number generator block which on
Exynos4 chipsets must be seeded with some value.  On newer Exynos5420
chipsets it might seed itself from true random number generator block
but this is not implemented yet.

New driver is a complete rework to use the crypto ALGAPI instead of
hw_random API.  Rationale for the change:
1. hw_random interface is for true RNG devices.
2. The old driver was seeding itself with jiffies which is not a
   reliable source for randomness.
3. Device generates five random numbers in each pass but old driver was
   returning only one thus its performance was reduced.

Compatibility with DeviceTree bindings is preserved.

New driver does not use runtime power management but manually enables
and disables the clock when needed.  This is preferred approach because
using runtime PM just to toggle clock is huge overhead.

Another difference is reseeding itself with generated random data
periodically and during resuming from system suspend (previously driver
was re-seeding itself again with jiffies).

Signed-off-by: Krzysztof Kozlowski 

---

The resume path is untested as my Odroid U3 fails to go online. Testing
on Trats2 or other devices would be appreciated.
---
 MAINTAINERS |   8 +
 drivers/char/hw_random/Kconfig  |  14 --
 drivers/char/hw_random/Makefile |   1 -
 drivers/char/hw_random/exynos-rng.c | 231 -
 drivers/crypto/Kconfig  |  15 ++
 drivers/crypto/Makefile |   1 +
 drivers/crypto/exynos-rng.c | 392 
 7 files changed, 416 insertions(+), 246 deletions(-)
 delete mode 100644 drivers/char/hw_random/exynos-rng.c
 create mode 100644 drivers/crypto/exynos-rng.c

diff --git a/MAINTAINERS b/MAINTAINERS
index affecc6d59f4..371fda859d43 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10977,6 +10977,14 @@ L: alsa-de...@alsa-project.org (moderated for 
non-subscribers)
 S: Supported
 F: sound/soc/samsung/
 
+SAMSUNG EXYNOS PSEUDO RANDOM NUMBER GENERATOR (RNG) DRIVER
+M: Krzysztof Kozlowski 
+L: linux-crypto@vger.kernel.org
+L: linux-samsung-...@vger.kernel.org
+S: Maintained
+F: drivers/crypto/exynos-rng.c
+F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
+
 SAMSUNG FRAMEBUFFER DRIVER
 M: Jingoo Han 
 L: linux-fb...@vger.kernel.org
diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 0cafe08919c9..bdae802e7154 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -294,20 +294,6 @@ config HW_RANDOM_POWERNV
 
  If unsure, say Y.
 
-config HW_RANDOM_EXYNOS
-   tristate "EXYNOS HW random number generator support"
-   depends on ARCH_EXYNOS || COMPILE_TEST
-   depends on HAS_IOMEM
-   default HW_RANDOM
-   ---help---
- This driver provides kernel-side support for the Random Number
- Generator hardware found on EXYNOS SOCs.
-
- To compile this driver as a module, choose M here: the
- module will be called exynos-rng.
-
- If unsure, say Y.
-
 config HW_RANDOM_TPM
tristate "TPM HW Random Number Generator support"
depends on TCG_TPM
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index 5f52b1e4e7be..6f1eecc2045c 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -24,7 +24,6 @@ obj-$(CONFIG_HW_RANDOM_OCTEON) += octeon-rng.o
 obj-$(CONFIG_HW_RANDOM_NOMADIK) += nomadik-rng.o
 obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-rng.o
 obj-$(CONFIG_HW_RANDOM_POWERNV) += powernv-rng.o
-obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-rng.o
 obj-$(CONFIG_HW_RANDOM_HISI)   += hisi-rng.o
 obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o
 obj-$(CONFIG_HW_RANDOM_BCM2835) += bcm2835-rng.o
diff --git a/drivers/char/hw_random/exynos-rng.c 
b/drivers/char/hw_random/exynos-rng.c
deleted file mode 100644
index 23d358553b21..
--- a/drivers/char/hw_random/exynos-rng.c
+++ /dev/null
@@ -1,231 +0,0 @@
-/*
- * exynos-rng.c - Random Number Generator driver for the exynos
- *
- * Copyright (C) 2012 Samsung Electronics
- * Jonghwa Lee 
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation;
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307