[PATCH 0/3]: hwrng: Add support for qcpm v2 hwrng

[PATCH 0/3]: hwrng: Add support for qcpm v2 hwrng

[Vinod Koul]

This series adds support for newer version of hwrng as found in
Qualcomm SoCs. To do that refactor the msm hwrng driver and add
support for v2 ops

Vinod Koul (3):
  hwrng: msm - Move hwrng to a table
  dt-bindings: rng: Add new compatible qcom,prng-v2
  hwrng: msm - Add support for prng v2

 .../devicetree/bindings/rng/qcom,prng.txt          |  3 +-
 drivers/char/hw_random/msm-rng.c                   | 36 +++++++++++++++-------
 2 files changed, 27 insertions(+), 12 deletions(-)

-- 
2.14.4

[PATCH 1/3] hwrng: msm - Move hwrng to a table

[Vinod Koul]

Future versions of msm-prng require bit differs callbacks so move
driver to use a table for hwrng.

No functional change.

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/char/hw_random/msm-rng.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/char/hw_random/msm-rng.c b/drivers/char/hw_random/msm-rng.c
index 841fee845ec9..7644474035e5 100644
--- a/drivers/char/hw_random/msm-rng.c
+++ b/drivers/char/hw_random/msm-rng.c
@@ -38,14 +38,12 @@
 struct msm_rng {
 	void __iomem *base;
 	struct clk *clk;
-	struct hwrng hwrng;
+	struct hwrng *hwrng;
 };
 
