[PATCH 1/3] rng: add OP-TEE based Random Number Generator

[Patrick DELAUNAY]

Hi,

On 3/22/22 15:38, Heinrich Schuchardt wrote:
> On 3/22/22 15:06, Patrick Delaunay wrote:
>> Add driver for OP-TEE based Random Number Generator on ARM SoCs
>> where hardware entropy sources are not accessible to normal world
>> and the RNG service is provided by a HWRNG Trusted Application (TA).
>>
>> This driver is based on the linux driver: char/hw_random/optee-rng.c
>>
>> Signed-off-by: Patrick Delaunay <patrick.delaunay at foss.st.com>
>> ---
>>
>>   MAINTAINERS             |   1 +
>>   drivers/rng/Kconfig     |   8 +++
>>   drivers/rng/Makefile    |   1 +
>>   drivers/rng/optee_rng.c | 156 ++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 166 insertions(+)
>>   create mode 100644 drivers/rng/optee_rng.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index f25ca7cd00..3356c65dd0 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -481,6 +481,7 @@ F:    drivers/power/regulator/stpmic1.c
>>   F:    drivers/ram/stm32mp1/
>>   F:    drivers/remoteproc/stm32_copro.c
>>   F:    drivers/reset/stm32-reset.c
>> +F:    drivers/rng/optee_rng.c
>>   F:    drivers/rng/stm32mp1_rng.c
>>   F:    drivers/rtc/stm32_rtc.c
>>   F:    drivers/serial/serial_stm32.*
>> diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
>> index b1c5ab93d1..a02c585f61 100644
>> --- a/drivers/rng/Kconfig
>> +++ b/drivers/rng/Kconfig
>> @@ -31,6 +31,14 @@ config RNG_MSM
>>         This driver provides support for the Random Number
>>         Generator hardware found on Qualcomm SoCs.
>>
>> +config RNG_OPTEE
>> +    bool "OP-TEE based Random Number Generator support"
>> +    depends on DM_RNG && OPTEE
>> +    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.
>
>
> https://optee.readthedocs.io/en/latest/architecture/porting_guidelines.html 
>
> has:
>
> "By default OP-TEE is configured with a software PRNG. The entropy is
> added to software PRNG at various places, but unfortunately it is still
> quite easy to predict the data added as entropy. As a consequence,
> unless the RNG is based on hardware the generated random will be quite
> weak."
>
> Having a similiar warning in the long text for the CONFIG_RNG_OPTEE
> symbol would be helpful.
>

I propose something as :


+config RNG_OPTEE
+    bool "OP-TEE based Random Number Generator support"
+    depends on DM_RNG && OPTEE
+    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 but reserved and used by the OP-TEE

+      to avoid weakness of the software PRNG.


>> +
>>   config RNG_STM32MP1
>>       bool "Enable random number generator for STM32MP1"
>>       depends on ARCH_STM32MP
>> diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
>> index 39f7ee3f03..435b3b965a 100644
>> --- a/drivers/rng/Makefile
>> +++ b/drivers/rng/Makefile
>> @@ -7,6 +7,7 @@ obj-$(CONFIG_DM_RNG) += rng-uclass.o
>>   obj-$(CONFIG_RNG_MESON) += meson-rng.o
>>   obj-$(CONFIG_RNG_SANDBOX) += sandbox_rng.o
>>   obj-$(CONFIG_RNG_MSM) += msm_rng.o
>> +obj-$(CONFIG_RNG_OPTEE) += optee_rng.o
>>   obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
>>   obj-$(CONFIG_RNG_ROCKCHIP) += rockchip_rng.o
>>   obj-$(CONFIG_RNG_IPROC200) += iproc_rng200.o
>> diff --git a/drivers/rng/optee_rng.c b/drivers/rng/optee_rng.c
>> new file mode 100644
>> index 0000000000..a0833d0862
>> --- /dev/null
>> +++ b/drivers/rng/optee_rng.c
>> @@ -0,0 +1,156 @@
>> +// SPDX-License-Identifier: GPL-2.0+ OR BSD-3-Clause
>> +/*
>> + * Copyright (C) 2022, STMicroelectronics - All Rights Reserved
>> + */
>> +#define LOG_CATEGORY UCLASS_RNG
>> +
>> +#include <common.h>
>> +
>> +#include <rng.h>
>> +#include <tee.h>
>> +#include <dm/device.h>
>> +#include <dm/device_compat.h>
>> +#include <linux/sizes.h>
>> +
>> +#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
>> +
>> +#define MAX_ENTROPY_REQ_SZ        SZ_4K
>> +
>> +#define TA_HWRNG_UUID { 0xab7a617c, 0xb8e7, 0x4d8f, \
>> +            { 0x83, 0x01, 0xd0, 0x9b, 0x61, 0x03, 0x6b, 0x64 } }
>> +
>> +/**
>> + * struct optee_rng_priv - OP-TEE Random Number Generator private data
>> + * @session_id:        RNG TA session identifier.
>> + */
>> +struct optee_rng_priv {
>> +    u32 session_id;
>> +};
>> +
>
> Please, provide as Sphinx style function description.
>

OK


>> +static int get_optee_rng_data(struct udevice *dev,
>> +                  struct tee_shm *entropy_shm_pool,
>> +                  void *buf, size_t *size)
>> +{
>> +    struct optee_rng_priv *priv = dev_get_priv(dev);
>> +    int ret = 0;
>> +    struct tee_invoke_arg arg;
>
> Wouldn't it be preferable to use
>
> struct tee_invoke_arg arg = { 0 }; ?
>
> This way the compiler can optimize the initialization and set all 0
> initialized variables with a single memset() call.
>

I wasn't sure of the portability for struct zero-initialisation.

But I will change it in V2.


> Best regards
>
> Heinrich
>
>> +    struct tee_param param;
>> +
>> +    memset(&arg, 0, sizeof(arg));
>> +    memset(&param, 0, sizeof(param));
>> +
>> +    /* Invoke TA_CMD_GET_ENTROPY function of Trusted App */
>> +    arg.func = TA_CMD_GET_ENTROPY;
>> +    arg.session = priv->session_id;
>> +
>> +    /* Fill invoke cmd params */
>> +    param.attr = TEE_PARAM_ATTR_TYPE_MEMREF_INOUT;
>> +    param.u.memref.shm = entropy_shm_pool;
>> +    param.u.memref.size = *size;
>> +
>> +    ret = tee_invoke_func(dev->parent, &arg, 1, &param);
>> +    if (ret || arg.ret) {
>> +        if (!ret)
>> +            ret = -EPROTO;
>> +        dev_err(dev, "TA_CMD_GET_ENTROPY invoke err: %d 0x%x\n", 
>> ret, arg.ret);
>> +        *size = 0;
>> +
>> +        return ret;
>> +    }
>> +
>> +    memcpy(buf, param.u.memref.shm->addr, param.u.memref.size);
>> +    *size = param.u.memref.size;
>> +
>> +    return 0;
>> +}
>> + 


(...)

Regards


Patrick