diff mbox series

[v2,2/2] hwrng: add OP-TEE based rng driver

Message ID 1545300998-10069-3-git-send-email-sumit.garg@linaro.org
State Superseded
Headers show
Series Add OP-TEE based hwrng driver | expand

Commit Message

Sumit Garg Dec. 20, 2018, 10:16 a.m. UTC
On ARM SoC's with TrustZone enabled, peripherals like entropy sources
might not be accessible to normal world (linux in this case) and rather
accessible to secure world (OP-TEE in this case) only. So this driver
aims to provides a generic interface to OP-TEE based random number
generator service.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

---
 MAINTAINERS                        |   5 +
 drivers/char/hw_random/Kconfig     |  15 ++
 drivers/char/hw_random/Makefile    |   1 +
 drivers/char/hw_random/optee-rng.c | 280 +++++++++++++++++++++++++++++++++++++
 4 files changed, 301 insertions(+)
 create mode 100644 drivers/char/hw_random/optee-rng.c

-- 
2.7.4

Comments

Daniel Thompson Dec. 20, 2018, 12:01 p.m. UTC | #1
On Thu, Dec 20, 2018 at 03:46:38PM +0530, Sumit Garg wrote:
> On ARM SoC's with TrustZone enabled, peripherals like entropy sources

> might not be accessible to normal world (linux in this case) and rather

> accessible to secure world (OP-TEE in this case) only. So this driver

> aims to provides a generic interface to OP-TEE based random number

> generator service.

> 

> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

> ---

>  MAINTAINERS                        |   5 +

>  drivers/char/hw_random/Kconfig     |  15 ++

>  drivers/char/hw_random/Makefile    |   1 +

>  drivers/char/hw_random/optee-rng.c | 280 +++++++++++++++++++++++++++++++++++++

>  4 files changed, 301 insertions(+)

>  create mode 100644 drivers/char/hw_random/optee-rng.c

> 

> diff --git a/MAINTAINERS b/MAINTAINERS

> index 0767f1d..fe0fb74 100644

> --- a/MAINTAINERS

> +++ b/MAINTAINERS

> @@ -11100,6 +11100,11 @@ M:	Jens Wiklander <jens.wiklander@linaro.org>

>  S:	Maintained

>  F:	drivers/tee/optee/

>  

> +OP-TEE RANDOM NUMBER GENERATOR (RNG) DRIVER

> +M:	Sumit Garg <sumit.garg@linaro.org>

> +S:	Maintained

> +F:	drivers/char/hw_random/optee-rng.c

> +

>  OPA-VNIC DRIVER

>  M:	Dennis Dalessandro <dennis.dalessandro@intel.com>

>  M:	Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>

> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig

> index dac895d..25a7d8f 100644

> --- a/drivers/char/hw_random/Kconfig

> +++ b/drivers/char/hw_random/Kconfig

> @@ -424,6 +424,21 @@ config HW_RANDOM_EXYNOS

>  	  will be called exynos-trng.

>  

>  	  If unsure, say Y.

> +

> +config HW_RANDOM_OPTEE

> +	tristate "OP-TEE based Random Number Generator support"

> +	depends on OPTEE

> +	default HW_RANDOM

> +	help

> +	  This  driver provides support for OP-TEE based Random Number

> +	  Generator on ARM SoCs where hardware entropy sources are not

> +	  accessible to normal world (Linux).

> +

> +	  To compile this driver as a module, choose M here: the module

> +	  will be called optee-rng.

> +

> +	  If unsure, say Y.

> +

>  endif # HW_RANDOM

>  

>  config UML_RANDOM

> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile

> index e35ec3c..7c9ef4a 100644

> --- a/drivers/char/hw_random/Makefile

> +++ b/drivers/char/hw_random/Makefile

> @@ -38,3 +38,4 @@ obj-$(CONFIG_HW_RANDOM_CAVIUM) += cavium-rng.o cavium-rng-vf.o

>  obj-$(CONFIG_HW_RANDOM_MTK)	+= mtk-rng.o

>  obj-$(CONFIG_HW_RANDOM_S390) += s390-trng.o

>  obj-$(CONFIG_HW_RANDOM_KEYSTONE) += ks-sa-rng.o

> +obj-$(CONFIG_HW_RANDOM_OPTEE) += optee-rng.o

> diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c

> new file mode 100644

> index 0000000..0dbe210

> --- /dev/null

> +++ b/drivers/char/hw_random/optee-rng.c

> @@ -0,0 +1,280 @@

> +// SPDX-License-Identifier: GPL-2.0

> +/*

> + * Copyright (C) 2018 Linaro Ltd.

> + */

> +

> +#include <linux/delay.h>

> +#include <linux/of.h>

> +#include <linux/hw_random.h>

> +#include <linux/kernel.h>

> +#include <linux/module.h>

> +#include <linux/tee_drv.h>

> +#include <linux/uuid.h>

> +

> +#define PFX	KBUILD_MODNAME ": "


Definitely not! This is what pr_fmt() is for.


> +

> +#define TEE_ERROR_HEALTH_TEST_FAIL	0x00000001

> +

> +/*

> + * TA_CMD_GET_ENTROPY - Get Entropy from RNG

> + *

> + * param[0] (inout memref) - Entropy buffer memory reference

> + * param[1] unused

> + * param[2] unused

> + * param[3] unused

> + *

> + * Result:

> + * TEE_SUCCESS - Invoke command success

> + * TEE_ERROR_BAD_PARAMETERS - Incorrect input param

> + * TEE_ERROR_NOT_SUPPORTED - Requested entropy size greater than size of pool

> + * TEE_ERROR_HEALTH_TEST_FAIL - Continuous health testing failed

> + */

> +#define TA_CMD_GET_ENTROPY		0x0

> +

> +/*

> + * PTA_CMD_GET_RNG_INFO - Get RNG information

      ^^^

Does not match.


> + *

> + * param[0] (out value) - value.a: RNG data-rate

> + *                        value.b: Quality/Entropy per 1024 bit of data

> + * param[1] unused

> + * param[2] unused

> + * param[3] unused

> + *

> + * Result:

> + * TEE_SUCCESS - Invoke command success

> + * TEE_ERROR_BAD_PARAMETERS - Incorrect input param

> + */

> +#define TA_CMD_GET_RNG_INFO		0x1

> +

> +#define MAX_ENTROPY_REQ_SZ		(4 * 1024)

> +

> +static struct tee_context *ctx;

> +static struct tee_shm *entropy_shm_pool;

> +static u32 ta_rng_data_rate;

> +static u32 ta_rng_seesion_id;

> +

> +static size_t get_optee_rng_data(void *buf, size_t req_size)

> +{

> +	u32 ret = 0;

> +	u8 *rng_data = NULL;

> +	size_t rng_size = 0;

> +	struct tee_ioctl_invoke_arg inv_arg = {0};

> +	struct tee_param param[4] = {0};

> +

> +	/* Invoke TA_CMD_GET_RNG function of Trusted App */

> +	inv_arg.func = TA_CMD_GET_ENTROPY;

> +	inv_arg.session = ta_rng_seesion_id;

> +	inv_arg.num_params = 4;

> +

> +	/* Fill invoke cmd params */

> +	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT;

> +	param[0].u.memref.shm = entropy_shm_pool;

> +	param[0].u.memref.size = req_size;

> +	param[0].u.memref.shm_offs = 0;

> +

> +	ret = tee_client_invoke_func(ctx, &inv_arg, param);

> +	if ((ret < 0) || (inv_arg.ret != 0)) {

> +		pr_err(PFX "TA_CMD_GET_ENTROPY invoke function error: %x\n",

> +		       inv_arg.ret);

> +		return 0;

> +	}

> +

> +	rng_data = tee_shm_get_va(entropy_shm_pool, 0);

> +	if (IS_ERR(rng_data)) {

> +		pr_err(PFX "tee_shm_get_va failed\n");

> +		return 0;

> +	}

> +

> +	rng_size = param[0].u.memref.size;

> +	memcpy(buf, rng_data, rng_size);

> +

> +	return rng_size;

> +}

> +

> +static int optee_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)

> +{

> +	u8 *data = buf;

> +	size_t read = 0, rng_size = 0;

> +	int timeout = max / MAX_ENTROPY_REQ_SZ + 1;

> +

> +	/* New random numbers are generated approximately 1byte per 2ms */


This is only true on Developerbox.


> +	while (read < max) {


Not sure about this. This is a read.2 like interface. If we have data
available why not return it to the upper level.


> +		if ((max - read) < MAX_ENTROPY_REQ_SZ)

> +			rng_size = get_optee_rng_data(data, (max - read));

> +		else

> +			rng_size = get_optee_rng_data(data, MAX_ENTROPY_REQ_SZ);

> +

> +		data += rng_size;

> +		read += rng_size;

> +

> +		if (wait) {

> +			if (timeout-- == 0)

> +				return read;

> +			if ((max - read) < MAX_ENTROPY_REQ_SZ)

> +				msleep((1000 * (max - read)) /

> +				       ta_rng_data_rate);

> +			else

> +				msleep((1000 * MAX_ENTROPY_REQ_SZ) /

> +				       ta_rng_data_rate);


Similarly don't do this. Handle it in the header:

if (max > MAX_ENTROPY_REQ_SZ)
	max = MAX_ENTROPY_REQ_SZ;

(IIRC max is never larger than 32 anyway... there might be something in
the headers to help confirm this).


> +		} else {

> +			return read;

> +		}

> +	}

> +

> +	return read;

> +}

> +

> +static int optee_rng_init(struct hwrng *rng)

> +{

> +	entropy_shm_pool = tee_shm_alloc(ctx, MAX_ENTROPY_REQ_SZ,

> +					 TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);

> +	if (IS_ERR(entropy_shm_pool)) {

> +		pr_err(PFX "tee_shm_alloc failed\n");

> +		return PTR_ERR(entropy_shm_pool);

> +	}

> +

> +	return 0;

> +}

> +

> +static void optee_rng_cleanup(struct hwrng *rng)

> +{

> +	tee_shm_free(entropy_shm_pool);

> +}

> +

