Re: [PATCH v1] crypto: caam - set hwrng quality level

[Herbert Xu]

On Thu, Aug 03, 2017 at 07:48:51AM +, Horia Geantă wrote:
>
> Right now it's configured as a DRBG.
> If I read correctly, it doesn't matter it's using the internal TRNG for
> (automated) seeding, it still shouldn't use hwrng.
> This means it's broken since the very beginning:
> e24f7c9e87d4 crypto: caam - hwrng support

Right.  If it can be switched over to a TRNG then we should do that.
Otherwise it should be converted over to use the crypto rng API.

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

Re: [PATCH v1] crypto: caam - set hwrng quality level

[Oleksij Rempel]

On 03.08.2017 09:48, Horia Geantă wrote:

On 8/3/2017 6:17 AM, Herbert Xu wrote:

On Wed, Aug 02, 2017 at 02:03:14PM +, Horia Geantă wrote:


Take CAAM's engine HWRNG: it can work both as a TRNG and as a
TRNG-seeded DRBG (that's how it's currently configured).
IIUC, both setups are fit as source for the entropy pool.


So which is it? If it's a DRBG then it should not be exposed through
the hwrng interface.  Only TRNG should go through hwrng.  DRBGs
can use the crypto rng API.


Right now it's configured as a DRBG.
If I read correctly, it doesn't matter it's using the internal TRNG for
(automated) seeding, it still shouldn't use hwrng.
This means it's broken since the very beginning:
e24f7c9e87d4 crypto: caam - hwrng support


Hmmm..
- what is the security risk of this issue? For example system which use 
/dev/hwrng directly?

- And who will fix it?

Re: [PATCH v1] crypto: caam - set hwrng quality level

[Horia Geantă]

On 8/3/2017 6:17 AM, Herbert Xu wrote:
> On Wed, Aug 02, 2017 at 02:03:14PM +, Horia Geantă wrote:
>>
>> Take CAAM's engine HWRNG: it can work both as a TRNG and as a
>> TRNG-seeded DRBG (that's how it's currently configured).
>> IIUC, both setups are fit as source for the entropy pool.
> 
> So which is it? If it's a DRBG then it should not be exposed through
> the hwrng interface.  Only TRNG should go through hwrng.  DRBGs
> can use the crypto rng API.

Right now it's configured as a DRBG.
If I read correctly, it doesn't matter it's using the internal TRNG for
(automated) seeding, it still shouldn't use hwrng.
This means it's broken since the very beginning:
e24f7c9e87d4 crypto: caam - hwrng support

Thanks,
Horia

Re: [PATCH v1] crypto: caam - set hwrng quality level

[Herbert Xu]

On Wed, Aug 02, 2017 at 02:03:14PM +, Horia Geantă wrote:
>
> Take CAAM's engine HWRNG: it can work both as a TRNG and as a
> TRNG-seeded DRBG (that's how it's currently configured).
> IIUC, both setups are fit as source for the entropy pool.

So which is it? If it's a DRBG then it should not be exposed through
the hwrng interface.  Only TRNG should go through hwrng.  DRBGs
can use the crypto rng API.

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

Re: [PATCH v1] crypto: caam - set hwrng quality level

[Horia Geantă]

On 7/20/2017 4:08 PM, Harald Freudenberger wrote:
> On 07/19/2017 08:13 PM, Oleksij Rempel wrote:
>> On Wed, Jul 19, 2017 at 04:53:21PM +, Horia Geantă wrote:
>>> On 7/19/2017 7:32 PM, Oleksij Rempel wrote:
 On Wed, Jul 19, 2017 at 12:49:47PM +, Horia Geantă wrote:
> On 7/19/2017 10:45 AM, Oleksij Rempel wrote:
>> According documentation, it is NIST certified TRNG.
>> So, set high quality to let the HWRNG framework automatically use it.
>>
>> Signed-off-by: Oleksij Rempel 
>> ---
>>  drivers/crypto/caam/caamrng.c | 6 ++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/crypto/caam/caamrng.c 
>> b/drivers/crypto/caam/caamrng.c
>> index 41398da3edf4..684c0bc88dfd 100644
>> --- a/drivers/crypto/caam/caamrng.c
>> +++ b/drivers/crypto/caam/caamrng.c
>> @@ -292,10 +292,16 @@ static int caam_init_rng(struct caam_rng_ctx *ctx, 
>> struct device *jrdev)
>>  return 0;
>>  }
>>  
>> +/*
>> + * hwrng register struct
>> + * The trng is suppost to have 100% entropy, and thus
>> + * we register with a very high quality value.
>> + */
>>  static struct hwrng caam_rng = {
>>  .name   = "rng-caam",
>>  .cleanup= caam_cleanup,
>>  .read   = caam_read,
>> +.quality= 999,
> Why not 1024, i.e. where is 999 coming from?
 It comes from s390-trng.c driver.
 Should I use 1024 instead?

>>> AFAICT the range for quality is from 0 to 1024 (no entropy -> perfect
>>> entropy).
>>>
>>> 1024 should be used since I'd expect a HW TRNG to provide perfect
>>> entropy unless otherwise stated.
>> I assume 1024 can be given only on verified HW with accessible verilog
>> files and compared with resulting chip :)
>> May be this would be a good example https://www.sifive.com/
>>
> Well, the header file says:
> ...
> /**
>  * struct hwrng - Hardware Random Number Generator driver
>  * @name:   Unique RNG name.
>  * @init:   Initialization callback (can be NULL).
>  * @cleanup:Cleanup callback (can be NULL).
>  * @data_present:   Callback to determine if data is available
>  *  on the RNG. If NULL, it is assumed that
>  *  there is always data available.  *OBSOLETE*
>  * @data_read:  Read data from the RNG device.
>  *  Returns the number of lower random bytes in "data".
>  *  Must not be NULL.*OBSOLETE*
>  * @read:   New API. drivers can fill up to max bytes of data
>  *  into the buffer. The buffer is aligned for any type
>  *  and max is a multiple of 4 and >= 32 bytes.
>  * @priv:   Private data, for use by the RNG driver.
>  * @quality:Estimation of true entropy in RNG's bitstream
>  *  (per mill).
>  */
> ...
> quality = estimation of true entropy per mill.
"per mill as in https://en.wikipedia.org/wiki/Mill_(currency) ?
I consider it rather unfortunate, as already noticed here:
https://lkml.org/lkml/2014/3/27/210

And isn't this inaccurate, since the (de)rating factor is quality/1024,
not quality/1000?

> I understand this as quality=1000 meaning 100% entropy.
> However, the core code currently does not really check this value.
> When more than one hwrng sources do register, simple the one with
> the higher quality value wins :-) The value is not even checked
> to be in a given range.
> 
> I searched through some device drivers which do register at
> the hwrng and it looks like most of the drivers do not even
> set this value. My feeling is, you should use 999 when your

Maybe this is because it's not clear how to determine quality's value?

Take CAAM's engine HWRNG: it can work both as a TRNG and as a
TRNG-seeded DRBG (that's how it's currently configured).
IIUC, both setups are fit as source for the entropy pool.

Do I have to set quality value comparing the two cases?
(It's a bit like comparing the quality of entropy offered by RDSEED vs.
RDRAND.)
Meaning: give full credit - maximum quality - for the TRNG setup, and
provide a lower value for quality in the case of TRNG-seeded DRBG?

> hardware provides 'perfect' random. So there is a chance for
> an even 'more perfect' hardware coming up later to overrule
> your 'perfect' hardware.

I am not sure why the hwrng with the best quality wins, instead of using
all available resources, as suggested here:
https://lkml.org/lkml/2014/3/27/210

Thanks,
Horia

Re: [PATCH v1] crypto: caam - set hwrng quality level

[Harald Freudenberger]

On 07/19/2017 08:13 PM, Oleksij Rempel wrote:
> On Wed, Jul 19, 2017 at 04:53:21PM +, Horia Geantă wrote:
>> On 7/19/2017 7:32 PM, Oleksij Rempel wrote:
>>> On Wed, Jul 19, 2017 at 12:49:47PM +, Horia Geantă wrote:
 On 7/19/2017 10:45 AM, Oleksij Rempel wrote:
> According documentation, it is NIST certified TRNG.
> So, set high quality to let the HWRNG framework automatically use it.
>
> Signed-off-by: Oleksij Rempel 
> ---
>  drivers/crypto/caam/caamrng.c | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/crypto/caam/caamrng.c b/drivers/crypto/caam/caamrng.c
> index 41398da3edf4..684c0bc88dfd 100644
> --- a/drivers/crypto/caam/caamrng.c
> +++ b/drivers/crypto/caam/caamrng.c
> @@ -292,10 +292,16 @@ static int caam_init_rng(struct caam_rng_ctx *ctx, 
> struct device *jrdev)
>   return 0;
>  }
>  
> +/*
> + * hwrng register struct
> + * The trng is suppost to have 100% entropy, and thus
> + * we register with a very high quality value.
> + */
>  static struct hwrng caam_rng = {
>   .name   = "rng-caam",
>   .cleanup= caam_cleanup,
>   .read   = caam_read,
> + .quality= 999,
 Why not 1024, i.e. where is 999 coming from?
>>> It comes from s390-trng.c driver.
>>> Should I use 1024 instead?
>>>
>> AFAICT the range for quality is from 0 to 1024 (no entropy -> perfect
>> entropy).
>>
>> 1024 should be used since I'd expect a HW TRNG to provide perfect
>> entropy unless otherwise stated.
> I assume 1024 can be given only on verified HW with accessible verilog
> files and compared with resulting chip :)
> May be this would be a good example https://www.sifive.com/
>
Well, the header file says:
...
/**
 * struct hwrng - Hardware Random Number Generator driver
 * @name:   Unique RNG name.
 * @init:   Initialization callback (can be NULL).
 * @cleanup:Cleanup callback (can be NULL).
 * @data_present:   Callback to determine if data is available
 *  on the RNG. If NULL, it is assumed that
 *  there is always data available.  *OBSOLETE*
 * @data_read:  Read data from the RNG device.
 *  Returns the number of lower random bytes in "data".
 *  Must not be NULL.*OBSOLETE*
 * @read:   New API. drivers can fill up to max bytes of data
 *  into the buffer. The buffer is aligned for any type
 *  and max is a multiple of 4 and >= 32 bytes.
 * @priv:   Private data, for use by the RNG driver.
 * @quality:Estimation of true entropy in RNG's bitstream
 *  (per mill).
 */