-#define to_msm_rng(p)	container_of(p, struct msm_rng, hwrng)
-
 static int msm_rng_enable(struct hwrng *hwrng, int enable)
 {
-	struct msm_rng *rng = to_msm_rng(hwrng);
+	struct msm_rng *rng = (struct msm_rng *)hwrng->priv;
 	u32 val;
 	int ret;
 
@@ -80,7 +78,7 @@ static int msm_rng_enable(struct hwrng *hwrng, int enable)
 
 static int msm_rng_read(struct hwrng *hwrng, void *data, size_t max, bool wait)
 {
-	struct msm_rng *rng = to_msm_rng(hwrng);
+	struct msm_rng *rng = (struct msm_rng *)hwrng->priv;
 	size_t currsize = 0;
 	u32 *retdata = data;
 	size_t maxsize;
@@ -127,6 +125,13 @@ static void msm_rng_cleanup(struct hwrng *hwrng)
 	msm_rng_enable(hwrng, 0);
 }
 
+static struct hwrng msm_rng = {
+	.name = KBUILD_MODNAME,
+	.init = msm_rng_init,
+	.cleanup = msm_rng_cleanup,
+	.read = msm_rng_read,
+};
+
 static int msm_rng_probe(struct platform_device *pdev)
 {
 	struct resource *res;
@@ -148,12 +153,10 @@ static int msm_rng_probe(struct platform_device *pdev)
 	if (IS_ERR(rng->clk))
 		return PTR_ERR(rng->clk);
 
-	rng->hwrng.name = KBUILD_MODNAME,
-	rng->hwrng.init = msm_rng_init,
-	rng->hwrng.cleanup = msm_rng_cleanup,
-	rng->hwrng.read = msm_rng_read,
+	rng->hwrng = &msm_rng;
 
-	ret = devm_hwrng_register(&pdev->dev, &rng->hwrng);
+	rng->hwrng->priv = (unsigned long)rng;
+	ret = devm_hwrng_register(&pdev->dev, rng->hwrng);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to register hwrng\n");
 		return ret;
-- 
2.14.4

Re: [PATCH 1/3] hwrng: msm - Move hwrng to a table

[Stephen Boyd]

Quoting Vinod Koul (2018-06-18 07:12:57)
> @@ -127,6 +125,13 @@ static void msm_rng_cleanup(struct hwrng *hwrng)
>         msm_rng_enable(hwrng, 0);
>  }
>  
> +static struct hwrng msm_rng = {
> +       .name = KBUILD_MODNAME,
> +       .init = msm_rng_init,
> +       .cleanup = msm_rng_cleanup,
> +       .read = msm_rng_read,
> +};
> +
>  static int msm_rng_probe(struct platform_device *pdev)
>  {
>         struct resource *res;
> @@ -148,12 +153,10 @@ static int msm_rng_probe(struct platform_device *pdev)
>         if (IS_ERR(rng->clk))
>                 return PTR_ERR(rng->clk);
>  
> -       rng->hwrng.name = KBUILD_MODNAME,
> -       rng->hwrng.init = msm_rng_init,
> -       rng->hwrng.cleanup = msm_rng_cleanup,

Wouldn't it be a lot easier to skip assigning the init and cleanup
functions on v2 devices with an if statement? It would be smaller size
wise too because then we don't have two structs for v1 and v2 hwrngs.
Plus the patch would be smaller overall because we would do everything
else pretty much the same besides the if condition in probe.

> -       rng->hwrng.read = msm_rng_read,
> +       rng->hwrng = &msm_rng;
>  
> -       ret = devm_hwrng_register(&pdev->dev, &rng->hwrng);
> +       rng->hwrng->priv = (unsigned long)rng;
> +       ret = devm_hwrng_register(&pdev->dev, rng->hwrng);
>         if (ret) {
>                 dev_err(&pdev->dev, "failed to register hwrng\n");
>                 return ret;
> -- 
> 2.14.4
> 

Re: [PATCH 1/3] hwrng: msm - Move hwrng to a table

[Vinod]

On 18-06-18, 08:58, Stephen Boyd wrote:
> Quoting Vinod Koul (2018-06-18 07:12:57)
> > @@ -127,6 +125,13 @@ static void msm_rng_cleanup(struct hwrng *hwrng)
> >         msm_rng_enable(hwrng, 0);
> >  }
> >  
> > +static struct hwrng msm_rng = {
> > +       .name = KBUILD_MODNAME,
> > +       .init = msm_rng_init,
> > +       .cleanup = msm_rng_cleanup,
> > +       .read = msm_rng_read,
> > +};
> > +
> >  static int msm_rng_probe(struct platform_device *pdev)
> >  {
> >         struct resource *res;
> > @@ -148,12 +153,10 @@ static int msm_rng_probe(struct platform_device *pdev)
> >         if (IS_ERR(rng->clk))
> >                 return PTR_ERR(rng->clk);
> >  
> > -       rng->hwrng.name = KBUILD_MODNAME,
> > -       rng->hwrng.init = msm_rng_init,
> > -       rng->hwrng.cleanup = msm_rng_cleanup,
> 
> Wouldn't it be a lot easier to skip assigning the init and cleanup
> functions on v2 devices with an if statement? It would be smaller size
> wise too because then we don't have two structs for v1 and v2 hwrngs.
> Plus the patch would be smaller overall because we would do everything
> else pretty much the same besides the if condition in probe.

Yes it would be an alternate approach and would involve lesser code
change.

My personal preference is table based init rather open coding and it
makes adding future revs easier, but said that I can change this...

-- 
~Vinod

[PATCH 2/3] dt-bindings: rng: Add new compatible qcom,prng-v2

[Vinod Koul]

Later qcom chips support v2 of the prng, so add new compatible
qcom,prng-v2 for this.

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 Documentation/devicetree/bindings/rng/qcom,prng.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/rng/qcom,prng.txt b/Documentation/devicetree/bindings/rng/qcom,prng.txt
index 8e5853c2879b..03fd218bd21a 100644
--- a/Documentation/devicetree/bindings/rng/qcom,prng.txt
+++ b/Documentation/devicetree/bindings/rng/qcom,prng.txt
@@ -2,7 +2,8 @@ Qualcomm MSM pseudo random number generator.
 
 Required properties:
 
-- compatible  : should be "qcom,prng"
+- compatible  : should be "qcom,prng" for 8916 etc
+              : should be "qcom,prng-v2" for 8996 and later
 - reg         : specifies base physical address and size of the registers map
 - clocks      : phandle to clock-controller plus clock-specifier pair
 - clock-names : "core" clocks all registers, FIFO and circuits in PRNG IP block
-- 
2.14.4

[PATCH 3/3] hwrng: msm - Add support for prng v2

[Vinod Koul]

Qcom 8996 and later chips support prng v2 where we need to only
implement .read callback for hwrng.

Add a new table for v2 which supports this and get version required for
driver data.

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/char/hw_random/msm-rng.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/msm-rng.c b/drivers/char/hw_random/msm-rng.c
index 7644474035e5..3f509072a6c6 100644
--- a/drivers/char/hw_random/msm-rng.c
+++ b/drivers/char/hw_random/msm-rng.c
@@ -17,6 +17,7 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 
 /* Device specific register offsets */
@@ -132,10 +133,16 @@ static struct hwrng msm_rng = {
 	.read = msm_rng_read,
 };
 
+static struct hwrng msm_rng_v2 = {
+	.name = KBUILD_MODNAME,
+	.read = msm_rng_read,
+};
+
 static int msm_rng_probe(struct platform_device *pdev)
 {
 	struct resource *res;
 	struct msm_rng *rng;
+	unsigned int version;
 	int ret;
 
 	rng = devm_kzalloc(&pdev->dev, sizeof(*rng), GFP_KERNEL);
@@ -154,6 +161,9 @@ static int msm_rng_probe(struct platform_device *pdev)
 		return PTR_ERR(rng->clk);
 
 	rng->hwrng = &msm_rng;
+	version = (unsigned long)of_device_get_match_data(&pdev->dev);
+	if (version)
+		rng->hwrng = &msm_rng_v2;
 
 	rng->hwrng->priv = (unsigned long)rng;
 	ret = devm_hwrng_register(&pdev->dev, rng->hwrng);
@@ -166,7 +176,8 @@ static int msm_rng_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id msm_rng_of_match[] = {
-	{ .compatible = "qcom,prng", },
+	{ .compatible = "qcom,prng", .data = (void *)0},
+	{ .compatible = "qcom,prng-v2", .data = (void *)1},
 	{}
 };
 MODULE_DEVICE_TABLE(of, msm_rng_of_match);
-- 
2.14.4

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Bjorn Andersson]

On Mon 18 Jun 07:12 PDT 2018, Vinod Koul wrote:

> Qcom 8996 and later chips support prng v2 where we need to only
> implement .read callback for hwrng.
> 

The hardware still needs initialization, so I think you should expand
this to mention that the initialization is moved to secure world and
that's the reason why we only implement read.

The question is what happens in projects with other security models.

> Add a new table for v2 which supports this and get version required for
> driver data.
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/char/hw_random/msm-rng.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/hw_random/msm-rng.c b/drivers/char/hw_random/msm-rng.c
> index 7644474035e5..3f509072a6c6 100644
> --- a/drivers/char/hw_random/msm-rng.c
> +++ b/drivers/char/hw_random/msm-rng.c
> @@ -17,6 +17,7 @@
>  #include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  
>  /* Device specific register offsets */
> @@ -132,10 +133,16 @@ static struct hwrng msm_rng = {
>  	.read = msm_rng_read,
>  };
>  
> +static struct hwrng msm_rng_v2 = {
> +	.name = KBUILD_MODNAME,
> +	.read = msm_rng_read,
> +};
> +
>  static int msm_rng_probe(struct platform_device *pdev)
>  {
>  	struct resource *res;
>  	struct msm_rng *rng;
> +	unsigned int version;
>  	int ret;
>  
>  	rng = devm_kzalloc(&pdev->dev, sizeof(*rng), GFP_KERNEL);
> @@ -154,6 +161,9 @@ static int msm_rng_probe(struct platform_device *pdev)
>  		return PTR_ERR(rng->clk);
>  
>  	rng->hwrng = &msm_rng;
> +	version = (unsigned long)of_device_get_match_data(&pdev->dev);

If this is "version" then please make it 1 or 2, if you agree with
Stephen's suggestion of omitting the initialization of init I think this
would be better as 0/1 and the variable named "skip_init".

> +	if (version)
> +		rng->hwrng = &msm_rng_v2;
>  
>  	rng->hwrng->priv = (unsigned long)rng;
>  	ret = devm_hwrng_register(&pdev->dev, rng->hwrng);
> @@ -166,7 +176,8 @@ static int msm_rng_probe(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id msm_rng_of_match[] = {
> -	{ .compatible = "qcom,prng", },
> +	{ .compatible = "qcom,prng", .data = (void *)0},
> +	{ .compatible = "qcom,prng-v2", .data = (void *)1},
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, msm_rng_of_match);

Regards,
Bjorn

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Stephen Boyd]

Quoting Bjorn Andersson (2018-06-18 11:21:23)
> On Mon 18 Jun 07:12 PDT 2018, Vinod Koul wrote:
> 
> > Qcom 8996 and later chips support prng v2 where we need to only
> > implement .read callback for hwrng.
> > 
> 
> The hardware still needs initialization, so I think you should expand
> this to mention that the initialization is moved to secure world and
> that's the reason why we only implement read.
> 
> The question is what happens in projects with other security models.

Can we still read the PRNG_CONFIG register to see if it's already been
configured or not and then bail out if it isn't configured? That would
be better as long as we the system doesn't blow up if non-secure mode
tries to read the config register.

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Vinod]

On 18-06-18, 13:24, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2018-06-18 11:21:23)
> > On Mon 18 Jun 07:12 PDT 2018, Vinod Koul wrote:
> > 
> > > Qcom 8996 and later chips support prng v2 where we need to only
> > > implement .read callback for hwrng.
> > > 
> > 
> > The hardware still needs initialization, so I think you should expand
> > this to mention that the initialization is moved to secure world and
> > that's the reason why we only implement read.
> > 
> > The question is what happens in projects with other security models.
> 
> Can we still read the PRNG_CONFIG register to see if it's already been
> configured or not and then bail out if it isn't configured? That would
> be better as long as we the system doesn't blow up if non-secure mode
> tries to read the config register.

Unfortunately it did blow up for me when I tried it..

-- 
~Vinod

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Vinod]

On 18-06-18, 11:21, Bjorn Andersson wrote:
> On Mon 18 Jun 07:12 PDT 2018, Vinod Koul wrote:
> 
> > Qcom 8996 and later chips support prng v2 where we need to only
> > implement .read callback for hwrng.
> > 
> 
> The hardware still needs initialization, so I think you should expand
> this to mention that the initialization is moved to secure world and
> that's the reason why we only implement read.
> 
> The question is what happens in projects with other security models.

I did think about that, in those case a DT change would do the trick by
pointing to the TZ EE and loading v1 driver but then is DT board sepcific
or SoC specific, I think latter :(

Can we detect the model..?

> >  	rng = devm_kzalloc(&pdev->dev, sizeof(*rng), GFP_KERNEL);
> > @@ -154,6 +161,9 @@ static int msm_rng_probe(struct platform_device *pdev)
> >  		return PTR_ERR(rng->clk);
> >  
> >  	rng->hwrng = &msm_rng;
> > +	version = (unsigned long)of_device_get_match_data(&pdev->dev);
> 
> If this is "version" then please make it 1 or 2, if you agree with
> Stephen's suggestion of omitting the initialization of init I think this
> would be better as 0/1 and the variable named "skip_init".

ok will do

-- 
~Vinod

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Herbert Xu]

On Mon, Jun 18, 2018 at 07:42:59PM +0530, Vinod Koul wrote:
> Qcom 8996 and later chips support prng v2 where we need to only
> implement .read callback for hwrng.
> 
> Add a new table for v2 which supports this and get version required for
> driver data.
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>

Is this really a pseudo-RNG? If so it needs to be moved over to
the algif_rng interface.

hwrng is for true hardware RNGs only, because it may be directly
fed into /dev/random.

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

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Vinod]