> +static struct hwrng optee_rng = {

> +	.name		= "optee-rng",

> +	.init		= optee_rng_init,

> +	.cleanup	= optee_rng_cleanup,

> +	.read		= optee_rng_read,

> +};

> +

> +static const struct of_device_id optee_match[] = {

> +	{ .compatible = "linaro,optee-tz" },

> +	{},

> +};

> +

> +static int get_optee_rng_uuid(uuid_t *ta_rng_uuid)

> +{

> +	struct device_node *fw_np;

> +	struct device_node *np;

> +	const char *uuid;

> +

> +	/* Node is supposed to be below /firmware */

> +	fw_np = of_find_node_by_name(NULL, "firmware");

> +	if (!fw_np)

> +		return -ENODEV;

> +

> +	np = of_find_matching_node(fw_np, optee_match);

> +	if (!np || !of_device_is_available(np))

> +		return -ENODEV;

> +

> +	if (of_property_read_string(np, "rng-uuid", &uuid)) {

> +		pr_warn(PFX "missing \"uuid\" property\n");

> +		return -ENXIO;

> +	}

> +

> +	if (uuid_parse(uuid, ta_rng_uuid)) {

> +		pr_warn(PFX "incorrect rng ta uuid\n");

> +		return -EINVAL;

> +	}

> +

> +	return 0;

> +}

> +

> +static int get_optee_rng_info(void)

> +{

> +	u32 ret = 0;

> +	struct tee_ioctl_invoke_arg inv_arg = {0};

> +	struct tee_param param[4] = {0};

> +

> +	/* Invoke TA_CMD_GET_RNG function of Trusted App */

> +	inv_arg.func = TA_CMD_GET_RNG_INFO;

> +	inv_arg.session = ta_rng_seesion_id;

> +	inv_arg.num_params = 4;

> +

> +	/* Fill invoke cmd params */

> +	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;

> +

> +	ret = tee_client_invoke_func(ctx, &inv_arg, param);

> +	if ((ret < 0) || (inv_arg.ret != 0)) {

> +		pr_err(PFX "TA_CMD_GET_RNG_INFO invoke function error: %x\n",

> +		       inv_arg.ret);

> +		return -EINVAL;

> +	}

> +

> +	ta_rng_data_rate = param[0].u.value.a;

> +	optee_rng.quality = param[0].u.value.b;

> +

> +	return 0;

> +}

> +

> +static int tee_match(struct tee_ioctl_version_data *ver, const void *data)

> +{

> +	if (ver->impl_id == TEE_IMPL_ID_OPTEE)


Either this is the tee-rng driver or this special tee matcher function
is pointless and can be removed (and the comparison done inline).


> +		return 1;

> +	else

> +		return 0;

> +}

> +

> +static int __init mod_init(void)

> +{

> +	int ret = 0, err = -ENODEV;

> +	struct tee_ioctl_open_session_arg sess_arg = {0};

> +	uuid_t ta_rng_uuid = {0};

> +

> +	err = get_optee_rng_uuid(&ta_rng_uuid);

> +	if (err)

> +		return err;

> +

> +	/* Open context with TEE driver */

> +	ctx = tee_client_open_context(NULL, tee_match, NULL, NULL);

> +	if (IS_ERR(ctx))

> +		return -ENODEV;


Wow!

A driver that doesn't use the driver model. I haven't seen one of those
in *years*. That might be a problem but I'm not entirely sure.

Ultimately if feels like the kernel code is missing a "tee bus".
Basically I'd expect a driver to register itself and say what UUIDs it
supports and then the TEE bus to probe drivers based on the list of
UUIDs it thinks it has.

Probably better to discuss this in the upstream though.


> +

> +	/* Open session with hwrng Trusted App */

> +	memcpy(sess_arg.uuid, ta_rng_uuid.b, TEE_IOCTL_UUID_LEN);

> +	sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;

> +	sess_arg.num_params = 0;

> +

> +	ret = tee_client_open_session(ctx, &sess_arg, NULL);

> +	if ((ret < 0) || (sess_arg.ret != 0)) {

> +		pr_err(PFX "tee_client_open_session failed, error: %x\n",

> +				 sess_arg.ret);

> +		err = -EINVAL;

> +		goto out_ctx;

> +	}

> +	ta_rng_seesion_id = sess_arg.session;

> +

> +	err = get_optee_rng_info();

> +	if (err)

> +		goto out_sess;

> +

> +	err = hwrng_register(&optee_rng);

> +	if (err) {

> +		pr_err(PFX " registering failed (%d)\n", err);

> +		goto out_sess;

> +	}

> +

> +	return 0;

> +

> +out_sess:

> +	tee_client_close_session(ctx, ta_rng_seesion_id);

> +out_ctx:

> +	tee_client_close_context(ctx);

> +

> +	return err;

> +}

> +

> +static void __exit mod_exit(void)

> +{

> +	tee_client_close_session(ctx, ta_rng_seesion_id);

> +	tee_client_close_context(ctx);

> +	hwrng_unregister(&optee_rng);

> +}

> +

> +module_init(mod_init);

> +module_exit(mod_exit);

> +

> +MODULE_LICENSE("GPL");

> +MODULE_AUTHOR("Sumit Garg <sumit.garg@linaro.org>");

> +MODULE_DESCRIPTION("OP-TEE based random number generator driver");

> -- 

> 2.7.4

>
Sumit Garg Dec. 20, 2018, 12:46 p.m. UTC | #2
On Thu, 20 Dec 2018 at 17:31, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>

> On Thu, Dec 20, 2018 at 03:46:38PM +0530, Sumit Garg wrote:

> > On ARM SoC's with TrustZone enabled, peripherals like entropy sources

> > might not be accessible to normal world (linux in this case) and rather

> > accessible to secure world (OP-TEE in this case) only. So this driver

> > aims to provides a generic interface to OP-TEE based random number

> > generator service.

> >

> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

> > ---

> >  MAINTAINERS                        |   5 +

> >  drivers/char/hw_random/Kconfig     |  15 ++

> >  drivers/char/hw_random/Makefile    |   1 +

> >  drivers/char/hw_random/optee-rng.c | 280 +++++++++++++++++++++++++++++++++++++

> >  4 files changed, 301 insertions(+)

> >  create mode 100644 drivers/char/hw_random/optee-rng.c

> >

> > diff --git a/MAINTAINERS b/MAINTAINERS

> > index 0767f1d..fe0fb74 100644

> > --- a/MAINTAINERS

> > +++ b/MAINTAINERS

> > @@ -11100,6 +11100,11 @@ M:   Jens Wiklander <jens.wiklander@linaro.org>

> >  S:   Maintained

> >  F:   drivers/tee/optee/

> >

> > +OP-TEE RANDOM NUMBER GENERATOR (RNG) DRIVER

> > +M:   Sumit Garg <sumit.garg@linaro.org>

> > +S:   Maintained

> > +F:   drivers/char/hw_random/optee-rng.c

> > +

> >  OPA-VNIC DRIVER

> >  M:   Dennis Dalessandro <dennis.dalessandro@intel.com>

> >  M:   Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>

> > diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig

> > index dac895d..25a7d8f 100644

> > --- a/drivers/char/hw_random/Kconfig

> > +++ b/drivers/char/hw_random/Kconfig

> > @@ -424,6 +424,21 @@ config HW_RANDOM_EXYNOS

> >         will be called exynos-trng.

> >

> >         If unsure, say Y.

> > +

> > +config HW_RANDOM_OPTEE

> > +     tristate "OP-TEE based Random Number Generator support"

> > +     depends on OPTEE

> > +     default HW_RANDOM

> > +     help

> > +       This  driver provides support for OP-TEE based Random Number

> > +       Generator on ARM SoCs where hardware entropy sources are not

> > +       accessible to normal world (Linux).

> > +

> > +       To compile this driver as a module, choose M here: the module

> > +       will be called optee-rng.

> > +

> > +       If unsure, say Y.

> > +

> >  endif # HW_RANDOM

> >

> >  config UML_RANDOM

> > diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile

> > index e35ec3c..7c9ef4a 100644

> > --- a/drivers/char/hw_random/Makefile

> > +++ b/drivers/char/hw_random/Makefile

> > @@ -38,3 +38,4 @@ obj-$(CONFIG_HW_RANDOM_CAVIUM) += cavium-rng.o cavium-rng-vf.o

> >  obj-$(CONFIG_HW_RANDOM_MTK)  += mtk-rng.o

> >  obj-$(CONFIG_HW_RANDOM_S390) += s390-trng.o

> >  obj-$(CONFIG_HW_RANDOM_KEYSTONE) += ks-sa-rng.o

> > +obj-$(CONFIG_HW_RANDOM_OPTEE) += optee-rng.o

> > diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c

> > new file mode 100644

> > index 0000000..0dbe210

> > --- /dev/null

> > +++ b/drivers/char/hw_random/optee-rng.c

> > @@ -0,0 +1,280 @@

> > +// SPDX-License-Identifier: GPL-2.0

> > +/*

> > + * Copyright (C) 2018 Linaro Ltd.

> > + */

> > +

> > +#include <linux/delay.h>

> > +#include <linux/of.h>

> > +#include <linux/hw_random.h>

> > +#include <linux/kernel.h>

> > +#include <linux/module.h>

> > +#include <linux/tee_drv.h>

> > +#include <linux/uuid.h>

> > +

> > +#define PFX  KBUILD_MODNAME ": "

>

> Definitely not! This is what pr_fmt() is for.

>


Ok will remove this.

>

> > +

> > +#define TEE_ERROR_HEALTH_TEST_FAIL   0x00000001

> > +

> > +/*

> > + * TA_CMD_GET_ENTROPY - Get Entropy from RNG

> > + *

> > + * param[0] (inout memref) - Entropy buffer memory reference

> > + * param[1] unused

> > + * param[2] unused

> > + * param[3] unused

> > + *

> > + * Result:

> > + * TEE_SUCCESS - Invoke command success

> > + * TEE_ERROR_BAD_PARAMETERS - Incorrect input param

> > + * TEE_ERROR_NOT_SUPPORTED - Requested entropy size greater than size of pool

> > + * TEE_ERROR_HEALTH_TEST_FAIL - Continuous health testing failed

> > + */

