Re: [PATCH] crypto: sun4i-ss: support the Security System PRNG

[Herbert Xu]

On Wed, Jun 21, 2017 at 08:48:55AM +0200, Maxime Ripard wrote:
> On Tue, Jun 20, 2017 at 01:45:36PM +0200, Corentin Labbe wrote:
> > On Tue, Jun 20, 2017 at 11:59:47AM +0200, Maxime Ripard wrote:
> > > Hi,
> > > 
> > > On Tue, Jun 20, 2017 at 10:58:19AM +0200, Corentin Labbe wrote:
> > > > The Security System have a PRNG, this patch add support for it via
> > > > crypto_rng.
> > > 
> > > This might be a dumb question, but is the CRYPTO_RNG code really
> > > supposed to be used with PRNG?
> > > 
> > 
> > Yes, see recently added drivers/crypto/exynos-rng.c
> 
> It's still not really clear from the commit log (if you're talking
> about c46ea13f55b6) why and if using the RNG code for a PRNG is a good
> idea.

The hwrng interface is meant for true hardware RNGs.  The crypto API
rng interface is primarily intended for PRNGs.

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

Re: [PATCH] crypto: sun4i-ss: support the Security System PRNG

[Maxime Ripard]

On Tue, Jun 20, 2017 at 01:45:36PM +0200, Corentin Labbe wrote:
> On Tue, Jun 20, 2017 at 11:59:47AM +0200, Maxime Ripard wrote:
> > Hi,
> > 
> > On Tue, Jun 20, 2017 at 10:58:19AM +0200, Corentin Labbe wrote:
> > > The Security System have a PRNG, this patch add support for it via
> > > crypto_rng.
> > 
> > This might be a dumb question, but is the CRYPTO_RNG code really
> > supposed to be used with PRNG?
> > 
> 
> Yes, see recently added drivers/crypto/exynos-rng.c

It's still not really clear from the commit log (if you're talking
about c46ea13f55b6) why and if using the RNG code for a PRNG is a good
idea.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH] crypto: sun4i-ss: support the Security System PRNG

[Corentin Labbe]

On Tue, Jun 20, 2017 at 11:59:47AM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Tue, Jun 20, 2017 at 10:58:19AM +0200, Corentin Labbe wrote:
> > The Security System have a PRNG, this patch add support for it via
> > crypto_rng.
> 
> This might be a dumb question, but is the CRYPTO_RNG code really
> supposed to be used with PRNG?
> 

Yes, see recently added drivers/crypto/exynos-rng.c