...
quality = estimation of true entropy per mill.
I understand this as quality=1000 meaning 100% entropy.
However, the core code currently does not really check this value.
When more than one hwrng sources do register, simple the one with
the higher quality value wins :-) The value is not even checked
to be in a given range.

I searched through some device drivers which do register at
the hwrng and it looks like most of the drivers do not even
set this value. My feeling is, you should use 999 when your
hardware provides 'perfect' random. So there is a chance for
an even 'more perfect' hardware coming up later to overrule
your 'perfect' hardware.

regards
Harald Freudenberger

Re: [PATCH v1] crypto: caam - set hwrng quality level

[Oleksij Rempel]

On Wed, Jul 19, 2017 at 04:53:21PM +, Horia Geantă wrote:
> On 7/19/2017 7:32 PM, Oleksij Rempel wrote:
> > On Wed, Jul 19, 2017 at 12:49:47PM +, Horia Geantă wrote:
> >> On 7/19/2017 10:45 AM, Oleksij Rempel wrote:
> >>> According documentation, it is NIST certified TRNG.
> >>> So, set high quality to let the HWRNG framework automatically use it.
> >>>
> >>> Signed-off-by: Oleksij Rempel 
> >>> ---
> >>>  drivers/crypto/caam/caamrng.c | 6 ++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/drivers/crypto/caam/caamrng.c b/drivers/crypto/caam/caamrng.c
> >>> index 41398da3edf4..684c0bc88dfd 100644
> >>> --- a/drivers/crypto/caam/caamrng.c
> >>> +++ b/drivers/crypto/caam/caamrng.c
> >>> @@ -292,10 +292,16 @@ static int caam_init_rng(struct caam_rng_ctx *ctx, 
> >>> struct device *jrdev)
> >>>   return 0;
> >>>  }
> >>>  
> >>> +/*
> >>> + * hwrng register struct
> >>> + * The trng is suppost to have 100% entropy, and thus
> >>> + * we register with a very high quality value.
> >>> + */
> >>>  static struct hwrng caam_rng = {
> >>>   .name   = "rng-caam",
> >>>   .cleanup= caam_cleanup,
> >>>   .read   = caam_read,
> >>> + .quality= 999,
> >>
> >> Why not 1024, i.e. where is 999 coming from?
> > 
> > It comes from s390-trng.c driver.
> > Should I use 1024 instead?
> > 
> AFAICT the range for quality is from 0 to 1024 (no entropy -> perfect
> entropy).
> 
> 1024 should be used since I'd expect a HW TRNG to provide perfect
> entropy unless otherwise stated.

I assume 1024 can be given only on verified HW with accessible verilog
files and compared with resulting chip :)
May be this would be a good example https://www.sifive.com/

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |

Re: [PATCH v1] crypto: caam - set hwrng quality level

[Horia Geantă]