> > +#define TA_CMD_GET_ENTROPY           0x0

> > +

> > +/*

> > + * PTA_CMD_GET_RNG_INFO - Get RNG information

>       ^^^

>

> Does not match.

>


Will correct it.

>

> > + *

> > + * param[0] (out value) - value.a: RNG data-rate

> > + *                        value.b: Quality/Entropy per 1024 bit of data

> > + * param[1] unused

> > + * param[2] unused

> > + * param[3] unused

> > + *

> > + * Result:

> > + * TEE_SUCCESS - Invoke command success

> > + * TEE_ERROR_BAD_PARAMETERS - Incorrect input param

> > + */

> > +#define TA_CMD_GET_RNG_INFO          0x1

> > +

> > +#define MAX_ENTROPY_REQ_SZ           (4 * 1024)

> > +

> > +static struct tee_context *ctx;

> > +static struct tee_shm *entropy_shm_pool;

> > +static u32 ta_rng_data_rate;

> > +static u32 ta_rng_seesion_id;

> > +

> > +static size_t get_optee_rng_data(void *buf, size_t req_size)

> > +{

> > +     u32 ret = 0;

> > +     u8 *rng_data = NULL;

> > +     size_t rng_size = 0;

> > +     struct tee_ioctl_invoke_arg inv_arg = {0};

> > +     struct tee_param param[4] = {0};

> > +

> > +     /* Invoke TA_CMD_GET_RNG function of Trusted App */

> > +     inv_arg.func = TA_CMD_GET_ENTROPY;

> > +     inv_arg.session = ta_rng_seesion_id;

> > +     inv_arg.num_params = 4;

> > +

> > +     /* Fill invoke cmd params */

> > +     param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT;

> > +     param[0].u.memref.shm = entropy_shm_pool;

> > +     param[0].u.memref.size = req_size;

> > +     param[0].u.memref.shm_offs = 0;

> > +

> > +     ret = tee_client_invoke_func(ctx, &inv_arg, param);

> > +     if ((ret < 0) || (inv_arg.ret != 0)) {

> > +             pr_err(PFX "TA_CMD_GET_ENTROPY invoke function error: %x\n",

> > +                    inv_arg.ret);

> > +             return 0;

> > +     }

> > +

> > +     rng_data = tee_shm_get_va(entropy_shm_pool, 0);

> > +     if (IS_ERR(rng_data)) {

> > +             pr_err(PFX "tee_shm_get_va failed\n");

> > +             return 0;

> > +     }

> > +

> > +     rng_size = param[0].u.memref.size;

> > +     memcpy(buf, rng_data, rng_size);

> > +

> > +     return rng_size;

> > +}

> > +

> > +static int optee_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)

> > +{

> > +     u8 *data = buf;

> > +     size_t read = 0, rng_size = 0;

> > +     int timeout = max / MAX_ENTROPY_REQ_SZ + 1;

> > +

> > +     /* New random numbers are generated approximately 1byte per 2ms */

>

> This is only true on Developerbox.

>


Will rather remove this comment.

>

> > +     while (read < max) {

>

> Not sure about this. This is a read.2 like interface. If we have data

> available why not return it to the upper level.

>


Yes it could be read.2 like interface if argument "wait=false" as you
can see below it returns if "wait=false".

>

> > +             if ((max - read) < MAX_ENTROPY_REQ_SZ)

> > +                     rng_size = get_optee_rng_data(data, (max - read));

> > +             else

> > +                     rng_size = get_optee_rng_data(data, MAX_ENTROPY_REQ_SZ);

> > +

> > +             data += rng_size;

> > +             read += rng_size;

> > +

> > +             if (wait) {

> > +                     if (timeout-- == 0)

> > +                             return read;

> > +                     if ((max - read) < MAX_ENTROPY_REQ_SZ)

> > +                             msleep((1000 * (max - read)) /

> > +                                    ta_rng_data_rate);

> > +                     else

> > +                             msleep((1000 * MAX_ENTROPY_REQ_SZ) /

> > +                                    ta_rng_data_rate);

>

> Similarly don't do this. Handle it in the header:

>

> if (max > MAX_ENTROPY_REQ_SZ)

>         max = MAX_ENTROPY_REQ_SZ;

>

> (IIRC max is never larger than 32 anyway... there might be something in

> the headers to help confirm this).

>


Header says following:

 * @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.

>

> > +             } else {

> > +                     return read;


Here.

> > +             }

> > +     }

> > +

> > +     return read;

> > +}

> > +

> > +static int optee_rng_init(struct hwrng *rng)

> > +{

> > +     entropy_shm_pool = tee_shm_alloc(ctx, MAX_ENTROPY_REQ_SZ,

> > +                                      TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);

> > +     if (IS_ERR(entropy_shm_pool)) {

> > +             pr_err(PFX "tee_shm_alloc failed\n");

> > +             return PTR_ERR(entropy_shm_pool);

> > +     }

> > +

> > +     return 0;

> > +}

> > +

> > +static void optee_rng_cleanup(struct hwrng *rng)

> > +{

> > +     tee_shm_free(entropy_shm_pool);

> > +}

> > +

> > +static struct hwrng optee_rng = {

> > +     .name           = "optee-rng",

> > +     .init           = optee_rng_init,

> > +     .cleanup        = optee_rng_cleanup,

> > +     .read           = optee_rng_read,

> > +};

> > +

> > +static const struct of_device_id optee_match[] = {

> > +     { .compatible = "linaro,optee-tz" },

> > +     {},

> > +};

> > +

> > +static int get_optee_rng_uuid(uuid_t *ta_rng_uuid)

> > +{

> > +     struct device_node *fw_np;

> > +     struct device_node *np;

> > +     const char *uuid;

> > +

> > +     /* Node is supposed to be below /firmware */

> > +     fw_np = of_find_node_by_name(NULL, "firmware");

> > +     if (!fw_np)

> > +             return -ENODEV;

> > +

> > +     np = of_find_matching_node(fw_np, optee_match);

> > +     if (!np || !of_device_is_available(np))

> > +             return -ENODEV;

> > +

> > +     if (of_property_read_string(np, "rng-uuid", &uuid)) {

> > +             pr_warn(PFX "missing \"uuid\" property\n");

> > +             return -ENXIO;

> > +     }

> > +

> > +     if (uuid_parse(uuid, ta_rng_uuid)) {

> > +             pr_warn(PFX "incorrect rng ta uuid\n");

> > +             return -EINVAL;

> > +     }

> > +

> > +     return 0;

> > +}

> > +

> > +static int get_optee_rng_info(void)

> > +{

> > +     u32 ret = 0;

> > +     struct tee_ioctl_invoke_arg inv_arg = {0};

> > +     struct tee_param param[4] = {0};

> > +

> > +     /* Invoke TA_CMD_GET_RNG function of Trusted App */

> > +     inv_arg.func = TA_CMD_GET_RNG_INFO;

> > +     inv_arg.session = ta_rng_seesion_id;

> > +     inv_arg.num_params = 4;

> > +

> > +     /* Fill invoke cmd params */

> > +     param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;

> > +

> > +     ret = tee_client_invoke_func(ctx, &inv_arg, param);

> > +     if ((ret < 0) || (inv_arg.ret != 0)) {

> > +             pr_err(PFX "TA_CMD_GET_RNG_INFO invoke function error: %x\n",

> > +                    inv_arg.ret);

> > +             return -EINVAL;

> > +     }

> > +

> > +     ta_rng_data_rate = param[0].u.value.a;

> > +     optee_rng.quality = param[0].u.value.b;

> > +

> > +     return 0;

> > +}

> > +

> > +static int tee_match(struct tee_ioctl_version_data *ver, const void *data)

> > +{

> > +     if (ver->impl_id == TEE_IMPL_ID_OPTEE)

>

> Either this is the tee-rng driver or this special tee matcher function

> is pointless and can be removed (and the comparison done inline).

>


This is tee matcher callback function as there could be multiple tee
implementations, optee being one of them.

>

> > +             return 1;

> > +     else

> > +             return 0;

> > +}

> > +

> > +static int __init mod_init(void)

> > +{

> > +     int ret = 0, err = -ENODEV;

> > +     struct tee_ioctl_open_session_arg sess_arg = {0};

> > +     uuid_t ta_rng_uuid = {0};

> > +

> > +     err = get_optee_rng_uuid(&ta_rng_uuid);

> > +     if (err)

> > +             return err;

> > +

> > +     /* Open context with TEE driver */

> > +     ctx = tee_client_open_context(NULL, tee_match, NULL, NULL);

> > +     if (IS_ERR(ctx))

> > +             return -ENODEV;

>

> Wow!

>

> A driver that doesn't use the driver model. I haven't seen one of those

> in *years*. That might be a problem but I'm not entirely sure.

>


Is following the problem you are referring here?

If optee-rng driver is built in kernel-tree and probed earlier that
optee driver. In this case, could "-EPROBE_DEFER" be useful error code
to return from here?

> Ultimately if feels like the kernel code is missing a "tee bus".

> Basically I'd expect a driver to register itself and say what UUIDs it

> supports and then the TEE bus to probe drivers based on the list of

> UUIDs it thinks it has.


It seems to be good enhancement to TEE kernel internal interface in
upstream [1]. But I think there could be multiple TEE implementation
(optee one of them), so there should be implementation specific bus
(currently we could rather have optee bus).

[1] https://github.com/torvalds/linux/commit/25559c22cef879c5cf7119540bfe21fb379d29f3

-Sumit

>

> Probably better to discuss this in the upstream though.

>

>

> > +

> > +     /* Open session with hwrng Trusted App */

> > +     memcpy(sess_arg.uuid, ta_rng_uuid.b, TEE_IOCTL_UUID_LEN);

> > +     sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;

> > +     sess_arg.num_params = 0;

> > +

> > +     ret = tee_client_open_session(ctx, &sess_arg, NULL);

> > +     if ((ret < 0) || (sess_arg.ret != 0)) {

> > +             pr_err(PFX "tee_client_open_session failed, error: %x\n",

> > +                              sess_arg.ret);

> > +             err = -EINVAL;

> > +             goto out_ctx;

> > +     }

> > +     ta_rng_seesion_id = sess_arg.session;

> > +

> > +     err = get_optee_rng_info();

> > +     if (err)

> > +             goto out_sess;

> > +

> > +     err = hwrng_register(&optee_rng);

> > +     if (err) {

> > +             pr_err(PFX " registering failed (%d)\n", err);

> > +             goto out_sess;

> > +     }

> > +

> > +     return 0;

> > +

> > +out_sess:

> > +     tee_client_close_session(ctx, ta_rng_seesion_id);

> > +out_ctx:

> > +     tee_client_close_context(ctx);

> > +

> > +     return err;

> > +}