On 19-06-18, 22:28, Herbert Xu wrote:
> On Mon, Jun 18, 2018 at 07:42:59PM +0530, Vinod Koul wrote:
> > Qcom 8996 and later chips support prng v2 where we need to only
> > implement .read callback for hwrng.
> > 
> > Add a new table for v2 which supports this and get version required for
> > driver data.
> > 
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> 
> Is this really a pseudo-RNG? If so it needs to be moved over to
> the algif_rng interface.
> 
> hwrng is for true hardware RNGs only, because it may be directly
> fed into /dev/random.

I am trying to find how how much random output this hardware generates.

Meanwhile, can you please point out examples/Documentation for algif_rng
and if any test tools for this?

Thanks
-- 
~Vinod

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Herbert Xu]

On Wed, Jun 20, 2018 at 11:02:04AM +0530, Vinod wrote:
>
> I am trying to find how how much random output this hardware generates.
> 
> Meanwhile, can you please point out examples/Documentation for algif_rng
> and if any test tools for this?

Have a look at drivers/crypto/exynos-rng.c.

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

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[PrasannaKumar Muralidharan]

Hi Vinod,

On 20 June 2018 at 11:02, Vinod <vkoul@kernel.org> wrote:
> On 19-06-18, 22:28, Herbert Xu wrote:
>> On Mon, Jun 18, 2018 at 07:42:59PM +0530, Vinod Koul wrote:
>> > Qcom 8996 and later chips support prng v2 where we need to only
>> > implement .read callback for hwrng.
>> >
>> > Add a new table for v2 which supports this and get version required for
>> > driver data.
>> >
>> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
>>
>> Is this really a pseudo-RNG? If so it needs to be moved over to
>> the algif_rng interface.
>>
>> hwrng is for true hardware RNGs only, because it may be directly
>> fed into /dev/random.
>
> I am trying to find how how much random output this hardware generates.
>
> Meanwhile, can you please point out examples/Documentation for algif_rng
> and if any test tools for this?