On 7/19/2017 7:32 PM, Oleksij Rempel wrote:
> On Wed, Jul 19, 2017 at 12:49:47PM +, Horia Geantă wrote:
>> On 7/19/2017 10:45 AM, Oleksij Rempel wrote:
>>> According documentation, it is NIST certified TRNG.
>>> So, set high quality to let the HWRNG framework automatically use it.
>>>
>>> Signed-off-by: Oleksij Rempel 
>>> ---
>>>  drivers/crypto/caam/caamrng.c | 6 ++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/crypto/caam/caamrng.c b/drivers/crypto/caam/caamrng.c
>>> index 41398da3edf4..684c0bc88dfd 100644
>>> --- a/drivers/crypto/caam/caamrng.c
>>> +++ b/drivers/crypto/caam/caamrng.c
>>> @@ -292,10 +292,16 @@ static int caam_init_rng(struct caam_rng_ctx *ctx, 
>>> struct device *jrdev)
>>> return 0;
>>>  }
>>>  
>>> +/*
>>> + * hwrng register struct
>>> + * The trng is suppost to have 100% entropy, and thus
>>> + * we register with a very high quality value.
>>> + */
>>>  static struct hwrng caam_rng = {
>>> .name   = "rng-caam",
>>> .cleanup= caam_cleanup,
>>> .read   = caam_read,
>>> +   .quality= 999,
>>
>> Why not 1024, i.e. where is 999 coming from?
> 
> It comes from s390-trng.c driver.
> Should I use 1024 instead?
> 
AFAICT the range for quality is from 0 to 1024 (no entropy -> perfect
entropy).

1024 should be used since I'd expect a HW TRNG to provide perfect
entropy unless otherwise stated.

[Cc-ing Harald and Martin for awareness wrt. s390/trng.]

Thanks,
Horia

Re: [PATCH v1] crypto: caam - set hwrng quality level

[Oleksij Rempel]

On Wed, Jul 19, 2017 at 12:49:47PM +, Horia Geantă wrote:
> On 7/19/2017 10:45 AM, Oleksij Rempel wrote:
> > According documentation, it is NIST certified TRNG.
> > So, set high quality to let the HWRNG framework automatically use it.
> > 
> > Signed-off-by: Oleksij Rempel 
> > ---
> >  drivers/crypto/caam/caamrng.c | 6 ++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/crypto/caam/caamrng.c b/drivers/crypto/caam/caamrng.c
> > index 41398da3edf4..684c0bc88dfd 100644
> > --- a/drivers/crypto/caam/caamrng.c
> > +++ b/drivers/crypto/caam/caamrng.c
> > @@ -292,10 +292,16 @@ static int caam_init_rng(struct caam_rng_ctx *ctx, 
> > struct device *jrdev)
> > return 0;
> >  }
> >  
> > +/*
> > + * hwrng register struct
> > + * The trng is suppost to have 100% entropy, and thus
> > + * we register with a very high quality value.
> > + */
> >  static struct hwrng caam_rng = {
> > .name   = "rng-caam",
> > .cleanup= caam_cleanup,
> > .read   = caam_read,
> > +   .quality= 999,
> 
> Why not 1024, i.e. where is 999 coming from?

It comes from s390-trng.c driver.
Should I use 1024 instead?

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |

Re: [PATCH v1] crypto: caam - set hwrng quality level

[Horia Geantă]

On 7/19/2017 10:45 AM, Oleksij Rempel wrote:
> According documentation, it is NIST certified TRNG.
> So, set high quality to let the HWRNG framework automatically use it.
> 
> Signed-off-by: Oleksij Rempel 
> ---
>  drivers/crypto/caam/caamrng.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/crypto/caam/caamrng.c b/drivers/crypto/caam/caamrng.c
> index 41398da3edf4..684c0bc88dfd 100644
> --- a/drivers/crypto/caam/caamrng.c
> +++ b/drivers/crypto/caam/caamrng.c
> @@ -292,10 +292,16 @@ static int caam_init_rng(struct caam_rng_ctx *ctx, 
> struct device *jrdev)
>   return 0;
>  }
>  
> +/*
> + * hwrng register struct
> + * The trng is suppost to have 100% entropy, and thus
> + * we register with a very high quality value.
> + */
>  static struct hwrng caam_rng = {
>   .name   = "rng-caam",
>   .cleanup= caam_cleanup,
>   .read   = caam_read,
> + .quality= 999,

Why not 1024, i.e. where is 999 coming from?

Thanks,
Horia

[PATCH v1] crypto: caam - set hwrng quality level

[Oleksij Rempel]

According documentation, it is NIST certified TRNG.
So, set high quality to let the HWRNG framework automatically use it.

Signed-off-by: Oleksij Rempel 
---
 drivers/crypto/caam/caamrng.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/crypto/caam/caamrng.c b/drivers/crypto/caam/caamrng.c
index 41398da3edf4..684c0bc88dfd 100644
--- a/drivers/crypto/caam/caamrng.c
+++ b/drivers/crypto/caam/caamrng.c
@@ -292,10 +292,16 @@ static int caam_init_rng(struct caam_rng_ctx *ctx, struct 
device *jrdev)
return 0;
 }
 
+/*
+ * hwrng register struct
+ * The trng is suppost to have 100% entropy, and thus
+ * we register with a very high quality value.
+ */
 static struct hwrng caam_rng = {
.name   = "rng-caam",
.cleanup= caam_cleanup,
.read   = caam_read,
+   .quality= 999,
 };
 
 static void __exit caam_rng_exit(void)
-- 
2.11.0