> > +

> > +static void __exit mod_exit(void)

> > +{

> > +     tee_client_close_session(ctx, ta_rng_seesion_id);

> > +     tee_client_close_context(ctx);

> > +     hwrng_unregister(&optee_rng);

> > +}

> > +

> > +module_init(mod_init);

> > +module_exit(mod_exit);

> > +

> > +MODULE_LICENSE("GPL");

> > +MODULE_AUTHOR("Sumit Garg <sumit.garg@linaro.org>");

> > +MODULE_DESCRIPTION("OP-TEE based random number generator driver");

> > --

> > 2.7.4

> >
Daniel Thompson Dec. 20, 2018, 1:19 p.m. UTC | #3
On Thu, Dec 20, 2018 at 06:16:44PM +0530, Sumit Garg wrote:
> On Thu, 20 Dec 2018 at 17:31, Daniel Thompson

> <daniel.thompson@linaro.org> wrote:

> >

> > On Thu, Dec 20, 2018 at 03:46:38PM +0530, Sumit Garg wrote:

> > > On ARM SoC's with TrustZone enabled, peripherals like entropy sources

> > > might not be accessible to normal world (linux in this case) and rather

> > > accessible to secure world (OP-TEE in this case) only. So this driver

> > > aims to provides a generic interface to OP-TEE based random number

> > > generator service.

> > >

> > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

> > > ---

> > >  MAINTAINERS                        |   5 +

> > >  drivers/char/hw_random/Kconfig     |  15 ++

> > >  drivers/char/hw_random/Makefile    |   1 +

> > >  drivers/char/hw_random/optee-rng.c | 280 +++++++++++++++++++++++++++++++++++++

> > >  4 files changed, 301 insertions(+)

> > >  create mode 100644 drivers/char/hw_random/optee-rng.c

> > >

> > > diff --git a/MAINTAINERS b/MAINTAINERS

> > > index 0767f1d..fe0fb74 100644

> > > --- a/MAINTAINERS

> > > +++ b/MAINTAINERS

> > > @@ -11100,6 +11100,11 @@ M:   Jens Wiklander <jens.wiklander@linaro.org>

> > >  S:   Maintained

> > >  F:   drivers/tee/optee/

> > >

> > > +OP-TEE RANDOM NUMBER GENERATOR (RNG) DRIVER

> > > +M:   Sumit Garg <sumit.garg@linaro.org>

> > > +S:   Maintained

> > > +F:   drivers/char/hw_random/optee-rng.c

> > > +

> > >  OPA-VNIC DRIVER

> > >  M:   Dennis Dalessandro <dennis.dalessandro@intel.com>

> > >  M:   Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>

> > > diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig

> > > index dac895d..25a7d8f 100644

> > > --- a/drivers/char/hw_random/Kconfig

> > > +++ b/drivers/char/hw_random/Kconfig

> > > @@ -424,6 +424,21 @@ config HW_RANDOM_EXYNOS

> > >         will be called exynos-trng.

> > >

> > >         If unsure, say Y.

> > > +

> > > +config HW_RANDOM_OPTEE

> > > +     tristate "OP-TEE based Random Number Generator support"

> > > +     depends on OPTEE

> > > +     default HW_RANDOM

> > > +     help

> > > +       This  driver provides support for OP-TEE based Random Number

> > > +       Generator on ARM SoCs where hardware entropy sources are not

> > > +       accessible to normal world (Linux).

> > > +

> > > +       To compile this driver as a module, choose M here: the module

> > > +       will be called optee-rng.

> > > +

> > > +       If unsure, say Y.

> > > +

> > >  endif # HW_RANDOM

> > >

> > >  config UML_RANDOM

> > > diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile

> > > index e35ec3c..7c9ef4a 100644

> > > --- a/drivers/char/hw_random/Makefile

> > > +++ b/drivers/char/hw_random/Makefile

> > > @@ -38,3 +38,4 @@ obj-$(CONFIG_HW_RANDOM_CAVIUM) += cavium-rng.o cavium-rng-vf.o

> > >  obj-$(CONFIG_HW_RANDOM_MTK)  += mtk-rng.o

> > >  obj-$(CONFIG_HW_RANDOM_S390) += s390-trng.o

> > >  obj-$(CONFIG_HW_RANDOM_KEYSTONE) += ks-sa-rng.o

> > > +obj-$(CONFIG_HW_RANDOM_OPTEE) += optee-rng.o

> > > diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c

> > > new file mode 100644

> > > index 0000000..0dbe210

> > > --- /dev/null

> > > +++ b/drivers/char/hw_random/optee-rng.c

> > > @@ -0,0 +1,280 @@

> > > +// SPDX-License-Identifier: GPL-2.0

> > > +/*

> > > + * Copyright (C) 2018 Linaro Ltd.

> > > + */

> > > +

> > > +#include <linux/delay.h>

> > > +#include <linux/of.h>

> > > +#include <linux/hw_random.h>

> > > +#include <linux/kernel.h>

> > > +#include <linux/module.h>

> > > +#include <linux/tee_drv.h>

> > > +#include <linux/uuid.h>

> > > +

> > > +#define PFX  KBUILD_MODNAME ": "

> >

> > Definitely not! This is what pr_fmt() is for.

> >

> 

> Ok will remove this.

> 

> >

> > > +

> > > +#define TEE_ERROR_HEALTH_TEST_FAIL   0x00000001

> > > +

> > > +/*

> > > + * TA_CMD_GET_ENTROPY - Get Entropy from RNG

> > > + *

> > > + * param[0] (inout memref) - Entropy buffer memory reference

> > > + * param[1] unused

> > > + * param[2] unused

> > > + * param[3] unused

> > > + *

> > > + * Result:

> > > + * TEE_SUCCESS - Invoke command success

> > > + * TEE_ERROR_BAD_PARAMETERS - Incorrect input param

> > > + * TEE_ERROR_NOT_SUPPORTED - Requested entropy size greater than size of pool

> > > + * TEE_ERROR_HEALTH_TEST_FAIL - Continuous health testing failed

> > > + */

> > > +#define TA_CMD_GET_ENTROPY           0x0

> > > +

> > > +/*

> > > + * PTA_CMD_GET_RNG_INFO - Get RNG information

> >       ^^^

> >

> > Does not match.

> >

> 

> Will correct it.

> 

> >

> > > + *

> > > + * param[0] (out value) - value.a: RNG data-rate

> > > + *                        value.b: Quality/Entropy per 1024 bit of data

> > > + * param[1] unused

> > > + * param[2] unused

> > > + * param[3] unused

> > > + *

> > > + * Result:

> > > + * TEE_SUCCESS - Invoke command success

> > > + * TEE_ERROR_BAD_PARAMETERS - Incorrect input param

> > > + */

> > > +#define TA_CMD_GET_RNG_INFO          0x1

> > > +

> > > +#define MAX_ENTROPY_REQ_SZ           (4 * 1024)

> > > +

> > > +static struct tee_context *ctx;

> > > +static struct tee_shm *entropy_shm_pool;

> > > +static u32 ta_rng_data_rate;

> > > +static u32 ta_rng_seesion_id;

> > > +

> > > +static size_t get_optee_rng_data(void *buf, size_t req_size)

> > > +{

> > > +     u32 ret = 0;

> > > +     u8 *rng_data = NULL;

> > > +     size_t rng_size = 0;

> > > +     struct tee_ioctl_invoke_arg inv_arg = {0};

> > > +     struct tee_param param[4] = {0};

> > > +

> > > +     /* Invoke TA_CMD_GET_RNG function of Trusted App */

> > > +     inv_arg.func = TA_CMD_GET_ENTROPY;

> > > +     inv_arg.session = ta_rng_seesion_id;

> > > +     inv_arg.num_params = 4;

> > > +

> > > +     /* Fill invoke cmd params */

> > > +     param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT;

> > > +     param[0].u.memref.shm = entropy_shm_pool;

> > > +     param[0].u.memref.size = req_size;

> > > +     param[0].u.memref.shm_offs = 0;

> > > +

> > > +     ret = tee_client_invoke_func(ctx, &inv_arg, param);

> > > +     if ((ret < 0) || (inv_arg.ret != 0)) {

> > > +             pr_err(PFX "TA_CMD_GET_ENTROPY invoke function error: %x\n",

> > > +                    inv_arg.ret);

> > > +             return 0;

> > > +     }

> > > +

> > > +     rng_data = tee_shm_get_va(entropy_shm_pool, 0);

> > > +     if (IS_ERR(rng_data)) {

> > > +             pr_err(PFX "tee_shm_get_va failed\n");

> > > +             return 0;

> > > +     }

> > > +

> > > +     rng_size = param[0].u.memref.size;

> > > +     memcpy(buf, rng_data, rng_size);

> > > +

> > > +     return rng_size;

> > > +}

> > > +

> > > +static int optee_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)