As Herbert suggested exynos rng is a good example. It was wrongly
using hwrng framework then switched to using crypto framework.

I have a patch for msm rng using crypto framework, only compile tested
though. You can find the patch at
https://pastebin.ubuntu.com/p/44r6BJgBkN/. I am having a hard time
sending patches via git send-email form a gmail account so I have put
it in pastebin. Feel free to use it if that is useful.

Regards,
PrasannaKumar

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Vinod]

Hi PrasannaKumar,

On 20-06-18, 23:15, PrasannaKumar Muralidharan wrote:
> Hi Vinod,
> 
> On 20 June 2018 at 11:02, Vinod <vkoul@kernel.org> wrote:
> > On 19-06-18, 22:28, Herbert Xu wrote:
> >> On Mon, Jun 18, 2018 at 07:42:59PM +0530, Vinod Koul wrote:
> >> > Qcom 8996 and later chips support prng v2 where we need to only
> >> > implement .read callback for hwrng.
> >> >
> >> > Add a new table for v2 which supports this and get version required for
> >> > driver data.
> >> >
> >> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> >>
> >> Is this really a pseudo-RNG? If so it needs to be moved over to
> >> the algif_rng interface.
> >>
> >> hwrng is for true hardware RNGs only, because it may be directly
> >> fed into /dev/random.
> >
> > I am trying to find how how much random output this hardware generates.
> >
> > Meanwhile, can you please point out examples/Documentation for algif_rng
> > and if any test tools for this?
> 
> As Herbert suggested exynos rng is a good example. It was wrongly
> using hwrng framework then switched to using crypto framework.
> 
> I have a patch for msm rng using crypto framework, only compile tested
> though. You can find the patch at
> https://pastebin.ubuntu.com/p/44r6BJgBkN/. I am having a hard time
> sending patches via git send-email form a gmail account so I have put
> it in pastebin. Feel free to use it if that is useful.

Ah, I already started based on msm downstream implementation (they had
one using crypto APIs too). Ideally you should have posted it and tested
it.

Thanks
-- 
~Vinod

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Stanimir Varbanov]

Hi Herbert,

On 06/19/2018 05:28 PM, Herbert Xu wrote:
> On Mon, Jun 18, 2018 at 07:42:59PM +0530, Vinod Koul wrote:
>> Qcom 8996 and later chips support prng v2 where we need to only
>> implement .read callback for hwrng.
>>
>> Add a new table for v2 which supports this and get version required for
>> driver data.
>>
>> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> 
> Is this really a pseudo-RNG? If so it needs to be moved over to
> the algif_rng interface.

Despite the register name (PRNG_ registers prefix) the IP is using FIPS
approved algorithm and we can claim that this is true hardware entropy
generator.

I don't think that we need to move to algif_rng.

-- 
regards,
Stan

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Herbert Xu]