[...]
> > --- a/drivers/crypto/sunxi-ss/sun4i-ss.h
> > +++ b/drivers/crypto/sunxi-ss/sun4i-ss.h
> > @@ -32,6 +32,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #define SS_CTL0x00
> >  #define SS_KEY0   0x04
> > @@ -127,6 +128,9 @@
> >  #define SS_RXFIFO_EMP_INT_ENABLE   (1 << 2)
> >  #define SS_TXFIFO_AVA_INT_ENABLE   (1 << 0)
> >  
> > +#define SS_SEED_LEN (192 / 8)
> > +#define SS_DATA_LEN (160 / 8)
> > +
> >  struct sun4i_ss_ctx {
> > void __iomem *base;
> > int irq;
> > @@ -136,6 +140,7 @@ struct sun4i_ss_ctx {
> > struct device *dev;
> > struct resource *res;
> > spinlock_t slock; /* control the use of the device */
> > +   u32 seed[SS_SEED_LEN / 4];
> 
> Shouldn't you define SS_SEED_LEN in bits, and then use either
> BITS_PER_BYTE and BITS_PER_LONG so that it's obvious what you're doing
> ?
> 
> And you could also make that variable defined based on the option,
> otherwise you'll always allocate that array, even if you're not using
> it.

I will do that

Thanks

Re: [PATCH] crypto: sun4i-ss: support the Security System PRNG

[Maxime Ripard]

Hi,

On Tue, Jun 20, 2017 at 10:58:19AM +0200, Corentin Labbe wrote:
> The Security System have a PRNG, this patch add support for it via
> crypto_rng.

This might be a dumb question, but is the CRYPTO_RNG code really
supposed to be used with PRNG?

> Signed-off-by: Corentin Labbe 
> ---
>  drivers/crypto/Kconfig  |  8 +
>  drivers/crypto/sunxi-ss/Makefile|  1 +
>  drivers/crypto/sunxi-ss/sun4i-ss-core.c | 30 ++
>  drivers/crypto/sunxi-ss/sun4i-ss-prng.c | 56 
> +
>  drivers/crypto/sunxi-ss/sun4i-ss.h  |  9 ++
>  5 files changed, 104 insertions(+)
>  create mode 100644 drivers/crypto/sunxi-ss/sun4i-ss-prng.c
> 
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index ab82536d64e2..bde0b102eb70 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -618,6 +618,14 @@ config CRYPTO_DEV_SUN4I_SS
> To compile this driver as a module, choose M here: the module
> will be called sun4i-ss.
>  
> +config CRYPTO_DEV_SUN4I_SS_PRNG
> + bool "Support for Allwinner Security System PRNG"
> + depends on CRYPTO_DEV_SUN4I_SS
> + select CRYPTO_RNG
> + help
> +   Select this option if you to provides kernel-side support for
> +   the Pseudo-Random Number Generator found in the Security System.
> +
>  config CRYPTO_DEV_ROCKCHIP
>   tristate "Rockchip's Cryptographic Engine driver"
>   depends on OF && ARCH_ROCKCHIP
> diff --git a/drivers/crypto/sunxi-ss/Makefile 
> b/drivers/crypto/sunxi-ss/Makefile
> index 8f4c7a273141..ccb893219079 100644
> --- a/drivers/crypto/sunxi-ss/Makefile
> +++ b/drivers/crypto/sunxi-ss/Makefile
> @@ -1,2 +1,3 @@
>  obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sun4i-ss.o
>  sun4i-ss-y += sun4i-ss-core.o sun4i-ss-hash.o sun4i-ss-cipher.o
> +sun4i-ss-$(CONFIG_CRYPTO_DEV_SUN4I_SS_PRNG) += sun4i-ss-prng.o
> diff --git a/drivers/crypto/sunxi-ss/sun4i-ss-core.c 
> b/drivers/crypto/sunxi-ss/sun4i-ss-core.c
> index 02ad8256e900..d6bb2991c000 100644
> --- a/drivers/crypto/sunxi-ss/sun4i-ss-core.c
> +++ b/drivers/crypto/sunxi-ss/sun4i-ss-core.c
> @@ -213,6 +213,23 @@ static struct sun4i_ss_alg_template ss_algs[] = {
>   }
>   }
>  },
> +#ifdef CONFIG_CRYPTO_DEV_SUN4I_SS_PRNG
> +{
> + .type = CRYPTO_ALG_TYPE_RNG,
> + .alg.rng = {
> + .base = {
> + .cra_name   = "stdrng",
> + .cra_driver_name= "sun4i_ss_rng",
> + .cra_priority   = 300,
> + .cra_ctxsize= 0,
> + .cra_module = THIS_MODULE,
> + },
> + .generate   = sun4i_ss_prng_generate,
> + .seed   = sun4i_ss_prng_seed,
> + .seedsize   = SS_SEED_LEN,
> + }
> +},
> +#endif
>  };
>  
>  static int sun4i_ss_probe(struct platform_device *pdev)
> @@ -355,6 +372,13 @@ static int sun4i_ss_probe(struct platform_device *pdev)
>   goto error_alg;
>   }
>   break;
> + case CRYPTO_ALG_TYPE_RNG:
> + err = crypto_register_rng(_algs[i].alg.rng);
> + if (err) {
> + dev_err(ss->dev, "Fail to register %s\n",
> + ss_algs[i].alg.rng.base.cra_name);
> + }
> + break;
>   }
>   }
>   platform_set_drvdata(pdev, ss);
> @@ -369,6 +393,9 @@ static int sun4i_ss_probe(struct platform_device *pdev)
>   case CRYPTO_ALG_TYPE_AHASH:
>   crypto_unregister_ahash(_algs[i].alg.hash);
>   break;
> + case CRYPTO_ALG_TYPE_RNG:
> + crypto_unregister_rng(_algs[i].alg.rng);
> + break;
>   }
>   }
>   if (ss->reset)
> @@ -393,6 +420,9 @@ static int sun4i_ss_remove(struct platform_device *pdev)
>   case CRYPTO_ALG_TYPE_AHASH:
>   crypto_unregister_ahash(_algs[i].alg.hash);
>   break;
> + case CRYPTO_ALG_TYPE_RNG:
> + crypto_unregister_rng(_algs[i].alg.rng);
> + break;
>   }
>   }
>  
> diff --git a/drivers/crypto/sunxi-ss/sun4i-ss-prng.c 
> b/drivers/crypto/sunxi-ss/sun4i-ss-prng.c
> new file mode 100644
> index ..3941587def6b
> --- /dev/null
> +++ b/drivers/crypto/sunxi-ss/sun4i-ss-prng.c
> @@ -0,0 +1,56 @@
> +#include "sun4i-ss.h"
> +
> +int sun4i_ss_prng_seed(struct crypto_rng *tfm, const u8 *seed,
> +unsigned int slen)
> +{
> + struct sun4i_ss_alg_template *algt;
> + struct rng_alg *alg = crypto_rng_alg(tfm);
> +
> + algt = container_of(alg, struct sun4i_ss_alg_template, alg.rng);
> + memcpy(algt->ss->seed, seed, slen);
> +

Re: [PATCH] crypto: sun4i-ss: support the Security System PRNG

[Corentin Labbe]

On Thu, Nov 17, 2016 at 08:07:09PM -0500, Sandy Harris wrote:
> Add Ted T'so to cc list. Shouldn't he be included on anything affecting
> the random(4) driver?
> 

Blindy used get_maintainer.pl, and since the file is in crypto, hw_random 
people were not set.
Note that get_maintainer.pl on drivers/char/hw_random/, does not give his 
address also.
My V2 patch will have them in CC/TO.

> On Tue, Oct 18, 2016 at 8:34 AM, Corentin Labbe
>  wrote:
> 
> > From: LABBE Corentin 
> >
> > The Security System have a PRNG.
> > This patch add support for it as an hwrng.
> 
> Which is it? A PRNG & a HW RNG are quite different things.
> It would, in general, be a fairly serious error to treat a PRNG
> as a HWRNG.
> 
> If it is just a prng (which it appears to be from a quick look
> at your code) then it is not clear it is useful since the
> random(4) driver already has two PRNGs. It might be
> but I cannot tell.

For me hwrng is a way to give user space an another way to get "random" data 
via /dev/hwrng.
The only impact of hwrng with random is that just after init some data of hwrng 
is used for having more entropy.

Grepping prng in drivers/char/hw_random/ and drivers/crypto show me some other 
PRNG used with hwrng.

Regards
Corentin Labbe
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: sun4i-ss: support the Security System PRNG

[Sandy Harris]

Add Ted T'so to cc list. Shouldn't he be included on anything affecting
the random(4) driver?

On Tue, Oct 18, 2016 at 8:34 AM, Corentin Labbe
 wrote:

> From: LABBE Corentin 
>
> The Security System have a PRNG.
> This patch add support for it as an hwrng.

Which is it? A PRNG & a HW RNG are quite different things.
It would, in general, be a fairly serious error to treat a PRNG
as a HWRNG.

If it is just a prng (which it appears to be from a quick look
at your code) then it is not clear it is useful since the
random(4) driver already has two PRNGs. It might be
but I cannot tell.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: sun4i-ss: support the Security System PRNG

[Stephan Mueller]

Am Donnerstag, 17. November 2016, 09:07:48 CET schrieb Corentin Labbe:

Hi Corentin,
> 
> Seed again, or just do not seed (and so return -EAGAIN for read() function)
> until ready_callback ?

This is your choice. But for the start sequence, you should not simply rely on 
get_random_bytes.

For the DRBG in crypto/drbg.c we seed with get_random_bytes and the Jitter RNG 
in case the input_pool is not fully seeded. The reseed trigger is reduced to 
50 DRBG requests, i.e. after 50 requests, the DRBG again reseeds from 
get_random_bytes / Jitter RNG. This is continued until the input_pool has been 
sufficiently seeded (i.e. the registered callback is triggered). At that 
point, another get_random_bytes call is made, the Jitter RNG is deactivated 
and the reseed threshold is set to the common value.

Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: sun4i-ss: support the Security System PRNG

[Corentin Labbe]

On Tue, Oct 18, 2016 at 04:24:22PM +0200, Stephan Mueller wrote:
> Am Dienstag, 18. Oktober 2016, 14:34:27 CEST schrieb Corentin Labbe:
> 
> Hi Corentin,
> 
> > diff --git a/drivers/crypto/sunxi-ss/sun4i-ss-hwrng.c
> > b/drivers/crypto/sunxi-ss/sun4i-ss-hwrng.c new file mode 100644
> > index 000..95fadb7
> > --- /dev/null
> > +++ b/drivers/crypto/sunxi-ss/sun4i-ss-hwrng.c
> > @@ -0,0 +1,70 @@
> > +#include "sun4i-ss.h"
> > +
> > +static int sun4i_ss_hwrng_init(struct hwrng *hwrng)
> > +{
> > +   struct sun4i_ss_ctx *ss;
> > +
> > +   ss = container_of(hwrng, struct sun4i_ss_ctx, hwrng);
> > +   get_random_bytes(ss->seed, SS_SEED_LEN);
> 
> Is it wise to call get_random_bytes once in the init function and never 
> thereafter?
> 
> This init function may be called during boot time of the kernel at which the 
> input_pool may not yet have received sufficient amounts of entropy.
> 
> What about registering a callback with add_random_ready_callback and seed 
> again when sufficient entropy was collected?
> 

Seed again, or just do not seed (and so return -EAGAIN for read() function) 
until ready_callback ?

Thanks

Corentin Labbe
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: sun4i-ss: support the Security System PRNG

[Corentin Labbe]

On Tue, Oct 18, 2016 at 09:39:17PM +0530, PrasannaKumar Muralidharan wrote:
> Hi Corentin,
> 
> I have a few minor comments.
> 
> On 18 October 2016 at 18:04, Corentin Labbe  wrote:
> > From: LABBE Corentin 
> >
> > The Security System have a PRNG.
> > This patch add support for it as an hwrng.
> >
> > Signed-off-by: Corentin Labbe 
> > ---
> >  drivers/crypto/Kconfig   |  8 
> >  drivers/crypto/sunxi-ss/Makefile |  1 +
> >  drivers/crypto/sunxi-ss/sun4i-ss-core.c  | 14 +++
> >  drivers/crypto/sunxi-ss/sun4i-ss-hwrng.c | 70 
> > 
> >  drivers/crypto/sunxi-ss/sun4i-ss.h   |  8 
> >  5 files changed, 101 insertions(+)
> >  create mode 100644 drivers/crypto/sunxi-ss/sun4i-ss-hwrng.c
> >
> > diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> > index 4d2b81f..38f7aca 100644
> > --- a/drivers/crypto/Kconfig
> > +++ b/drivers/crypto/Kconfig
> > @@ -538,6 +538,14 @@ config CRYPTO_DEV_SUN4I_SS
> >   To compile this driver as a module, choose M here: the module
> >   will be called sun4i-ss.
> >
> > +config CRYPTO_DEV_SUN4I_SS_PRNG
> > +   bool "Support for Allwinner Security System PRNG"
> > +   depends on CRYPTO_DEV_SUN4I_SS
> > +   select HW_RANDOM
> > +   help
> > + This driver provides kernel-side support for the Pseudo-Random
> > + Number Generator found in the Security System.
> > +
> >  config CRYPTO_DEV_ROCKCHIP
> > tristate "Rockchip's Cryptographic Engine driver"
> > depends on OF && ARCH_ROCKCHIP
> > diff --git a/drivers/crypto/sunxi-ss/Makefile 
> > b/drivers/crypto/sunxi-ss/Makefile
> > index 8f4c7a2..ca049ee 100644
> > --- a/drivers/crypto/sunxi-ss/Makefile
> > +++ b/drivers/crypto/sunxi-ss/Makefile
> > @@ -1,2 +1,3 @@
> >  obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sun4i-ss.o
> >  sun4i-ss-y += sun4i-ss-core.o sun4i-ss-hash.o sun4i-ss-cipher.o
> > +sun4i-ss-$(CONFIG_CRYPTO_DEV_SUN4I_SS_PRNG) += sun4i-ss-hwrng.o
> > diff --git a/drivers/crypto/sunxi-ss/sun4i-ss-core.c 
> > b/drivers/crypto/sunxi-ss/sun4i-ss-core.c
> > index 3ac6c6c..fa739de 100644
> > --- a/drivers/crypto/sunxi-ss/sun4i-ss-core.c
> > +++ b/drivers/crypto/sunxi-ss/sun4i-ss-core.c
> > @@ -359,6 +359,16 @@ static int sun4i_ss_probe(struct platform_device *pdev)
> > }
> > }
> > platform_set_drvdata(pdev, ss);
> > +
> > +#ifdef CONFIG_CRYPTO_DEV_SUN4I_SS_PRNG
> > +   /* Voluntarily made the PRNG optional */
> > +   err = sun4i_ss_hwrng_register(>hwrng);
> > +   if (!err)
> > +   dev_info(ss->dev, "sun4i-ss PRNG loaded");
> > +   else
> > +   dev_err(ss->dev, "sun4i-ss PRNG failed");
> > +#endif
> > +
> > return 0;
> >  error_alg:
> > i--;
> > @@ -386,6 +396,10 @@ static int sun4i_ss_remove(struct platform_device 
> > *pdev)
> > int i;
> > struct sun4i_ss_ctx *ss = platform_get_drvdata(pdev);
> >
> > +#ifdef CONFIG_CRYPTO_DEV_SUN4I_SS_PRNG
> > +   sun4i_ss_hwrng_remove(>hwrng);
> > +#endif
> > +
> > for (i = 0; i < ARRAY_SIZE(ss_algs); i++) {
> > switch (ss_algs[i].type) {
> > case CRYPTO_ALG_TYPE_ABLKCIPHER:
> > diff --git a/drivers/crypto/sunxi-ss/sun4i-ss-hwrng.c 
> > b/drivers/crypto/sunxi-ss/sun4i-ss-hwrng.c
> > new file mode 100644
> > index 000..95fadb7
> > --- /dev/null
> > +++ b/drivers/crypto/sunxi-ss/sun4i-ss-hwrng.c
> > @@ -0,0 +1,70 @@
> > +#include "sun4i-ss.h"
> > +
> > +static int sun4i_ss_hwrng_init(struct hwrng *hwrng)
> > +{
> > +   struct sun4i_ss_ctx *ss;
> > +
> > +   ss = container_of(hwrng, struct sun4i_ss_ctx, hwrng);
> > +   get_random_bytes(ss->seed, SS_SEED_LEN);
> > +
> > +   return 0;
> > +}
> > +
> > +static int sun4i_ss_hwrng_read(struct hwrng *hwrng, void *buf,
> > +  size_t max, bool wait)
> > +{
> > +   int i;
> > +   u32 v;
> > +   u32 *data = buf;
> > +   const u32 mode = SS_OP_PRNG | SS_PRNG_CONTINUE | SS_ENABLED;
> > +   size_t len;
> > +   struct sun4i_ss_ctx *ss;
> > +
> > +   ss = container_of(hwrng, struct sun4i_ss_ctx, hwrng);
> > +   len = min_t(size_t, SS_DATA_LEN, max);
> > +
> > +   spin_lock_bh(>slock);
> 
> Is spin_lock_bh really required here? I could see it is being used in
> sun4i-ss-hash.c but could not find any comment/info about the need.
> 

No for sun4i-ss-hwrng it seems not required and work perfecly without _bh

> > +   writel(mode, ss->base + SS_CTL);
> > +
> > +   /* write the seed */
> > +   for (i = 0; i < SS_SEED_LEN / 4; i++)
> > +   writel(ss->seed[i], ss->base + SS_KEY0 + i * 4);
> > +   writel(mode | SS_PRNG_START, ss->base + SS_CTL);
> > +
> > +   /* Read the random data */
> > +   readsl(ss->base + SS_TXFIFO, data, len / 4);
> > +
> > +   if (len % 4 > 0) {
> > +   v =