> > > +{

> > > +     u8 *data = buf;

> > > +     size_t read = 0, rng_size = 0;

> > > +     int timeout = max / MAX_ENTROPY_REQ_SZ + 1;

> > > +

> > > +     /* New random numbers are generated approximately 1byte per 2ms */

> >

> > This is only true on Developerbox.

> >

> 

> Will rather remove this comment.

> 

> >

> > > +     while (read < max) {

> >

> > Not sure about this. This is a read.2 like interface. If we have data

> > available why not return it to the upper level.

> >

> 

> Yes it could be read.2 like interface if argument "wait=false" as you

> can see below it returns if "wait=false".


IIRC it is OK to interpret wait as "do not return 0 bytes".
Does it really mean that the driver must fill the whole buffer?

Basically if the calling code has to cope with the driver returning less
than requested then code in the driver to cope with it often pointless.


> > > +             if ((max - read) < MAX_ENTROPY_REQ_SZ)

> > > +                     rng_size = get_optee_rng_data(data, (max - read));

> > > +             else

> > > +                     rng_size = get_optee_rng_data(data, MAX_ENTROPY_REQ_SZ);

> > > +

> > > +             data += rng_size;

> > > +             read += rng_size;

> > > +

> > > +             if (wait) {

> > > +                     if (timeout-- == 0)

> > > +                             return read;

> > > +                     if ((max - read) < MAX_ENTROPY_REQ_SZ)

> > > +                             msleep((1000 * (max - read)) /

> > > +                                    ta_rng_data_rate);

> > > +                     else

> > > +                             msleep((1000 * MAX_ENTROPY_REQ_SZ) /

> > > +                                    ta_rng_data_rate);

> >

> > Similarly don't do this. Handle it in the header:

> >

> > if (max > MAX_ENTROPY_REQ_SZ)

> >         max = MAX_ENTROPY_REQ_SZ;

> >

> > (IIRC max is never larger than 32 anyway... there might be something in

> > the headers to help confirm this).

> >

> 

> Header says following:

> 

>  * @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.


Either way... there is no need to handle this in the inner loop. Even if the
framework gives you a large buffer I don't think the driver is required
to fill it.

> 

> >

> > > +             } else {

> > > +                     return read;

> 

> Here.

> 

> > > +             }

> > > +     }

> > > +

> > > +     return read;

> > > +}

> > > +

> > > +static int optee_rng_init(struct hwrng *rng)

> > > +{

> > > +     entropy_shm_pool = tee_shm_alloc(ctx, MAX_ENTROPY_REQ_SZ,

> > > +                                      TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);

> > > +     if (IS_ERR(entropy_shm_pool)) {

> > > +             pr_err(PFX "tee_shm_alloc failed\n");

> > > +             return PTR_ERR(entropy_shm_pool);

> > > +     }

> > > +

> > > +     return 0;

> > > +}

> > > +

> > > +static void optee_rng_cleanup(struct hwrng *rng)

> > > +{

> > > +     tee_shm_free(entropy_shm_pool);

> > > +}

> > > +

> > > +static struct hwrng optee_rng = {

> > > +     .name           = "optee-rng",

> > > +     .init           = optee_rng_init,

> > > +     .cleanup        = optee_rng_cleanup,

> > > +     .read           = optee_rng_read,

> > > +};

> > > +

> > > +static const struct of_device_id optee_match[] = {

> > > +     { .compatible = "linaro,optee-tz" },

> > > +     {},

> > > +};

> > > +

> > > +static int get_optee_rng_uuid(uuid_t *ta_rng_uuid)

> > > +{

> > > +     struct device_node *fw_np;

> > > +     struct device_node *np;

> > > +     const char *uuid;

> > > +

> > > +     /* Node is supposed to be below /firmware */

> > > +     fw_np = of_find_node_by_name(NULL, "firmware");

> > > +     if (!fw_np)

> > > +             return -ENODEV;

> > > +

> > > +     np = of_find_matching_node(fw_np, optee_match);

> > > +     if (!np || !of_device_is_available(np))

> > > +             return -ENODEV;

> > > +

> > > +     if (of_property_read_string(np, "rng-uuid", &uuid)) {

> > > +             pr_warn(PFX "missing \"uuid\" property\n");

> > > +             return -ENXIO;

> > > +     }

> > > +

> > > +     if (uuid_parse(uuid, ta_rng_uuid)) {

> > > +             pr_warn(PFX "incorrect rng ta uuid\n");

> > > +             return -EINVAL;

> > > +     }

> > > +

> > > +     return 0;

> > > +}

> > > +

> > > +static int get_optee_rng_info(void)

> > > +{

> > > +     u32 ret = 0;

> > > +     struct tee_ioctl_invoke_arg inv_arg = {0};

> > > +     struct tee_param param[4] = {0};

> > > +

> > > +     /* Invoke TA_CMD_GET_RNG function of Trusted App */

> > > +     inv_arg.func = TA_CMD_GET_RNG_INFO;

> > > +     inv_arg.session = ta_rng_seesion_id;

> > > +     inv_arg.num_params = 4;

> > > +

> > > +     /* Fill invoke cmd params */

> > > +     param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;

> > > +

> > > +     ret = tee_client_invoke_func(ctx, &inv_arg, param);

> > > +     if ((ret < 0) || (inv_arg.ret != 0)) {

> > > +             pr_err(PFX "TA_CMD_GET_RNG_INFO invoke function error: %x\n",

> > > +                    inv_arg.ret);

> > > +             return -EINVAL;

> > > +     }

> > > +

> > > +     ta_rng_data_rate = param[0].u.value.a;

> > > +     optee_rng.quality = param[0].u.value.b;

> > > +

> > > +     return 0;

> > > +}

> > > +

> > > +static int tee_match(struct tee_ioctl_version_data *ver, const void *data)

> > > +{

> > > +     if (ver->impl_id == TEE_IMPL_ID_OPTEE)

> >

> > Either this is the tee-rng driver or this special tee matcher function

> > is pointless and can be removed (and the comparison done inline).

> >

> 

> This is tee matcher callback function as there could be multiple tee

> implementations, optee being one of them.


The driver is currently called optee-rng . Either the driver is tee-rng
*or* this function is pointless. You choose ;-).

> 

> >

> > > +             return 1;

> > > +     else

> > > +             return 0;

> > > +}

> > > +

> > > +static int __init mod_init(void)

> > > +{

> > > +     int ret = 0, err = -ENODEV;

> > > +     struct tee_ioctl_open_session_arg sess_arg = {0};

> > > +     uuid_t ta_rng_uuid = {0};

> > > +

> > > +     err = get_optee_rng_uuid(&ta_rng_uuid);

> > > +     if (err)

> > > +             return err;

> > > +

> > > +     /* Open context with TEE driver */

> > > +     ctx = tee_client_open_context(NULL, tee_match, NULL, NULL);

> > > +     if (IS_ERR(ctx))

> > > +             return -ENODEV;

> >

> > Wow!

> >

> > A driver that doesn't use the driver model. I haven't seen one of those

> > in *years*. That might be a problem but I'm not entirely sure.

> >

> 

> Is following the problem you are referring here?

> 

> If optee-rng driver is built in kernel-tree and probed earlier that

> optee driver. In this case, could "-EPROBE_DEFER" be useful error code

> to return from here?


You can check but I don't think so. This is a module load function, not
a driver probe function.


> > Ultimately if feels like the kernel code is missing a "tee bus".

> > Basically I'd expect a driver to register itself and say what UUIDs it

> > supports and then the TEE bus to probe drivers based on the list of

> > UUIDs it thinks it has.

> 

> It seems to be good enhancement to TEE kernel internal interface in

> upstream [1]. But I think there could be multiple TEE implementation

> (optee one of them), so there should be implementation specific bus

> (currently we could rather have optee bus).

> 

> [1] https://github.com/torvalds/linux/commit/25559c22cef879c5cf7119540bfe21fb379d29f3


Don't really follow this. Sure, on a platform with multiple TEEs there would
be multiple TEE buses but I don't see why this should be implemented in
the OP-TEE driver.

> 

> -Sumit

> 

> >

> > Probably better to discuss this in the upstream though.

> >

> >

> > > +

> > > +     /* Open session with hwrng Trusted App */

> > > +     memcpy(sess_arg.uuid, ta_rng_uuid.b, TEE_IOCTL_UUID_LEN);

> > > +     sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;

> > > +     sess_arg.num_params = 0;

> > > +

> > > +     ret = tee_client_open_session(ctx, &sess_arg, NULL);

> > > +     if ((ret < 0) || (sess_arg.ret != 0)) {

> > > +             pr_err(PFX "tee_client_open_session failed, error: %x\n",

> > > +                              sess_arg.ret);

> > > +             err = -EINVAL;

> > > +             goto out_ctx;

> > > +     }

> > > +     ta_rng_seesion_id = sess_arg.session;

> > > +

> > > +     err = get_optee_rng_info();

> > > +     if (err)

> > > +             goto out_sess;

> > > +

> > > +     err = hwrng_register(&optee_rng);

> > > +     if (err) {

> > > +             pr_err(PFX " registering failed (%d)\n", err);

> > > +             goto out_sess;

> > > +     }

> > > +

> > > +     return 0;

> > > +

> > > +out_sess:

> > > +     tee_client_close_session(ctx, ta_rng_seesion_id);

> > > +out_ctx:

> > > +     tee_client_close_context(ctx);

> > > +

> > > +     return err;

> > > +}

> > > +

> > > +static void __exit mod_exit(void)

> > > +{

> > > +     tee_client_close_session(ctx, ta_rng_seesion_id);

> > > +     tee_client_close_context(ctx);

> > > +     hwrng_unregister(&optee_rng);

> > > +}

> > > +

> > > +module_init(mod_init);

> > > +module_exit(mod_exit);

> > > +

> > > +MODULE_LICENSE("GPL");

> > > +MODULE_AUTHOR("Sumit Garg <sumit.garg@linaro.org>");

> > > +MODULE_DESCRIPTION("OP-TEE based random number generator driver");

> > > --

> > > 2.7.4

> > >
Sumit Garg Dec. 21, 2018, 7:56 a.m. UTC | #4
On Thu, 20 Dec 2018 at 18:50, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>

> On Thu, Dec 20, 2018 at 06:16:44PM +0530, Sumit Garg wrote:

> > On Thu, 20 Dec 2018 at 17:31, Daniel Thompson

> > <daniel.thompson@linaro.org> wrote:

> > >

> > > On Thu, Dec 20, 2018 at 03:46:38PM +0530, Sumit Garg wrote:

> > > > On ARM SoC's with TrustZone enabled, peripherals like entropy sources

> > > > might not be accessible to normal world (linux in this case) and rather

> > > > accessible to secure world (OP-TEE in this case) only. So this driver

> > > > aims to provides a generic interface to OP-TEE based random number