On Thu, Jun 21, 2018 at 12:56:34PM +0300, Stanimir Varbanov wrote:
>
> > Is this really a pseudo-RNG? If so it needs to be moved over to
> > the algif_rng interface.
> 
> Despite the register name (PRNG_ registers prefix) the IP is using FIPS
> approved algorithm and we can claim that this is true hardware entropy
> generator.

Whether your RNG is a PRNG or not has nothing to do with the
algorithm you use.  No algorithm can generate entropy out of thin
air.

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

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Stanimir Varbanov]

Hi Herbert,

On 06/21/2018 01:15 PM, Herbert Xu wrote:
> On Thu, Jun 21, 2018 at 12:56:34PM +0300, Stanimir Varbanov wrote:
>>
>>> Is this really a pseudo-RNG? If so it needs to be moved over to
>>> the algif_rng interface.
>>
>> Despite the register name (PRNG_ registers prefix) the IP is using FIPS
>> approved algorithm and we can claim that this is true hardware entropy
>> generator.
> 
> Whether your RNG is a PRNG or not has nothing to do with the
> algorithm you use.  No algorithm can generate entropy out of thin
> air.

OK, I just wanted to say that it is _not_ PRNG and the register names
gives us wrong impression.

-- 
regards,
Stan

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Herbert Xu]

On Thu, Jun 21, 2018 at 02:27:10PM +0300, Stanimir Varbanov wrote:
>
> OK, I just wanted to say that it is _not_ PRNG and the register names
> gives us wrong impression.

So does it generate one bit of output for each bit of hardware-
generated entropy like /dev/random? Or does it use a hardware-
generated seed to power a PRNG?

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

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Stanimir Varbanov]

Hi Herbert,

On 06/21/2018 02:53 PM, Herbert Xu wrote:
> On Thu, Jun 21, 2018 at 02:27:10PM +0300, Stanimir Varbanov wrote:
>>
>> OK, I just wanted to say that it is _not_ PRNG and the register names
>> gives us wrong impression.
> 
> So does it generate one bit of output for each bit of hardware-
> generated entropy like /dev/random? Or does it use a hardware-
> generated seed to power a PRNG?

I believe it is the second one. Isn't the second one SP 800-90A?

-- 
regards,
Stan

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Herbert Xu]

On Fri, Jun 22, 2018 at 11:27:59AM +0300, Stanimir Varbanov wrote:
> Hi Herbert,
> 
> On 06/21/2018 02:53 PM, Herbert Xu wrote:
> > On Thu, Jun 21, 2018 at 02:27:10PM +0300, Stanimir Varbanov wrote:
> >>
> >> OK, I just wanted to say that it is _not_ PRNG and the register names
> >> gives us wrong impression.
> > 
> > So does it generate one bit of output for each bit of hardware-
> > generated entropy like /dev/random? Or does it use a hardware-
> > generated seed to power a PRNG?
> 
> I believe it is the second one. Isn't the second one SP 800-90A?

In that case it should switch over to algif_rng.

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

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Vinod]

On 22-06-18, 22:38, Herbert Xu wrote:
> On Fri, Jun 22, 2018 at 11:27:59AM +0300, Stanimir Varbanov wrote:
> > Hi Herbert,
> > 
> > On 06/21/2018 02:53 PM, Herbert Xu wrote:
> > > On Thu, Jun 21, 2018 at 02:27:10PM +0300, Stanimir Varbanov wrote:
> > >>
> > >> OK, I just wanted to say that it is _not_ PRNG and the register names
> > >> gives us wrong impression.
> > > 
> > > So does it generate one bit of output for each bit of hardware-
> > > generated entropy like /dev/random? Or does it use a hardware-
> > > generated seed to power a PRNG?
> > 
> > I believe it is the second one. Isn't the second one SP 800-90A?
> 
> In that case it should switch over to algif_rng.

Okay I am doing the port taking the exynos-rng as a ref.
Question is how to test it, how is one supposed to exercise the rng, any
test utils/apps for that? Sorry for noob question, new to crypto
interfaces.

-- 
~Vinod

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Herbert Xu]

On Fri, Jun 22, 2018 at 08:18:09PM +0530, Vinod wrote:
>
> Okay I am doing the port taking the exynos-rng as a ref.
> Question is how to test it, how is one supposed to exercise the rng, any
> test utils/apps for that? Sorry for noob question, new to crypto
> interfaces.

algif_rng is available through the af_alg socket interface.

Ccing Stephan as he has a library that may help you.

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

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Vinod]

On 22-06-18, 19:57, Stephan Mueller wrote:
> Hi
> 
> 
> > Am 22.06.2018 um 16:50 schrieb Herbert Xu <herbert@gondor.apana.org.au>:
> > 
> >> On Fri, Jun 22, 2018 at 08:18:09PM +0530, Vinod wrote:
> >> 
> >> Okay I am doing the port taking the exynos-rng as a ref.
> >> Question is how to test it, how is one supposed to exercise the rng, any
> >> test utils/apps for that? Sorry for noob question, new to crypto
> >> interfaces.
> > 
> > algif_rng is available through the af_alg socket interface.
> > 
> You can use the libkcapi library at http://www.chronox.de/libkcapi.html
> 
> The RNG API is documented at http://chronox.de/libkcapi/html/ch03s15.html and http://chronox.de/libkcapi/html/ch03s16.html
> 
> A command line app is also present with kcapi-rng as documented at https://github.com/smuellerDD/libkcapi/blob/master/README.md

Thanks for the pointers, it helped me to test the driver :)

I have two follow up question on crypto:

 - If there a way to avoid using a global variable in driver to hold the
   pointer for driver memory? Looks like exynos driver does that.

   I understand that the crypto callback don't provide driver context as
   they copy the data structures passed in registration API, but a simpler
   way to get driver context would be desirable.

 - .seed seems to be mandatory, if I do not set it and even use
   .seedsize = 0, it panics at crypto_rng_reset(). So is .seed
   mandatory?

Thanks
-- 
~Vinod

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Stephan Mueller]

Am Mittwoch, 27. Juni 2018, 07:08:53 CEST schrieb Vinod:

Hi Vinod,

> Thanks for the pointers, it helped me to test the driver :)
> 
> I have two follow up question on crypto:
> 
>  - If there a way to avoid using a global variable in driver to hold the
>    pointer for driver memory? Looks like exynos driver does that.
> 
>    I understand that the crypto callback don't provide driver context as
>    they copy the data structures passed in registration API, but a simpler
>    way to get driver context would be desirable.

Sure the kernel crypto API can and has to maintain a per-instance data 
structure.

See the crypto/drbg.c for instance.

static int drbg_kcapi_random(struct crypto_rng *tfm,
                             const u8 *src, unsigned int slen,
                             u8 *dst, unsigned int dlen)
{
        struct drbg_state *drbg = crypto_rng_ctx(tfm);

static int drbg_kcapi_seed(struct crypto_rng *tfm,
                           const u8 *seed, unsigned int slen)
{
        struct drbg_state *drbg = crypto_rng_ctx(tfm);

The key is:

        alg->base.cra_ctxsize   = sizeof(struct drbg_state);

during initialization since the kernel crypto API allocates that buffer for 
you and releases it during deallocation.
> 
>  - .seed seems to be mandatory, if I do not set it and even use
>    .seedsize = 0, it panics at crypto_rng_reset(). So is .seed
>    mandatory?

Well, seedsize = 0 just says that the RNG is ready to use after initialization 
(i.e. it does not need to be seeded after initialization).

That does not preclude that a caller wants to reseed.

And yes, .seed must be set.
> 
> Thanks



Ciao
Stephan

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Vinod]

Hi Stephan,

Thanks for the answers, they are helpful.

On 27-06-18, 08:13, Stephan Mueller wrote:
> > I have two follow up question on crypto:
> > 
> >  - If there a way to avoid using a global variable in driver to hold the
> >    pointer for driver memory? Looks like exynos driver does that.
> > 
> >    I understand that the crypto callback don't provide driver context as
> >    they copy the data structures passed in registration API, but a simpler
> >    way to get driver context would be desirable.
> 
> Sure the kernel crypto API can and has to maintain a per-instance data 
> structure.
> 
> See the crypto/drbg.c for instance.
> 
> static int drbg_kcapi_random(struct crypto_rng *tfm,
>                              const u8 *src, unsigned int slen,
>                              u8 *dst, unsigned int dlen)
> {
>         struct drbg_state *drbg = crypto_rng_ctx(tfm);
> 
> static int drbg_kcapi_seed(struct crypto_rng *tfm,
>                            const u8 *seed, unsigned int slen)
> {
>         struct drbg_state *drbg = crypto_rng_ctx(tfm);
> 
> The key is:
> 
>         alg->base.cra_ctxsize   = sizeof(struct drbg_state);
> 
> during initialization since the kernel crypto API allocates that buffer for 
> you and releases it during deallocation.

The difference here is that memory is allocated by crypto and driver has
no way to pass "it's" own data while doing registration. Ideally
registration should accept a pointer/long and pass that back on a
callbacks

Currently am doing bunch of initialization in .probe (platform driver)
and I think recommendation would be to move that to .cra_init, which seem
plausible but I don't have pdev to read hw_resource etc.. so would still
need to get that.

FWIW here is the code I wrote:
https://git.linaro.org/people/vinod.koul/kernel.git/commit/?h=topic/rng_v2&id=feb23a41afb0d4cf42a2825b84a43dbc9a49e8b9

-- 
~Vinod

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Stephan Mueller]

Am Mittwoch, 27. Juni 2018, 08:27:01 CEST schrieb Vinod:

Hi Vinod,

> Hi Stephan,
> 
> Thanks for the answers, they are helpful.
> 
> On 27-06-18, 08:13, Stephan Mueller wrote:
> > > I have two follow up question on crypto:
> > >  - If there a way to avoid using a global variable in driver to hold the
> > >  
> > >    pointer for driver memory? Looks like exynos driver does that.
> > >    
> > >    I understand that the crypto callback don't provide driver context as
> > >    they copy the data structures passed in registration API, but a
> > >    simpler
> > >    way to get driver context would be desirable.
> > 
> > Sure the kernel crypto API can and has to maintain a per-instance data
> > structure.
> > 
> > See the crypto/drbg.c for instance.
> > 
> > static int drbg_kcapi_random(struct crypto_rng *tfm,
> > 
> >                              const u8 *src, unsigned int slen,
> >                              u8 *dst, unsigned int dlen)
> > 
> > {
> > 
> >         struct drbg_state *drbg = crypto_rng_ctx(tfm);
> > 
> > static int drbg_kcapi_seed(struct crypto_rng *tfm,
> > 
> >                            const u8 *seed, unsigned int slen)
> > 
> > {
> > 
> >         struct drbg_state *drbg = crypto_rng_ctx(tfm);
> > 
> > The key is:
> >         alg->base.cra_ctxsize   = sizeof(struct drbg_state);
> > 
> > during initialization since the kernel crypto API allocates that buffer
> > for
> > you and releases it during deallocation.
> 
> The difference here is that memory is allocated by crypto and driver has
> no way to pass "it's" own data while doing registration. Ideally
> registration should accept a pointer/long and pass that back on a
> callbacks

Looking at your code, it seems you do what makes sense: there is only one 
instance of the driver, if at all. Thus, having qcom_rng_dev as static makes 
sense. The kernel crypto API allows arbitrary instances of the RNG as well as 
frequent allocations and deallocations. And this is why there must be a 
disconnect between the one hardware-resource driver-instance data structure 
and the (potentially) multiple crypto API RNG instances and their data 
structures.

> 
> Currently am doing bunch of initialization in .probe (platform driver)
> and I think recommendation would be to move that to .cra_init, which seem
> plausible but I don't have pdev to read hw_resource etc.. so would still
> need to get that.

It seems that your allocation during probe relates to the hardware resource 
where you only have one in the system. Thus, doing the allocation here makes 
sense. And, you do not want to perform probe or such resource allocation once 
per crypto API RNG instance allocation. As said, there can be multiple or even 
they can be allocated and deallocated frequently. This in particular applies 
if your driver's "stdrng" has the highest prio which means that it will be 
allocated and deallocated frequently.

Ciao
Stephan

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Vinod]

Hi Stephan,

Thanks for quick reply,

On 27-06-18, 08:43, Stephan Mueller wrote:
> > On 27-06-18, 08:13, Stephan Mueller wrote:
> > > The key is:
> > >         alg->base.cra_ctxsize   = sizeof(struct drbg_state);
> > > 
> > > during initialization since the kernel crypto API allocates that buffer
> > > for
> > > you and releases it during deallocation.
> > 
> > The difference here is that memory is allocated by crypto and driver has
> > no way to pass "it's" own data while doing registration. Ideally
> > registration should accept a pointer/long and pass that back on a
> > callbacks
> 
> Looking at your code, it seems you do what makes sense: there is only one 
> instance of the driver, if at all. Thus, having qcom_rng_dev as static makes 
> sense. The kernel crypto API allows arbitrary instances of the RNG as well as 
> frequent allocations and deallocations. And this is why there must be a 
> disconnect between the one hardware-resource driver-instance data structure 
> and the (potentially) multiple crypto API RNG instances and their data 
> structures.

For now it is true, but somehow doesn't make me happy to bank on that..

> > 
> > Currently am doing bunch of initialization in .probe (platform driver)
> > and I think recommendation would be to move that to .cra_init, which seem
> > plausible but I don't have pdev to read hw_resource etc.. so would still
> > need to get that.
> 
> It seems that your allocation during probe relates to the hardware resource 
> where you only have one in the system. Thus, doing the allocation here makes 
> sense. And, you do not want to perform probe or such resource allocation once 
> per crypto API RNG instance allocation. As said, there can be multiple or even 
> they can be allocated and deallocated frequently. This in particular applies 
> if your driver's "stdrng" has the highest prio which means that it will be 
> allocated and deallocated frequently.

Right, that is how I visualized it.

Is there a way where we can tweak the register API to pass hw_resource
pointer and get that back? Would that work with the security model in
crypto.

I do not like globals and somehow don't feel that we should do it that
way

Thanks for the quick look on the code.

~Vinod

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Stephan Mueller]

Am Mittwoch, 27. Juni 2018, 09:01:44 CEST schrieb Vinod:

Hi Vinod,

> > > Currently am doing bunch of initialization in .probe (platform driver)
> > > and I think recommendation would be to move that to .cra_init, which
> > > seem
> > > plausible but I don't have pdev to read hw_resource etc.. so would still
> > > need to get that.
> > 
> > It seems that your allocation during probe relates to the hardware
> > resource
> > where you only have one in the system. Thus, doing the allocation here
> > makes sense. And, you do not want to perform probe or such resource
> > allocation once per crypto API RNG instance allocation. As said, there
> > can be multiple or even they can be allocated and deallocated frequently.
> > This in particular applies if your driver's "stdrng" has the highest prio
> > which means that it will be allocated and deallocated frequently.
> 
> Right, that is how I visualized it.
> 
> Is there a way where we can tweak the register API to pass hw_resource
> pointer and get that back? Would that work with the security model in
> crypto.

I would not see an easy way for that. The core register API of the kernel 
crypto API would need to be changed.
> 
> I do not like globals and somehow don't feel that we should do it that
> way

Granted, I concur here.



Ciao
Stephan

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Stanimir Varbanov]

Hi,

On 06/22/2018 05:38 PM, Herbert Xu wrote:
> On Fri, Jun 22, 2018 at 11:27:59AM +0300, Stanimir Varbanov wrote:
>> Hi Herbert,
>>
>> On 06/21/2018 02:53 PM, Herbert Xu wrote:
>>> On Thu, Jun 21, 2018 at 02:27:10PM +0300, Stanimir Varbanov wrote:
>>>>
>>>> OK, I just wanted to say that it is _not_ PRNG and the register names
>>>> gives us wrong impression.
>>>
>>> So does it generate one bit of output for each bit of hardware-
>>> generated entropy like /dev/random? Or does it use a hardware-
>>> generated seed to power a PRNG?
>>
>> I believe it is the second one. Isn't the second one SP 800-90A?
> 
> In that case it should switch over to algif_rng.

OK, what about the rest drivers in drivers/char/hw_random? Are they all
true RNG?

-- 
regards,
Stan

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Timur Tabi]