> > > > generator service.

> > > >

> > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

> > > > ---

> > > >  MAINTAINERS                        |   5 +

> > > >  drivers/char/hw_random/Kconfig     |  15 ++

> > > >  drivers/char/hw_random/Makefile    |   1 +

> > > >  drivers/char/hw_random/optee-rng.c | 280 +++++++++++++++++++++++++++++++++++++

> > > >  4 files changed, 301 insertions(+)

> > > >  create mode 100644 drivers/char/hw_random/optee-rng.c

> > > >

> > > > diff --git a/MAINTAINERS b/MAINTAINERS

> > > > index 0767f1d..fe0fb74 100644

> > > > --- a/MAINTAINERS

> > > > +++ b/MAINTAINERS

> > > > @@ -11100,6 +11100,11 @@ M:   Jens Wiklander <jens.wiklander@linaro.org>

> > > >  S:   Maintained

> > > >  F:   drivers/tee/optee/

> > > >

> > > > +OP-TEE RANDOM NUMBER GENERATOR (RNG) DRIVER

> > > > +M:   Sumit Garg <sumit.garg@linaro.org>

> > > > +S:   Maintained

> > > > +F:   drivers/char/hw_random/optee-rng.c

> > > > +

> > > >  OPA-VNIC DRIVER

> > > >  M:   Dennis Dalessandro <dennis.dalessandro@intel.com>

> > > >  M:   Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>

> > > > diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig

> > > > index dac895d..25a7d8f 100644

> > > > --- a/drivers/char/hw_random/Kconfig

> > > > +++ b/drivers/char/hw_random/Kconfig

> > > > @@ -424,6 +424,21 @@ config HW_RANDOM_EXYNOS

> > > >         will be called exynos-trng.

> > > >

> > > >         If unsure, say Y.

> > > > +

> > > > +config HW_RANDOM_OPTEE

> > > > +     tristate "OP-TEE based Random Number Generator support"

> > > > +     depends on OPTEE

> > > > +     default HW_RANDOM

> > > > +     help

> > > > +       This  driver provides support for OP-TEE based Random Number

> > > > +       Generator on ARM SoCs where hardware entropy sources are not

> > > > +       accessible to normal world (Linux).

> > > > +

> > > > +       To compile this driver as a module, choose M here: the module

> > > > +       will be called optee-rng.

> > > > +

> > > > +       If unsure, say Y.

> > > > +

> > > >  endif # HW_RANDOM

> > > >

> > > >  config UML_RANDOM

> > > > diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile

> > > > index e35ec3c..7c9ef4a 100644

> > > > --- a/drivers/char/hw_random/Makefile

> > > > +++ b/drivers/char/hw_random/Makefile

> > > > @@ -38,3 +38,4 @@ obj-$(CONFIG_HW_RANDOM_CAVIUM) += cavium-rng.o cavium-rng-vf.o

> > > >  obj-$(CONFIG_HW_RANDOM_MTK)  += mtk-rng.o

> > > >  obj-$(CONFIG_HW_RANDOM_S390) += s390-trng.o

> > > >  obj-$(CONFIG_HW_RANDOM_KEYSTONE) += ks-sa-rng.o

> > > > +obj-$(CONFIG_HW_RANDOM_OPTEE) += optee-rng.o

> > > > diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c

> > > > new file mode 100644

> > > > index 0000000..0dbe210

> > > > --- /dev/null

> > > > +++ b/drivers/char/hw_random/optee-rng.c

> > > > @@ -0,0 +1,280 @@

> > > > +// SPDX-License-Identifier: GPL-2.0

> > > > +/*

> > > > + * Copyright (C) 2018 Linaro Ltd.

> > > > + */

> > > > +

> > > > +#include <linux/delay.h>

> > > > +#include <linux/of.h>

> > > > +#include <linux/hw_random.h>

> > > > +#include <linux/kernel.h>

> > > > +#include <linux/module.h>

> > > > +#include <linux/tee_drv.h>

> > > > +#include <linux/uuid.h>

> > > > +

> > > > +#define PFX  KBUILD_MODNAME ": "

> > >

> > > Definitely not! This is what pr_fmt() is for.

> > >

> >

> > Ok will remove this.

> >

> > >

> > > > +

> > > > +#define TEE_ERROR_HEALTH_TEST_FAIL   0x00000001

> > > > +

> > > > +/*

> > > > + * TA_CMD_GET_ENTROPY - Get Entropy from RNG

> > > > + *

> > > > + * param[0] (inout memref) - Entropy buffer memory reference

> > > > + * param[1] unused

> > > > + * param[2] unused

> > > > + * param[3] unused

> > > > + *

> > > > + * Result:

> > > > + * TEE_SUCCESS - Invoke command success

> > > > + * TEE_ERROR_BAD_PARAMETERS - Incorrect input param

> > > > + * TEE_ERROR_NOT_SUPPORTED - Requested entropy size greater than size of pool

> > > > + * TEE_ERROR_HEALTH_TEST_FAIL - Continuous health testing failed

> > > > + */

> > > > +#define TA_CMD_GET_ENTROPY           0x0

> > > > +

> > > > +/*

> > > > + * PTA_CMD_GET_RNG_INFO - Get RNG information

> > >       ^^^

> > >

> > > Does not match.

> > >

> >

> > Will correct it.

> >

> > >

> > > > + *

> > > > + * param[0] (out value) - value.a: RNG data-rate

> > > > + *                        value.b: Quality/Entropy per 1024 bit of data

> > > > + * param[1] unused

> > > > + * param[2] unused

> > > > + * param[3] unused

> > > > + *

> > > > + * Result:

> > > > + * TEE_SUCCESS - Invoke command success

> > > > + * TEE_ERROR_BAD_PARAMETERS - Incorrect input param

> > > > + */

> > > > +#define TA_CMD_GET_RNG_INFO          0x1

> > > > +

> > > > +#define MAX_ENTROPY_REQ_SZ           (4 * 1024)

> > > > +

> > > > +static struct tee_context *ctx;

> > > > +static struct tee_shm *entropy_shm_pool;

> > > > +static u32 ta_rng_data_rate;

> > > > +static u32 ta_rng_seesion_id;

> > > > +

> > > > +static size_t get_optee_rng_data(void *buf, size_t req_size)

> > > > +{

> > > > +     u32 ret = 0;

> > > > +     u8 *rng_data = NULL;

> > > > +     size_t rng_size = 0;

> > > > +     struct tee_ioctl_invoke_arg inv_arg = {0};

> > > > +     struct tee_param param[4] = {0};

> > > > +

> > > > +     /* Invoke TA_CMD_GET_RNG function of Trusted App */

> > > > +     inv_arg.func = TA_CMD_GET_ENTROPY;

> > > > +     inv_arg.session = ta_rng_seesion_id;

> > > > +     inv_arg.num_params = 4;

> > > > +

> > > > +     /* Fill invoke cmd params */

> > > > +     param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT;

> > > > +     param[0].u.memref.shm = entropy_shm_pool;

> > > > +     param[0].u.memref.size = req_size;

> > > > +     param[0].u.memref.shm_offs = 0;

> > > > +

> > > > +     ret = tee_client_invoke_func(ctx, &inv_arg, param);

> > > > +     if ((ret < 0) || (inv_arg.ret != 0)) {

> > > > +             pr_err(PFX "TA_CMD_GET_ENTROPY invoke function error: %x\n",

> > > > +                    inv_arg.ret);

> > > > +             return 0;

> > > > +     }

> > > > +

> > > > +     rng_data = tee_shm_get_va(entropy_shm_pool, 0);

> > > > +     if (IS_ERR(rng_data)) {

> > > > +             pr_err(PFX "tee_shm_get_va failed\n");

> > > > +             return 0;

> > > > +     }

> > > > +

> > > > +     rng_size = param[0].u.memref.size;

> > > > +     memcpy(buf, rng_data, rng_size);

> > > > +

> > > > +     return rng_size;

> > > > +}

> > > > +

> > > > +static int optee_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)

> > > > +{

> > > > +     u8 *data = buf;

> > > > +     size_t read = 0, rng_size = 0;

> > > > +     int timeout = max / MAX_ENTROPY_REQ_SZ + 1;

> > > > +

> > > > +     /* New random numbers are generated approximately 1byte per 2ms */

> > >

> > > This is only true on Developerbox.

> > >

> >

> > Will rather remove this comment.

> >

> > >

> > > > +     while (read < max) {

> > >

> > > Not sure about this. This is a read.2 like interface. If we have data

> > > available why not return it to the upper level.

> > >

> >

> > Yes it could be read.2 like interface if argument "wait=false" as you

> > can see below it returns if "wait=false".

>

> IIRC it is OK to interpret wait as "do not return 0 bytes".

> Does it really mean that the driver must fill the whole buffer?

>

> Basically if the calling code has to cope with the driver returning less

> than requested then code in the driver to cope with it often pointless.

>


It seems you are correct. After looking driver core
(drivers/char/hw_random/core.c) it seems that maximum size requested
can't be greater than "SMP_CACHE_BYTES" (L1_CACHE_BYTES in case of
arm64 that is 64 bytes). And its OK to interpret wait as "do not
return 0 bytes".

So I will update loop to "while (read == 0)".

>

> > > > +             if ((max - read) < MAX_ENTROPY_REQ_SZ)

> > > > +                     rng_size = get_optee_rng_data(data, (max - read));

> > > > +             else

> > > > +                     rng_size = get_optee_rng_data(data, MAX_ENTROPY_REQ_SZ);

> > > > +

> > > > +             data += rng_size;

> > > > +             read += rng_size;

> > > > +

> > > > +             if (wait) {

> > > > +                     if (timeout-- == 0)

> > > > +                             return read;

> > > > +                     if ((max - read) < MAX_ENTROPY_REQ_SZ)

> > > > +                             msleep((1000 * (max - read)) /

> > > > +                                    ta_rng_data_rate);

> > > > +                     else

> > > > +                             msleep((1000 * MAX_ENTROPY_REQ_SZ) /

> > > > +                                    ta_rng_data_rate);

> > >

> > > Similarly don't do this. Handle it in the header:

> > >

> > > if (max > MAX_ENTROPY_REQ_SZ)

> > >         max = MAX_ENTROPY_REQ_SZ;

> > >

> > > (IIRC max is never larger than 32 anyway... there might be something in

> > > the headers to help confirm this).

> > >

> >

> > Header says following:

> >

> >  * @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.

>

> Either way... there is no need to handle this in the inner loop. Even if the

> framework gives you a large buffer I don't think the driver is required

> to fill it.

>


Agree, will rather handle it in header.

> >

> > >

> > > > +             } else {

> > > > +                     return read;

> >

> > Here.

> >

> > > > +             }

> > > > +     }