On Thu, Jun 21, 2018 at 6:53 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Thu, Jun 21, 2018 at 02:27:10PM +0300, Stanimir Varbanov wrote:

> So does it generate one bit of output for each bit of hardware-
> generated entropy like /dev/random? Or does it use a hardware-
> generated seed to power a PRNG?

I have some information to answer this question, although I'm not sure
I can give a strict "yes/no" answer.

There are a couple relevant documents:

https://www.qualcomm.com/news/onq/2014/11/07/cryptographic-module-snapdragon-805-fips-140-2-certified
https://csrc.nist.gov/CSRC/media/projects/cryptographic-module-validation-program/documents/security-policies/140sp2944.pdf

I also got response from a Qualcomm employee:

"The Qualcomm random number generator used in Snapdragon chips
consists of an entropy source coupled with the HASH-DRBG deterministic
random bit generator from NIST Special Publication 800-90A, using
SHA-256 as the hash function.

The entropy source is based on sampled ring oscillators.  Four ring
oscillators are used to provide high assurance of adequate entropy.
The entropy from the ring oscillators is conditioned using the
'derivation function' specified by NIST Special Publication 800-90A.
The conditioned entropy is essentially perfect fully entropic data.
It is used both to seed and to periodically reseed the DRGB."

My understanding is that the PRNG is a real entropy source with some
logic used to normalize the values.  To quote: "No RNG uses data
directly from the entropy source; bits in the output are likely
correlated and unlikely to occur with 50% probability. The entropy
post-processing is designed to turn dirty data in clean data."