> > > > +

> > > > +     return read;

> > > > +}

> > > > +

> > > > +static int optee_rng_init(struct hwrng *rng)

> > > > +{

> > > > +     entropy_shm_pool = tee_shm_alloc(ctx, MAX_ENTROPY_REQ_SZ,

> > > > +                                      TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);

> > > > +     if (IS_ERR(entropy_shm_pool)) {

> > > > +             pr_err(PFX "tee_shm_alloc failed\n");

> > > > +             return PTR_ERR(entropy_shm_pool);

> > > > +     }

> > > > +

> > > > +     return 0;

> > > > +}

> > > > +

> > > > +static void optee_rng_cleanup(struct hwrng *rng)

> > > > +{

> > > > +     tee_shm_free(entropy_shm_pool);

> > > > +}

> > > > +

> > > > +static struct hwrng optee_rng = {

> > > > +     .name           = "optee-rng",

> > > > +     .init           = optee_rng_init,

> > > > +     .cleanup        = optee_rng_cleanup,

> > > > +     .read           = optee_rng_read,

> > > > +};

> > > > +

> > > > +static const struct of_device_id optee_match[] = {

> > > > +     { .compatible = "linaro,optee-tz" },

> > > > +     {},

> > > > +};

> > > > +

> > > > +static int get_optee_rng_uuid(uuid_t *ta_rng_uuid)

> > > > +{

> > > > +     struct device_node *fw_np;

> > > > +     struct device_node *np;

> > > > +     const char *uuid;

> > > > +

> > > > +     /* Node is supposed to be below /firmware */

> > > > +     fw_np = of_find_node_by_name(NULL, "firmware");

> > > > +     if (!fw_np)

> > > > +             return -ENODEV;

> > > > +

> > > > +     np = of_find_matching_node(fw_np, optee_match);

> > > > +     if (!np || !of_device_is_available(np))

> > > > +             return -ENODEV;

> > > > +

> > > > +     if (of_property_read_string(np, "rng-uuid", &uuid)) {

> > > > +             pr_warn(PFX "missing \"uuid\" property\n");

> > > > +             return -ENXIO;

> > > > +     }

> > > > +

> > > > +     if (uuid_parse(uuid, ta_rng_uuid)) {

> > > > +             pr_warn(PFX "incorrect rng ta uuid\n");

> > > > +             return -EINVAL;

> > > > +     }

> > > > +

> > > > +     return 0;

> > > > +}

> > > > +

> > > > +static int get_optee_rng_info(void)

> > > > +{

> > > > +     u32 ret = 0;

> > > > +     struct tee_ioctl_invoke_arg inv_arg = {0};

> > > > +     struct tee_param param[4] = {0};

> > > > +

> > > > +     /* Invoke TA_CMD_GET_RNG function of Trusted App */

> > > > +     inv_arg.func = TA_CMD_GET_RNG_INFO;

> > > > +     inv_arg.session = ta_rng_seesion_id;

> > > > +     inv_arg.num_params = 4;

> > > > +

> > > > +     /* Fill invoke cmd params */

> > > > +     param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;

> > > > +

> > > > +     ret = tee_client_invoke_func(ctx, &inv_arg, param);

> > > > +     if ((ret < 0) || (inv_arg.ret != 0)) {

> > > > +             pr_err(PFX "TA_CMD_GET_RNG_INFO invoke function error: %x\n",

> > > > +                    inv_arg.ret);

> > > > +             return -EINVAL;

> > > > +     }

> > > > +

> > > > +     ta_rng_data_rate = param[0].u.value.a;

> > > > +     optee_rng.quality = param[0].u.value.b;

> > > > +

> > > > +     return 0;

> > > > +}

> > > > +

> > > > +static int tee_match(struct tee_ioctl_version_data *ver, const void *data)

> > > > +{

> > > > +     if (ver->impl_id == TEE_IMPL_ID_OPTEE)

> > >

> > > Either this is the tee-rng driver or this special tee matcher function

> > > is pointless and can be removed (and the comparison done inline).

> > >

> >

> > This is tee matcher callback function as there could be multiple tee

> > implementations, optee being one of them.

>

> The driver is currently called optee-rng . Either the driver is tee-rng

> *or* this function is pointless. You choose ;-).

>


Will rather rename this function to "optee_match" as we need to tell
tee api to open context with optee driver only via this callback
function.

> >

> > >

> > > > +             return 1;

> > > > +     else

> > > > +             return 0;

> > > > +}

> > > > +

> > > > +static int __init mod_init(void)

> > > > +{

> > > > +     int ret = 0, err = -ENODEV;

> > > > +     struct tee_ioctl_open_session_arg sess_arg = {0};

> > > > +     uuid_t ta_rng_uuid = {0};

> > > > +

> > > > +     err = get_optee_rng_uuid(&ta_rng_uuid);

> > > > +     if (err)

> > > > +             return err;

> > > > +

> > > > +     /* Open context with TEE driver */

> > > > +     ctx = tee_client_open_context(NULL, tee_match, NULL, NULL);

> > > > +     if (IS_ERR(ctx))

> > > > +             return -ENODEV;

> > >

> > > Wow!

> > >

> > > A driver that doesn't use the driver model. I haven't seen one of those

> > > in *years*. That might be a problem but I'm not entirely sure.

> > >

> >

> > Is following the problem you are referring here?

> >

> > If optee-rng driver is built in kernel-tree and probed earlier that

> > optee driver. In this case, could "-EPROBE_DEFER" be useful error code

> > to return from here?

>

> You can check but I don't think so. This is a module load function, not

> a driver probe function.

>


You are correct. I think following should work in this case:

MODULE_SOFTDEP("pre: optee");

BTW, it looks like "optee_probe" (drivers/tee/optee/core.c) is
misnomer rather being called from module load function only.

>

> > > Ultimately if feels like the kernel code is missing a "tee bus".

> > > Basically I'd expect a driver to register itself and say what UUIDs it

> > > supports and then the TEE bus to probe drivers based on the list of

> > > UUIDs it thinks it has.

> >

> > It seems to be good enhancement to TEE kernel internal interface in

> > upstream [1]. But I think there could be multiple TEE implementation

> > (optee one of them), so there should be implementation specific bus

> > (currently we could rather have optee bus).

> >

> > [1] https://github.com/torvalds/linux/commit/25559c22cef879c5cf7119540bfe21fb379d29f3

>

> Don't really follow this. Sure, on a platform with multiple TEEs there would

> be multiple TEE buses but I don't see why this should be implemented in

> the OP-TEE driver.

>


Yeah it should rather be multiple TEE buses with corresponding
implementation identifier.

> >

> > -Sumit

> >

> > >

> > > Probably better to discuss this in the upstream though.

> > >


Agree it should be better to discuss it in upstream to get more views.

-Sumit

> > >

> > > > +

> > > > +     /* Open session with hwrng Trusted App */

> > > > +     memcpy(sess_arg.uuid, ta_rng_uuid.b, TEE_IOCTL_UUID_LEN);

> > > > +     sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;

> > > > +     sess_arg.num_params = 0;

> > > > +

> > > > +     ret = tee_client_open_session(ctx, &sess_arg, NULL);

> > > > +     if ((ret < 0) || (sess_arg.ret != 0)) {

> > > > +             pr_err(PFX "tee_client_open_session failed, error: %x\n",

> > > > +                              sess_arg.ret);

> > > > +             err = -EINVAL;

> > > > +             goto out_ctx;

> > > > +     }

> > > > +     ta_rng_seesion_id = sess_arg.session;

> > > > +

> > > > +     err = get_optee_rng_info();

> > > > +     if (err)

> > > > +             goto out_sess;

> > > > +

> > > > +     err = hwrng_register(&optee_rng);

> > > > +     if (err) {

> > > > +             pr_err(PFX " registering failed (%d)\n", err);

> > > > +             goto out_sess;

> > > > +     }

> > > > +

> > > > +     return 0;

> > > > +

> > > > +out_sess:

> > > > +     tee_client_close_session(ctx, ta_rng_seesion_id);

> > > > +out_ctx:

> > > > +     tee_client_close_context(ctx);

> > > > +

> > > > +     return err;

> > > > +}

> > > > +

> > > > +static void __exit mod_exit(void)

> > > > +{

> > > > +     tee_client_close_session(ctx, ta_rng_seesion_id);

> > > > +     tee_client_close_context(ctx);

> > > > +     hwrng_unregister(&optee_rng);

> > > > +}

> > > > +

> > > > +module_init(mod_init);

> > > > +module_exit(mod_exit);

> > > > +

> > > > +MODULE_LICENSE("GPL");

> > > > +MODULE_AUTHOR("Sumit Garg <sumit.garg@linaro.org>");

> > > > +MODULE_DESCRIPTION("OP-TEE based random number generator driver");

> > > > --

> > > > 2.7.4

> > > >
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 0767f1d..fe0fb74 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11100,6 +11100,11 @@  M:	Jens Wiklander <jens.wiklander@linaro.org>
 S:	Maintained
 F:	drivers/tee/optee/
 
+OP-TEE RANDOM NUMBER GENERATOR (RNG) DRIVER
+M:	Sumit Garg <sumit.garg@linaro.org>
+S:	Maintained
+F:	drivers/char/hw_random/optee-rng.c
+
 OPA-VNIC DRIVER
 M:	Dennis Dalessandro <dennis.dalessandro@intel.com>
 M:	Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index dac895d..25a7d8f 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -424,6 +424,21 @@  config HW_RANDOM_EXYNOS
 	  will be called exynos-trng.
 
 	  If unsure, say Y.
+
+config HW_RANDOM_OPTEE
+	tristate "OP-TEE based Random Number Generator support"
+	depends on OPTEE
+	default HW_RANDOM
+	help
+	  This  driver provides support for OP-TEE based Random Number
+	  Generator on ARM SoCs where hardware entropy sources are not
+	  accessible to normal world (Linux).
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called optee-rng.
+
+	  If unsure, say Y.
+
 endif # HW_RANDOM
 
 config UML_RANDOM
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index e35ec3c..7c9ef4a 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -38,3 +38,4 @@  obj-$(CONFIG_HW_RANDOM_CAVIUM) += cavium-rng.o cavium-rng-vf.o
 obj-$(CONFIG_HW_RANDOM_MTK)	+= mtk-rng.o
 obj-$(CONFIG_HW_RANDOM_S390) += s390-trng.o
 obj-$(CONFIG_HW_RANDOM_KEYSTONE) += ks-sa-rng.o
+obj-$(CONFIG_HW_RANDOM_OPTEE) += optee-rng.o
diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c
new file mode 100644
index 0000000..0dbe210
--- /dev/null
+++ b/drivers/char/hw_random/optee-rng.c
@@ -0,0 +1,280 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Linaro Ltd.
+ */
+
+#include <linux/delay.h>
+#include <linux/of.h>
+#include <linux/hw_random.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/tee_drv.h>
+#include <linux/uuid.h>
+
+#define PFX	KBUILD_MODNAME ": "
+
+#define TEE_ERROR_HEALTH_TEST_FAIL	0x00000001
+
+/*
+ * TA_CMD_GET_ENTROPY - Get Entropy from RNG
+ *
+ * param[0] (inout memref) - Entropy buffer memory reference
+ * param[1] unused
+ * param[2] unused
+ * param[3] unused
+ *
+ * Result:
+ * TEE_SUCCESS - Invoke command success
+ * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
+ * TEE_ERROR_NOT_SUPPORTED - Requested entropy size greater than size of pool
+ * TEE_ERROR_HEALTH_TEST_FAIL - Continuous health testing failed
+ */
+#define TA_CMD_GET_ENTROPY		0x0
+
+/*
+ * PTA_CMD_GET_RNG_INFO - Get RNG information
+ *
+ * param[0] (out value) - value.a: RNG data-rate
+ *                        value.b: Quality/Entropy per 1024 bit of data
+ * param[1] unused
+ * param[2] unused
+ * param[3] unused
+ *
+ * Result:
+ * TEE_SUCCESS - Invoke command success
+ * TEE_ERROR_BAD_PARAMETERS - Incorrect input param
+ */
+#define TA_CMD_GET_RNG_INFO		0x1
+
+#define MAX_ENTROPY_REQ_SZ		(4 * 1024)
+
+static struct tee_context *ctx;
+static struct tee_shm *entropy_shm_pool;
+static u32 ta_rng_data_rate;
+static u32 ta_rng_seesion_id;
+
+static size_t get_optee_rng_data(void *buf, size_t req_size)
+{
+	u32 ret = 0;
+	u8 *rng_data = NULL;
+	size_t rng_size = 0;
+	struct tee_ioctl_invoke_arg inv_arg = {0};
+	struct tee_param param[4] = {0};
+
+	/* Invoke TA_CMD_GET_RNG function of Trusted App */
+	inv_arg.func = TA_CMD_GET_ENTROPY;
+	inv_arg.session = ta_rng_seesion_id;
+	inv_arg.num_params = 4;
+
+	/* Fill invoke cmd params */
+	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT;
+	param[0].u.memref.shm = entropy_shm_pool;
+	param[0].u.memref.size = req_size;
+	param[0].u.memref.shm_offs = 0;
+
+	ret = tee_client_invoke_func(ctx, &inv_arg, param);
+	if ((ret < 0) || (inv_arg.ret != 0)) {
+		pr_err(PFX "TA_CMD_GET_ENTROPY invoke function error: %x\n",
+		       inv_arg.ret);
+		return 0;
+	}
+
+	rng_data = tee_shm_get_va(entropy_shm_pool, 0);
+	if (IS_ERR(rng_data)) {
+		pr_err(PFX "tee_shm_get_va failed\n");
+		return 0;
+	}
+
+	rng_size = param[0].u.memref.size;
+	memcpy(buf, rng_data, rng_size);
+
+	return rng_size;
+}
+
+static int optee_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
+{
+	u8 *data = buf;
+	size_t read = 0, rng_size = 0;
+	int timeout = max / MAX_ENTROPY_REQ_SZ + 1;
+
+	/* New random numbers are generated approximately 1byte per 2ms */
+	while (read < max) {
+		if ((max - read) < MAX_ENTROPY_REQ_SZ)
+			rng_size = get_optee_rng_data(data, (max - read));
+		else
+			rng_size = get_optee_rng_data(data, MAX_ENTROPY_REQ_SZ);
+
+		data += rng_size;
+		read += rng_size;
+
+		if (wait) {
+			if (timeout-- == 0)
+				return read;
+			if ((max - read) < MAX_ENTROPY_REQ_SZ)
+				msleep((1000 * (max - read)) /
+				       ta_rng_data_rate);
+			else
+				msleep((1000 * MAX_ENTROPY_REQ_SZ) /
+				       ta_rng_data_rate);
+		} else {
+			return read;
+		}
+	}
+
+	return read;
+}
+
+static int optee_rng_init(struct hwrng *rng)
+{
+	entropy_shm_pool = tee_shm_alloc(ctx, MAX_ENTROPY_REQ_SZ,
+					 TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
+	if (IS_ERR(entropy_shm_pool)) {
+		pr_err(PFX "tee_shm_alloc failed\n");
+		return PTR_ERR(entropy_shm_pool);
+	}
+
+	return 0;
+}
+
+static void optee_rng_cleanup(struct hwrng *rng)
+{
+	tee_shm_free(entropy_shm_pool);
+}
+
+static struct hwrng optee_rng = {
+	.name		= "optee-rng",
+	.init		= optee_rng_init,
+	.cleanup	= optee_rng_cleanup,
+	.read		= optee_rng_read,
+};
+
+static const struct of_device_id optee_match[] = {
+	{ .compatible = "linaro,optee-tz" },
+	{},
+};
+
+static int get_optee_rng_uuid(uuid_t *ta_rng_uuid)
+{
+	struct device_node *fw_np;
+	struct device_node *np;
+	const char *uuid;
+
+	/* Node is supposed to be below /firmware */
+	fw_np = of_find_node_by_name(NULL, "firmware");
+	if (!fw_np)
+		return -ENODEV;
+
+	np = of_find_matching_node(fw_np, optee_match);
+	if (!np || !of_device_is_available(np))
+		return -ENODEV;
+
+	if (of_property_read_string(np, "rng-uuid", &uuid)) {
+		pr_warn(PFX "missing \"uuid\" property\n");
+		return -ENXIO;
+	}
+
+	if (uuid_parse(uuid, ta_rng_uuid)) {
+		pr_warn(PFX "incorrect rng ta uuid\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int get_optee_rng_info(void)
+{
+	u32 ret = 0;
+	struct tee_ioctl_invoke_arg inv_arg = {0};
+	struct tee_param param[4] = {0};
+
+	/* Invoke TA_CMD_GET_RNG function of Trusted App */
+	inv_arg.func = TA_CMD_GET_RNG_INFO;
+	inv_arg.session = ta_rng_seesion_id;
+	inv_arg.num_params = 4;
+
+	/* Fill invoke cmd params */
+	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
+
+	ret = tee_client_invoke_func(ctx, &inv_arg, param);
+	if ((ret < 0) || (inv_arg.ret != 0)) {
+		pr_err(PFX "TA_CMD_GET_RNG_INFO invoke function error: %x\n",
+		       inv_arg.ret);
+		return -EINVAL;
+	}
+
+	ta_rng_data_rate = param[0].u.value.a;
+	optee_rng.quality = param[0].u.value.b;
+
+	return 0;
+}
+
+static int tee_match(struct tee_ioctl_version_data *ver, const void *data)
+{
+	if (ver->impl_id == TEE_IMPL_ID_OPTEE)
+		return 1;
+	else
+		return 0;
+}
+
+static int __init mod_init(void)
+{
+	int ret = 0, err = -ENODEV;
+	struct tee_ioctl_open_session_arg sess_arg = {0};
+	uuid_t ta_rng_uuid = {0};
+
+	err = get_optee_rng_uuid(&ta_rng_uuid);
+	if (err)
+		return err;
+
+	/* Open context with TEE driver */
+	ctx = tee_client_open_context(NULL, tee_match, NULL, NULL);
+	if (IS_ERR(ctx))
+		return -ENODEV;
+
+	/* Open session with hwrng Trusted App */
+	memcpy(sess_arg.uuid, ta_rng_uuid.b, TEE_IOCTL_UUID_LEN);
+	sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
+	sess_arg.num_params = 0;
+
+	ret = tee_client_open_session(ctx, &sess_arg, NULL);
+	if ((ret < 0) || (sess_arg.ret != 0)) {
+		pr_err(PFX "tee_client_open_session failed, error: %x\n",
+				 sess_arg.ret);
+		err = -EINVAL;
+		goto out_ctx;
+	}
+	ta_rng_seesion_id = sess_arg.session;
+
+	err = get_optee_rng_info();
+	if (err)
+		goto out_sess;
+
+	err = hwrng_register(&optee_rng);
+	if (err) {
+		pr_err(PFX " registering failed (%d)\n", err);
+		goto out_sess;
+	}
+
+	return 0;
+
+out_sess:
+	tee_client_close_session(ctx, ta_rng_seesion_id);
+out_ctx:
+	tee_client_close_context(ctx);
+
+	return err;
+}
+
+static void __exit mod_exit(void)
+{
+	tee_client_close_session(ctx, ta_rng_seesion_id);
+	tee_client_close_context(ctx);
+	hwrng_unregister(&optee_rng);
+}
+
+module_init(mod_init);
+module_exit(mod_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Sumit Garg <sumit.garg@linaro.org>");
+MODULE_DESCRIPTION("OP-TEE based random number generator driver");