Based on the above, it seems to me that the Qualcomm PRNG qualifies as
a real hardware RNG and porting to algif_rng is not the correct path.

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Vinod]

On 28-06-18, 17:04, Timur Tabi wrote:
> On Thu, Jun 21, 2018 at 6:53 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > On Thu, Jun 21, 2018 at 02:27:10PM +0300, Stanimir Varbanov wrote:
> 
> > So does it generate one bit of output for each bit of hardware-
> > generated entropy like /dev/random? Or does it use a hardware-
> > generated seed to power a PRNG?
> 
> I have some information to answer this question, although I'm not sure
> I can give a strict "yes/no" answer.
> 
> There are a couple relevant documents:
> 
> https://www.qualcomm.com/news/onq/2014/11/07/cryptographic-module-snapdragon-805-fips-140-2-certified
> https://csrc.nist.gov/CSRC/media/projects/cryptographic-module-validation-program/documents/security-policies/140sp2944.pdf
> 
> I also got response from a Qualcomm employee:
> 
> "The Qualcomm random number generator used in Snapdragon chips
> consists of an entropy source coupled with the HASH-DRBG deterministic
> random bit generator from NIST Special Publication 800-90A, using
> SHA-256 as the hash function.
> 
> The entropy source is based on sampled ring oscillators.  Four ring
> oscillators are used to provide high assurance of adequate entropy.
> The entropy from the ring oscillators is conditioned using the
> 'derivation function' specified by NIST Special Publication 800-90A.
> The conditioned entropy is essentially perfect fully entropic data.
> It is used both to seed and to periodically reseed the DRGB."
> 
> My understanding is that the PRNG is a real entropy source with some
> logic used to normalize the values.  To quote: "No RNG uses data
> directly from the entropy source; bits in the output are likely
> correlated and unlikely to occur with 50% probability. The entropy
> post-processing is designed to turn dirty data in clean data."
> 
> Based on the above, it seems to me that the Qualcomm PRNG qualifies as
> a real hardware RNG and porting to algif_rng is not the correct path.

I think Stan did bring this point earlier that PRNG is compliant to
FIPS-140-2. So it can be used by rng clients for various purposes but
should not be fed to dev/random as the hw_random does.

Herbert, can you please confirm..

-- 
~Vinod

Re: [PATCH 3/3] hwrng: msm - Add support for prng v2

[Herbert Xu]

On Fri, Jun 29, 2018 at 02:07:32PM +0530, Vinod wrote:
>
> I think Stan did bring this point earlier that PRNG is compliant to
> FIPS-140-2. So it can be used by rng clients for various purposes but
> should not be fed to dev/random as the hw_random does.
> 
> Herbert, can you please confirm..

Yes this is a PRNG.  A hardware RNG should return 1 bit of entropy
per bit of output.

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